| Message ID | 20210401233216.2540591-15-samitolvanen@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for Clang CFI | expand |
[adding Ard for EFI runtime services bits] On Thu, Apr 01, 2021 at 04:32:12PM -0700, Sami Tolvanen wrote: > Disable CFI checking for functions that switch to linear mapping and > make an indirect call to a physical address, since the compiler only > understands virtual addresses and the CFI check for such indirect calls > would always fail. What does physical vs virtual have to do with this? Does the address actually matter, or is this just a general thing that when calling an assembly function we won't have a trampoline that the caller expects? I wonder if we need to do something with asmlinkage here, perhaps? I didn't spot anything in the seriues handling EFI runtime services calls, and I strongly suspect we need to do something for those, unless they're handled implicitly by something else. > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > Reviewed-by: Kees Cook <keescook@chromium.org> > --- > arch/arm64/include/asm/mmu_context.h | 2 +- > arch/arm64/kernel/cpu-reset.h | 8 ++++---- > arch/arm64/kernel/cpufeature.c | 2 +- > 3 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h > index 386b96400a57..d3cef9133539 100644 > --- a/arch/arm64/include/asm/mmu_context.h > +++ b/arch/arm64/include/asm/mmu_context.h > @@ -119,7 +119,7 @@ static inline void cpu_install_idmap(void) > * Atomically replaces the active TTBR1_EL1 PGD with a new VA-compatible PGD, > * avoiding the possibility of conflicting TLB entries being allocated. > */ > -static inline void cpu_replace_ttbr1(pgd_t *pgdp) > +static inline void __nocfi cpu_replace_ttbr1(pgd_t *pgdp) Given these are inlines, what's the effect when these are inlined into a function that would normally use CFI? Does CFI get supressed for the whole function, or just the bit that got inlined? Is there an attribute that we could place on a function pointer to tell the compiler to not check calls via that pointer? If that existed we'd be able to scope this much more tightly. Thanks, Mark. > { > typedef void (ttbr_replace_func)(phys_addr_t); > extern ttbr_replace_func idmap_cpu_replace_ttbr1; > diff --git a/arch/arm64/kernel/cpu-reset.h b/arch/arm64/kernel/cpu-reset.h > index f3adc574f969..9a7b1262ef17 100644 > --- a/arch/arm64/kernel/cpu-reset.h > +++ b/arch/arm64/kernel/cpu-reset.h > @@ -13,10 +13,10 @@ > void __cpu_soft_restart(unsigned long el2_switch, unsigned long entry, > unsigned long arg0, unsigned long arg1, unsigned long arg2); > > -static inline void __noreturn cpu_soft_restart(unsigned long entry, > - unsigned long arg0, > - unsigned long arg1, > - unsigned long arg2) > +static inline void __noreturn __nocfi cpu_soft_restart(unsigned long entry, > + unsigned long arg0, > + unsigned long arg1, > + unsigned long arg2) > { > typeof(__cpu_soft_restart) *restart; > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 0b2e0d7b13ec..c2f94a5206e0 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -1445,7 +1445,7 @@ static bool unmap_kernel_at_el0(const struct arm64_cpu_capabilities *entry, > } > > #ifdef CONFIG_UNMAP_KERNEL_AT_EL0 > -static void > +static void __nocfi > kpti_install_ng_mappings(const struct arm64_cpu_capabilities *__unused) > { > typedef void (kpti_remap_fn)(int, int, phys_addr_t); > -- > 2.31.0.208.g409f899ff0-goog >
On Tue, 6 Apr 2021 at 13:54, Mark Rutland <mark.rutland@arm.com> wrote: > > [adding Ard for EFI runtime services bits] > > On Thu, Apr 01, 2021 at 04:32:12PM -0700, Sami Tolvanen wrote: > > Disable CFI checking for functions that switch to linear mapping and > > make an indirect call to a physical address, since the compiler only > > understands virtual addresses and the CFI check for such indirect calls > > would always fail. > > What does physical vs virtual have to do with this? Does the address > actually matter, or is this just a general thing that when calling an > assembly function we won't have a trampoline that the caller expects? > > I wonder if we need to do something with asmlinkage here, perhaps? > > I didn't spot anything in the seriues handling EFI runtime services > calls, and I strongly suspect we need to do something for those, unless > they're handled implicitly by something else. > All indirect EFI calls are routed via a asm helper that I originally added to check whether x18 was corrupted by the firmware. So from the caller side, we should be fine. All callees are addresses that are provided by the firmware via tables in memory, so I would assume that this addresses the callee side as well. AFAICT, it is never left up to the compiler to emit these indirect calls, or take the address of a firmware routine. But a test would be nice :-) > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > > Reviewed-by: Kees Cook <keescook@chromium.org> > > --- > > arch/arm64/include/asm/mmu_context.h | 2 +- > > arch/arm64/kernel/cpu-reset.h | 8 ++++---- > > arch/arm64/kernel/cpufeature.c | 2 +- > > 3 files changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h > > index 386b96400a57..d3cef9133539 100644 > > --- a/arch/arm64/include/asm/mmu_context.h > > +++ b/arch/arm64/include/asm/mmu_context.h > > @@ -119,7 +119,7 @@ static inline void cpu_install_idmap(void) > > * Atomically replaces the active TTBR1_EL1 PGD with a new VA-compatible PGD, > > * avoiding the possibility of conflicting TLB entries being allocated. > > */ > > -static inline void cpu_replace_ttbr1(pgd_t *pgdp) > > +static inline void __nocfi cpu_replace_ttbr1(pgd_t *pgdp) > > Given these are inlines, what's the effect when these are inlined into a > function that would normally use CFI? Does CFI get supressed for the > whole function, or just the bit that got inlined? > > Is there an attribute that we could place on a function pointer to tell > the compiler to not check calls via that pointer? If that existed we'd > be able to scope this much more tightly. > I agree that it would be very helpful to be able to define a function pointer type that is exempt from CFI checks.
On Tue, Apr 6, 2021 at 4:54 AM Mark Rutland <mark.rutland@arm.com> wrote: > > [adding Ard for EFI runtime services bits] > > On Thu, Apr 01, 2021 at 04:32:12PM -0700, Sami Tolvanen wrote: > > Disable CFI checking for functions that switch to linear mapping and > > make an indirect call to a physical address, since the compiler only > > understands virtual addresses and the CFI check for such indirect calls > > would always fail. > > What does physical vs virtual have to do with this? Does the address > actually matter, or is this just a general thing that when calling an > assembly function we won't have a trampoline that the caller expects? No, this is about the actual address. The compiler-generated runtime checks only know about EL1 virtual addresses, so if we switch to a different address space, all indirect calls will trip CFI. > I wonder if we need to do something with asmlinkage here, perhaps? > > I didn't spot anything in the seriues handling EFI runtime services > calls, and I strongly suspect we need to do something for those, unless > they're handled implicitly by something else. > > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > > Reviewed-by: Kees Cook <keescook@chromium.org> > > --- > > arch/arm64/include/asm/mmu_context.h | 2 +- > > arch/arm64/kernel/cpu-reset.h | 8 ++++---- > > arch/arm64/kernel/cpufeature.c | 2 +- > > 3 files changed, 6 insertions(+), 6 deletions(-) > >https://www.cnbc.com/2021/04/06/donald-trump-save-america-pac-has-85-million-on-hand-ahead-of-midterms.html > > diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h > > index 386b96400a57..d3cef9133539 100644 > > --- a/arch/arm64/include/asm/mmu_context.h > > +++ b/arch/arm64/include/asm/mmu_context.h > > @@ -119,7 +119,7 @@ static inline void cpu_install_idmap(void) > > * Atomically replaces the active TTBR1_EL1 PGD with a new VA-compatible PGD, > > * avoiding the possibility of conflicting TLB entries being allocated. > > */ > > -static inline void cpu_replace_ttbr1(pgd_t *pgdp) > > +static inline void __nocfi cpu_replace_ttbr1(pgd_t *pgdp) > > Given these are inlines, what's the effect when these are inlined into a > function that would normally use CFI? Does CFI get supressed for the > whole function, or just the bit that got inlined? Just for the bit that gets inlined. > Is there an attribute that we could place on a function pointer to tell > the compiler to not check calls via that pointer? If that existed we'd > be able to scope this much more tightly. There isn't, but I do agree that this would be a useful feature. Sami
diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h index 386b96400a57..d3cef9133539 100644 --- a/arch/arm64/include/asm/mmu_context.h +++ b/arch/arm64/include/asm/mmu_context.h @@ -119,7 +119,7 @@ static inline void cpu_install_idmap(void) * Atomically replaces the active TTBR1_EL1 PGD with a new VA-compatible PGD, * avoiding the possibility of conflicting TLB entries being allocated. */ -static inline void cpu_replace_ttbr1(pgd_t *pgdp) +static inline void __nocfi cpu_replace_ttbr1(pgd_t *pgdp) { typedef void (ttbr_replace_func)(phys_addr_t); extern ttbr_replace_func idmap_cpu_replace_ttbr1; diff --git a/arch/arm64/kernel/cpu-reset.h b/arch/arm64/kernel/cpu-reset.h index f3adc574f969..9a7b1262ef17 100644 --- a/arch/arm64/kernel/cpu-reset.h +++ b/arch/arm64/kernel/cpu-reset.h @@ -13,10 +13,10 @@ void __cpu_soft_restart(unsigned long el2_switch, unsigned long entry, unsigned long arg0, unsigned long arg1, unsigned long arg2); -static inline void __noreturn cpu_soft_restart(unsigned long entry, - unsigned long arg0, - unsigned long arg1, - unsigned long arg2) +static inline void __noreturn __nocfi cpu_soft_restart(unsigned long entry, + unsigned long arg0, + unsigned long arg1, + unsigned long arg2) { typeof(__cpu_soft_restart) *restart; diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 0b2e0d7b13ec..c2f94a5206e0 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -1445,7 +1445,7 @@ static bool unmap_kernel_at_el0(const struct arm64_cpu_capabilities *entry, } #ifdef CONFIG_UNMAP_KERNEL_AT_EL0 -static void +static void __nocfi kpti_install_ng_mappings(const struct arm64_cpu_capabilities *__unused) { typedef void (kpti_remap_fn)(int, int, phys_addr_t);