| Message ID | 20240311-arm32-cfi-v3-2-224a0f0a45c2@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CFI for ARM32 using LLVM | expand |
On Mon, Mar 11, 2024 at 10:15:39AM +0100, Linus Walleij wrote: > Instead of just using defines to define the TLB flush functions, > use static inlines. > > This has the upside that we can tag those as __nocfi so we can > execute a CFI-enabled kernel. Why? This seems to be brain dead. Why can't CLANG cope with directly calling e.g. cpu_tlb.flush_user_range? Why does it need a static function to do exactly the same as the macro does?
On Mon, 11 Mar 2024 at 10:39, Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > On Mon, Mar 11, 2024 at 10:15:39AM +0100, Linus Walleij wrote: > > Instead of just using defines to define the TLB flush functions, > > use static inlines. > > > > This has the upside that we can tag those as __nocfi so we can > > execute a CFI-enabled kernel. > > Why? This seems to be brain dead. > > Why can't CLANG cope with directly calling e.g. > cpu_tlb.flush_user_range? Why does it need a static function to do > exactly the same as the macro does? > I had the same question, so I played around a bit with the code. What I think would be better is if we could add the __nocfi annotation to the type, i.e., --- a/arch/arm/include/asm/tlbflush.h +++ b/arch/arm/include/asm/tlbflush.h @@ -205,8 +205,8 @@ #include <linux/sched.h> struct cpu_tlb_fns { - void (*flush_user_range)(unsigned long, unsigned long, ...); - void (*flush_kern_range)(unsigned long, unsigned long); + void (__nocfi *flush_user_range)(unsigned long, unsigned long, ...); + void (__nocfi *flush_kern_range)(unsigned long, unsigned long); unsigned long tlb_flags; }; This works for some function attributes (e.g., __efiapi is used like this), but the attribute specifier to which __nocfi resolves does not appear to be usable in the same manner. Best would be to annotate the asm code using SYM_TYPED_FUNC_START/_END, so that the CFI machinery is invoked at the call site to validate the function type of the destination.
On Mon, Mar 11, 2024 at 3:04 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > On Mon, 11 Mar 2024 at 10:39, Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > > > On Mon, Mar 11, 2024 at 10:15:39AM +0100, Linus Walleij wrote: > > > Instead of just using defines to define the TLB flush functions, > > > use static inlines. > > > > > > This has the upside that we can tag those as __nocfi so we can > > > execute a CFI-enabled kernel. > > > > Why? This seems to be brain dead. > > > > Why can't CLANG cope with directly calling e.g. > > cpu_tlb.flush_user_range? Why does it need a static function to do > > exactly the same as the macro does? > > > > I had the same question, so I played around a bit with the code. > > What I think would be better is if we could add the __nocfi annotation > to the type, i.e., > > --- a/arch/arm/include/asm/tlbflush.h > +++ b/arch/arm/include/asm/tlbflush.h > @@ -205,8 +205,8 @@ > #include <linux/sched.h> > > struct cpu_tlb_fns { > - void (*flush_user_range)(unsigned long, unsigned long, ...); > - void (*flush_kern_range)(unsigned long, unsigned long); > + void (__nocfi *flush_user_range)(unsigned long, unsigned long, ...); > + void (__nocfi *flush_kern_range)(unsigned long, unsigned long); > unsigned long tlb_flags; > }; > > This works for some function attributes (e.g., __efiapi is used like > this), but the attribute specifier to which __nocfi resolves does not > appear to be usable in the same manner. > > Best would be to annotate the asm code using > SYM_TYPED_FUNC_START/_END, so that the CFI machinery is invoked at the > call site to validate the function type of the destination. Agreed, ideally we would annotate indirectly called assembly functions with CFI types and avoid __nocfi wrappers. Sami
On Mon, Mar 11, 2024 at 4:35 PM Sami Tolvanen <samitolvanen@google.com> wrote: > On Mon, Mar 11, 2024 at 3:04 AM Ard Biesheuvel <ardb@kernel.org> wrote: >> > > This works for some function attributes (e.g., __efiapi is used like > > this), but the attribute specifier to which __nocfi resolves does not > > appear to be usable in the same manner. > > > > Best would be to annotate the asm code using > > SYM_TYPED_FUNC_START/_END, so that the CFI machinery is invoked at the > > call site to validate the function type of the destination. > > Agreed, ideally we would annotate indirectly called assembly functions > with CFI types and avoid __nocfi wrappers. I'm taking a stab at SYM_TYPED_FUNC_* for ARM, as we don't have them yet. Yours, Linus Walleij
On Mon, Mar 11, 2024 at 7:51 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Mon, Mar 11, 2024 at 4:35 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > On Mon, Mar 11, 2024 at 3:04 AM Ard Biesheuvel <ardb@kernel.org> wrote: > >> > > > This works for some function attributes (e.g., __efiapi is used like > > > this), but the attribute specifier to which __nocfi resolves does not > > > appear to be usable in the same manner. > > > > > > Best would be to annotate the asm code using > > > SYM_TYPED_FUNC_START/_END, so that the CFI machinery is invoked at the > > > call site to validate the function type of the destination. > > > > Agreed, ideally we would annotate indirectly called assembly functions > > with CFI types and avoid __nocfi wrappers. > > I'm taking a stab at SYM_TYPED_FUNC_* for ARM, as we don't have them > yet. Does the default implementation in include/linux/cfi_types.h not work on arm for some reason? Sami
On Mon, Mar 11, 2024 at 10:37 PM Sami Tolvanen <samitolvanen@google.com> wrote: > On Mon, Mar 11, 2024 at 7:51 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Mon, Mar 11, 2024 at 4:35 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > > On Mon, Mar 11, 2024 at 3:04 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > >> > > > > This works for some function attributes (e.g., __efiapi is used like > > > > this), but the attribute specifier to which __nocfi resolves does not > > > > appear to be usable in the same manner. > > > > > > > > Best would be to annotate the asm code using > > > > SYM_TYPED_FUNC_START/_END, so that the CFI machinery is invoked at the > > > > call site to validate the function type of the destination. > > > > > > Agreed, ideally we would annotate indirectly called assembly functions > > > with CFI types and avoid __nocfi wrappers. > > > > I'm taking a stab at SYM_TYPED_FUNC_* for ARM, as we don't have them > > yet. > > Does the default implementation in include/linux/cfi_types.h not work > on arm for some reason? For example I try to switch over the TLB symbols like this: diff --git a/arch/arm/mm/proc-macros.S b/arch/arm/mm/proc-macros.S index e43f6d716b4b..bbe47ca32e55 100644 --- a/arch/arm/mm/proc-macros.S +++ b/arch/arm/mm/proc-macros.S @@ -341,7 +341,7 @@ ENTRY(\name\()_cache_fns) .macro define_tlb_functions name:req, flags_up:req, flags_smp .type \name\()_tlb_fns, #object .align 2 -ENTRY(\name\()_tlb_fns) +SYM_TYPED_FUNC_START(\name\()_tlb_fns) .long \name\()_flush_user_tlb_range .long \name\()_flush_kern_tlb_range .ifnb \flags_smp diff --git a/arch/arm/mm/tlb-v7.S b/arch/arm/mm/tlb-v7.S index 35fd6d4f0d03..aff9d884c30d 100644 --- a/arch/arm/mm/tlb-v7.S +++ b/arch/arm/mm/tlb-v7.S @@ -10,6 +10,7 @@ */ #include <linux/init.h> #include <linux/linkage.h> +#include <linux/cfi_types.h> #include <asm/assembler.h> #include <asm/asm-offsets.h> #include <asm/page.h> @@ -31,7 +32,7 @@ * - the "Invalidate single entry" instruction will invalidate * both the I and the D TLBs on Harvard-style TLBs */ -ENTRY(v7wbi_flush_user_tlb_range) +SYM_TYPED_FUNC_START(v7wbi_flush_user_tlb_range) vma_vm_mm r3, r2 @ get vma->vm_mm mmid r3, r3 @ get vm_mm->context.id dsb ish @@ -57,7 +58,7 @@ ENTRY(v7wbi_flush_user_tlb_range) blo 1b dsb ish ret lr -ENDPROC(v7wbi_flush_user_tlb_range) +SYM_FUNC_END(v7wbi_flush_user_tlb_range) /* * v7wbi_flush_kern_tlb_range(start,end) @@ -67,7 +68,7 @@ ENDPROC(v7wbi_flush_user_tlb_range) * - start - start address (may not be aligned) * - end - end address (exclusive, may not be aligned) */ -ENTRY(v7wbi_flush_kern_tlb_range) +SYM_TYPED_FUNC_START(v7wbi_flush_kern_tlb_range) dsb ish mov r0, r0, lsr #PAGE_SHIFT @ align address mov r1, r1, lsr #PAGE_SHIFT @@ -86,7 +87,7 @@ ENTRY(v7wbi_flush_kern_tlb_range) dsb ish isb ret lr -ENDPROC(v7wbi_flush_kern_tlb_range) +SYM_FUNC_END(v7wbi_flush_kern_tlb_range) __INIT Compiling results in: AR vmlinux.a LD vmlinux.o OBJCOPY modules.builtin.modinfo GEN modules.builtin MODPOST vmlinux.symvers UPD include/generated/utsversion.h CC init/version-timestamp.o LD .tmp_vmlinux.kallsyms1 ld.lld: error: undefined symbol: __kcfi_typeid_v7wbi_flush_user_tlb_range >>> referenced by arch/arm/mm/tlb-v7.o:(.text+0x0) in archive vmlinux.a ld.lld: error: undefined symbol: __kcfi_typeid_v7wbi_flush_kern_tlb_range >>> referenced by tlb-v7.S:60 (/mnt/storage/linus/linux-integrator/build-vexpress/../arch/arm/mm/tlb-v7.S:60) >>> arch/arm/mm/tlb-v7.o:(.text+0x40) in archive vmlinux.a ld.lld: error: undefined symbol: __kcfi_typeid_v7wbi_tlb_fns >>> referenced by arch/arm/mm/tlb-v7.o:(.init.text+0x0) in archive vmlinux.a Yours, Linus Walleij
On Mon, Mar 11, 2024 at 3:17 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > LD .tmp_vmlinux.kallsyms1 > ld.lld: error: undefined symbol: __kcfi_typeid_v7wbi_flush_user_tlb_range > >>> referenced by arch/arm/mm/tlb-v7.o:(.text+0x0) in archive vmlinux.a > > ld.lld: error: undefined symbol: __kcfi_typeid_v7wbi_flush_kern_tlb_range > >>> referenced by tlb-v7.S:60 (/mnt/storage/linus/linux-integrator/build-vexpress/../arch/arm/mm/tlb-v7.S:60) > >>> arch/arm/mm/tlb-v7.o:(.text+0x40) in archive vmlinux.a > > ld.lld: error: undefined symbol: __kcfi_typeid_v7wbi_tlb_fns > >>> referenced by arch/arm/mm/tlb-v7.o:(.init.text+0x0) in archive vmlinux.a Clang only emits __kcfi_typeid symbols for functions that are address-taken in C code. You need to add __ADDRESSABLE(function) references to a C file somewhere for functions that otherwise are not address-taken. Sami
On Mon, Mar 11, 2024 at 11:28 PM Sami Tolvanen <samitolvanen@google.com> wrote: > On Mon, Mar 11, 2024 at 3:17 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > LD .tmp_vmlinux.kallsyms1 > > ld.lld: error: undefined symbol: __kcfi_typeid_v7wbi_flush_user_tlb_range > > >>> referenced by arch/arm/mm/tlb-v7.o:(.text+0x0) in archive vmlinux.a > > > > ld.lld: error: undefined symbol: __kcfi_typeid_v7wbi_flush_kern_tlb_range > > >>> referenced by tlb-v7.S:60 (/mnt/storage/linus/linux-integrator/build-vexpress/../arch/arm/mm/tlb-v7.S:60) > > >>> arch/arm/mm/tlb-v7.o:(.text+0x40) in archive vmlinux.a > > > > ld.lld: error: undefined symbol: __kcfi_typeid_v7wbi_tlb_fns > > >>> referenced by arch/arm/mm/tlb-v7.o:(.init.text+0x0) in archive vmlinux.a > > Clang only emits __kcfi_typeid symbols for functions that are > address-taken in C code. You need to add __ADDRESSABLE(function) > references to a C file somewhere for functions that otherwise are not > address-taken. Hey it works. So for example if for these functions I also add: diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c index d19d140a10c7..23eb0f9358cb 100644 --- a/arch/arm/mm/flush.c +++ b/arch/arm/mm/flush.c @@ -18,6 +18,11 @@ #include "mm.h" +void v7wbi_flush_user_tlb_range(unsigned long, unsigned long, struct vm_area_struct *); +void v7wbi_flush_kern_tlb_range(unsigned long, unsigned long); +__ADDRESSABLE(v7wbi_flush_user_tlb_range); +__ADDRESSABLE(v7wbi_flush_kern_tlb_range); Then that works. The problem is that I also have to define all these function signatures that are never used in C and there are quite a few of them, if I start listing them all and #ifdefining them for selected CPUs it's not going to be pretty. It can be done and they can be in a cfi-defs.c file though. And it's better than __nocfi. The complexity comes from the fact that arm can boot a kernel with support for several different CPU:s. The different CPU management functions are put in a list of supported processors by the linker, and then e.g. the tlb maintenance functions are dereferenced directly from *list->tlb in setup_processor() in arch/arm/kernel/setup.c. Yours, Linus Walleij
On Tue, 12 Mar 2024 at 00:56, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Mon, Mar 11, 2024 at 11:28 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > On Mon, Mar 11, 2024 at 3:17 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > > > LD .tmp_vmlinux.kallsyms1 > > > ld.lld: error: undefined symbol: __kcfi_typeid_v7wbi_flush_user_tlb_range > > > >>> referenced by arch/arm/mm/tlb-v7.o:(.text+0x0) in archive vmlinux.a > > > > > > ld.lld: error: undefined symbol: __kcfi_typeid_v7wbi_flush_kern_tlb_range > > > >>> referenced by tlb-v7.S:60 (/mnt/storage/linus/linux-integrator/build-vexpress/../arch/arm/mm/tlb-v7.S:60) > > > >>> arch/arm/mm/tlb-v7.o:(.text+0x40) in archive vmlinux.a > > > > > > ld.lld: error: undefined symbol: __kcfi_typeid_v7wbi_tlb_fns > > > >>> referenced by arch/arm/mm/tlb-v7.o:(.init.text+0x0) in archive vmlinux.a > > > > Clang only emits __kcfi_typeid symbols for functions that are > > address-taken in C code. You need to add __ADDRESSABLE(function) > > references to a C file somewhere for functions that otherwise are not > > address-taken. > > Hey it works. So for example if for these functions I also add: > > diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c > index d19d140a10c7..23eb0f9358cb 100644 > --- a/arch/arm/mm/flush.c > +++ b/arch/arm/mm/flush.c > @@ -18,6 +18,11 @@ > > #include "mm.h" > > +void v7wbi_flush_user_tlb_range(unsigned long, unsigned long, struct > vm_area_struct *); > +void v7wbi_flush_kern_tlb_range(unsigned long, unsigned long); > +__ADDRESSABLE(v7wbi_flush_user_tlb_range); > +__ADDRESSABLE(v7wbi_flush_kern_tlb_range); > > Then that works. > > The problem is that I also have to define all these function signatures that > are never used in C and there are quite a few of them, if I start listing them > all and #ifdefining them for selected CPUs it's not going to be pretty. > > It can be done and they can be in a cfi-defs.c file though. > And it's better than __nocfi. > > The complexity comes from the fact that arm can boot a kernel > with support for several different CPU:s. > > The different CPU management functions are put in a list of supported > processors by the linker, and then e.g. the tlb maintenance functions > are dereferenced directly from *list->tlb in setup_processor() > in arch/arm/kernel/setup.c. > Another option is to move the struct definitions to C entirely. For example, the branch below implements this for the tlbflush code: https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=arm-cfi However, doing the same will be tricky for the proc_info structs, as they have a member that contains a place-relative offset, and those cannot be easily emitted in C (similar to the SMP_ON_UP hack in the TLB code above).
On Tue, Mar 12, 2024 at 8:25 AM Ard Biesheuvel <ardb@kernel.org> wrote: > On Tue, 12 Mar 2024 at 00:56, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Mon, Mar 11, 2024 at 11:28 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > > On Mon, Mar 11, 2024 at 3:17 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > +void v7wbi_flush_user_tlb_range(unsigned long, unsigned long, struct > > vm_area_struct *); > > +void v7wbi_flush_kern_tlb_range(unsigned long, unsigned long); > > +__ADDRESSABLE(v7wbi_flush_user_tlb_range); > > +__ADDRESSABLE(v7wbi_flush_kern_tlb_range); > > > > Then that works. > > > > The problem is that I also have to define all these function signatures that > > are never used in C and there are quite a few of them, if I start listing them > > all and #ifdefining them for selected CPUs it's not going to be pretty. > > > > It can be done and they can be in a cfi-defs.c file though. > > And it's better than __nocfi. > > Another option is to move the struct definitions to C entirely. For > example, the branch below implements this for the tlbflush code: > > https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=arm-cfi Hm that looks very nice IMO, I think we could go for that for the tlb functions for sure, I will just steal your patch :D The same can probably be done for some other vtables. > However, doing the same will be tricky for the proc_info structs, as > they have a member that contains a place-relative offset, and those > cannot be easily emitted in C (similar to the SMP_ON_UP hack in the > TLB code above). Hm hm. I might have to do the first approach and list all functions in a file for those, it will probably be the lesser evil. I'll take a stab at it and see where we end up. Yours, Linus Walleij
diff --git a/arch/arm/include/asm/tlbflush.h b/arch/arm/include/asm/tlbflush.h index 38c6e4a2a0b6..7340518ee0e9 100644 --- a/arch/arm/include/asm/tlbflush.h +++ b/arch/arm/include/asm/tlbflush.h @@ -210,13 +210,23 @@ struct cpu_tlb_fns { unsigned long tlb_flags; }; +extern struct cpu_tlb_fns cpu_tlb; + +#define __cpu_tlb_flags cpu_tlb.tlb_flags + /* * Select the calling method */ #ifdef MULTI_TLB -#define __cpu_flush_user_tlb_range cpu_tlb.flush_user_range -#define __cpu_flush_kern_tlb_range cpu_tlb.flush_kern_range +static inline void __nocfi __cpu_flush_user_tlb_range(unsigned long s, unsigned long e, struct vm_area_struct *vma) +{ + cpu_tlb.flush_user_range(s, e, vma); +} +static inline void __nocfi __cpu_flush_kern_tlb_range(unsigned long s, unsigned long e) +{ + cpu_tlb.flush_kern_range(s, e); +} #else @@ -228,10 +238,6 @@ extern void __cpu_flush_kern_tlb_range(unsigned long, unsigned long); #endif -extern struct cpu_tlb_fns cpu_tlb; - -#define __cpu_tlb_flags cpu_tlb.tlb_flags - /* * TLB Management * ==============
Instead of just using defines to define the TLB flush functions, use static inlines. This has the upside that we can tag those as __nocfi so we can execute a CFI-enabled kernel. Move the variables around a bit so the functions can find their global variable cpu_tlb. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- arch/arm/include/asm/tlbflush.h | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)