| Message ID | 20210930180531.1190642-5-samitolvanen@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: Add support for Clang CFI | expand |
On Thu, Sep 30, 2021 at 11:05 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > This change introduces the DEFINE_CFI_IMMEDIATE_RETURN_STUB macro, > which defines a stub function that immediately returns and when > defined in the core kernel, always passes indirect call checking > with CONFIG_CFI_CLANG. Note that this macro should only be used when > a stub cannot be called using the correct function type. Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Seems like the only use is in patch 5/15. Probably could be squashed... > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > --- > include/asm-generic/vmlinux.lds.h | 11 +++++++++++ > include/linux/cfi.h | 13 +++++++++++++ > kernel/cfi.c | 24 +++++++++++++++++++++++- > 3 files changed, 47 insertions(+), 1 deletion(-) > > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h > index f2984af2b85b..5b77284f7221 100644 > --- a/include/asm-generic/vmlinux.lds.h > +++ b/include/asm-generic/vmlinux.lds.h > @@ -407,6 +407,16 @@ > KEEP(*(.static_call_tramp_key)) \ > __stop_static_call_tramp_key = .; > > +#ifdef CONFIG_CFI_CLANG > +#define CFI_EXCLUDED_DATA \ > + . = ALIGN(8); \ > + __start_cfi_excluded = .; \ > + KEEP(*(.cfi_excluded_stubs)) \ > + __stop_cfi_excluded = .; > +#else > +#define CFI_EXCLUDED_DATA > +#endif > + > /* > * Allow architectures to handle ro_after_init data on their > * own by defining an empty RO_AFTER_INIT_DATA. > @@ -430,6 +440,7 @@ > __start_rodata = .; \ > *(.rodata) *(.rodata.*) \ > SCHED_DATA \ > + CFI_EXCLUDED_DATA \ > RO_AFTER_INIT_DATA /* Read only after init */ \ > . = ALIGN(8); \ > __start___tracepoints_ptrs = .; \ > diff --git a/include/linux/cfi.h b/include/linux/cfi.h > index 879744aaa6e0..19f74af8eac2 100644 > --- a/include/linux/cfi.h > +++ b/include/linux/cfi.h > @@ -20,6 +20,17 @@ extern void __cfi_check(uint64_t id, void *ptr, void *diag); > #define __CFI_ADDRESSABLE(fn, __attr) \ > const void *__cfi_jt_ ## fn __visible __attr = (void *)&fn > > +/* > + * Defines a stub function that returns immediately, and when defined and > + * referenced in the core kernel, always passes CFI checking. This should > + * be used only for stubs that cannot be called using the correct function > + * pointer type, which should be rare. > + */ > +#define DEFINE_CFI_IMMEDIATE_RETURN_STUB(fn) \ > + void fn(void) { return; } \ > + const void *__cfi_excl_ ## fn __visible \ > + __section(".cfi_excluded_stubs") = (void *)&fn > + > #ifdef CONFIG_CFI_CLANG_SHADOW > > extern void cfi_module_add(struct module *mod, unsigned long base_addr); > @@ -35,6 +46,8 @@ static inline void cfi_module_remove(struct module *mod, unsigned long base_addr > #else /* !CONFIG_CFI_CLANG */ > > #define __CFI_ADDRESSABLE(fn, __attr) > +#define DEFINE_CFI_IMMEDIATE_RETURN_STUB(fn) \ > + void fn(void) { return; } > > #endif /* CONFIG_CFI_CLANG */ > > diff --git a/kernel/cfi.c b/kernel/cfi.c > index 9594cfd1cf2c..8d931089141b 100644 > --- a/kernel/cfi.c > +++ b/kernel/cfi.c > @@ -278,12 +278,34 @@ static inline cfi_check_fn find_module_check_fn(unsigned long ptr) > return fn; > } > > +extern unsigned long __start_cfi_excluded[]; > +extern unsigned long __stop_cfi_excluded[]; > + > +static inline bool is_cfi_excluded(unsigned long ptr) > +{ > + unsigned long *p = __start_cfi_excluded; > + > + for ( ; p < __stop_cfi_excluded; ++p) > + if (*p == ptr) > + return true; > + > + return false; > +} > + > +static void __cfi_pass(uint64_t id, void *ptr, void *diag) > +{ > +} > + > static inline cfi_check_fn find_check_fn(unsigned long ptr) > { > cfi_check_fn fn = NULL; > > - if (is_kernel_text(ptr)) > + if (is_kernel_text(ptr)) { > + if (unlikely(is_cfi_excluded(ptr))) > + return __cfi_pass; > + > return __cfi_check; > + } > > /* > * Indirect call checks can happen when RCU is not watching. Both > -- > 2.33.0.800.g4c38ced690-goog >
On Thu, Sep 30, 2021 at 11:50 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Thu, Sep 30, 2021 at 11:05 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > This change introduces the DEFINE_CFI_IMMEDIATE_RETURN_STUB macro, > > which defines a stub function that immediately returns and when > > defined in the core kernel, always passes indirect call checking > > with CONFIG_CFI_CLANG. Note that this macro should only be used when > > a stub cannot be called using the correct function type. > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > Seems like the only use is in patch 5/15. Probably could be squashed... I would prefer to keep these separate, but if everyone thinks that's unnecessary, I'm happy to combine them. Sami
On Thu, Sep 30, 2021 at 11:05:20AM -0700, Sami Tolvanen wrote: > This change introduces the DEFINE_CFI_IMMEDIATE_RETURN_STUB macro, > which defines a stub function that immediately returns and when > defined in the core kernel, always passes indirect call checking > with CONFIG_CFI_CLANG. Note that this macro should only be used when > a stub cannot be called using the correct function type. > diff --git a/include/linux/cfi.h b/include/linux/cfi.h > index 879744aaa6e0..19f74af8eac2 100644 > --- a/include/linux/cfi.h > +++ b/include/linux/cfi.h > @@ -20,6 +20,17 @@ extern void __cfi_check(uint64_t id, void *ptr, void *diag); > #define __CFI_ADDRESSABLE(fn, __attr) \ > const void *__cfi_jt_ ## fn __visible __attr = (void *)&fn > > +/* > + * Defines a stub function that returns immediately, and when defined and > + * referenced in the core kernel, always passes CFI checking. This should > + * be used only for stubs that cannot be called using the correct function > + * pointer type, which should be rare. > + */ > +#define DEFINE_CFI_IMMEDIATE_RETURN_STUB(fn) \ > + void fn(void) { return; } \ > + const void *__cfi_excl_ ## fn __visible \ > + __section(".cfi_excluded_stubs") = (void *)&fn > + > #ifdef CONFIG_CFI_CLANG_SHADOW > > extern void cfi_module_add(struct module *mod, unsigned long base_addr); > @@ -35,6 +46,8 @@ static inline void cfi_module_remove(struct module *mod, unsigned long base_addr > #else /* !CONFIG_CFI_CLANG */ > > #define __CFI_ADDRESSABLE(fn, __attr) > +#define DEFINE_CFI_IMMEDIATE_RETURN_STUB(fn) \ > + void fn(void) { return; } > > #endif /* CONFIG_CFI_CLANG */ > Why DEFINE_CFI_IMMEDIATE_RETURN_STUB() vs __no_cfi attribute that we can stick on the relvant functions? Because I've got at least one more variant for you :-) See kernel/static_call.c:__static_call_return0
On Mon, Oct 4, 2021 at 6:50 AM Peter Zijlstra <peterz@infradead.org> wrote: > > On Thu, Sep 30, 2021 at 11:05:20AM -0700, Sami Tolvanen wrote: > > This change introduces the DEFINE_CFI_IMMEDIATE_RETURN_STUB macro, > > which defines a stub function that immediately returns and when > > defined in the core kernel, always passes indirect call checking > > with CONFIG_CFI_CLANG. Note that this macro should only be used when > > a stub cannot be called using the correct function type. > > > diff --git a/include/linux/cfi.h b/include/linux/cfi.h > > index 879744aaa6e0..19f74af8eac2 100644 > > --- a/include/linux/cfi.h > > +++ b/include/linux/cfi.h > > @@ -20,6 +20,17 @@ extern void __cfi_check(uint64_t id, void *ptr, void *diag); > > #define __CFI_ADDRESSABLE(fn, __attr) \ > > const void *__cfi_jt_ ## fn __visible __attr = (void *)&fn > > > > +/* > > + * Defines a stub function that returns immediately, and when defined and > > + * referenced in the core kernel, always passes CFI checking. This should > > + * be used only for stubs that cannot be called using the correct function > > + * pointer type, which should be rare. > > + */ > > +#define DEFINE_CFI_IMMEDIATE_RETURN_STUB(fn) \ > > + void fn(void) { return; } \ > > + const void *__cfi_excl_ ## fn __visible \ > > + __section(".cfi_excluded_stubs") = (void *)&fn > > + > > #ifdef CONFIG_CFI_CLANG_SHADOW > > > > extern void cfi_module_add(struct module *mod, unsigned long base_addr); > > @@ -35,6 +46,8 @@ static inline void cfi_module_remove(struct module *mod, unsigned long base_addr > > #else /* !CONFIG_CFI_CLANG */ > > > > #define __CFI_ADDRESSABLE(fn, __attr) > > +#define DEFINE_CFI_IMMEDIATE_RETURN_STUB(fn) \ > > + void fn(void) { return; } > > > > #endif /* CONFIG_CFI_CLANG */ > > > > Why DEFINE_CFI_IMMEDIATE_RETURN_STUB() vs __no_cfi attribute that we can > stick on the relvant functions? To avoid accidentally creating useful gadgets for attackers. For example, while excluding an empty stub isn't necessarily ideal, allowing calls to a function that always returns zero would be worse. > Because I've got at least one more variant for you :-) See > kernel/static_call.c:__static_call_return0 Does __static_call_return0 ever get called indirectly on architectures that support static calls? If it's always patched into a direct call, the type mismatch isn't an issue. Sami
On Mon, Oct 04, 2021 at 12:10:46PM -0700, Sami Tolvanen wrote: > On Mon, Oct 4, 2021 at 6:50 AM Peter Zijlstra <peterz@infradead.org> wrote: > > Why DEFINE_CFI_IMMEDIATE_RETURN_STUB() vs __no_cfi attribute that we can > > stick on the relvant functions? > > To avoid accidentally creating useful gadgets for attackers. For > example, while excluding an empty stub isn't necessarily ideal, > allowing calls to a function that always returns zero would be worse. I was afraid you'd say something like that... > > Because I've got at least one more variant for you :-) See > > kernel/static_call.c:__static_call_return0 > > Does __static_call_return0 ever get called indirectly on architectures > that support static calls? If it's always patched into a direct call, > the type mismatch isn't an issue. For x86_64 it should indeed never get called, however if you plan on supporting i386 then you need the annotation. Also, it might get called on arm64 which is about to grow basic HAVE_STATIC_CALL support. (and just in case you care about CFI on PPC32, they too grew basic static_call support)
On Mon, Oct 4, 2021 at 11:59 PM Peter Zijlstra <peterz@infradead.org> wrote: > > On Mon, Oct 04, 2021 at 12:10:46PM -0700, Sami Tolvanen wrote: > > On Mon, Oct 4, 2021 at 6:50 AM Peter Zijlstra <peterz@infradead.org> wrote: > > > > Why DEFINE_CFI_IMMEDIATE_RETURN_STUB() vs __no_cfi attribute that we can > > > stick on the relvant functions? > > > > To avoid accidentally creating useful gadgets for attackers. For > > example, while excluding an empty stub isn't necessarily ideal, > > allowing calls to a function that always returns zero would be worse. > > I was afraid you'd say something like that... > > > > Because I've got at least one more variant for you :-) See > > > kernel/static_call.c:__static_call_return0 > > > > Does __static_call_return0 ever get called indirectly on architectures > > that support static calls? If it's always patched into a direct call, > > the type mismatch isn't an issue. > > For x86_64 it should indeed never get called, however if you plan on > supporting i386 then you need the annotation. Also, it might get called > on arm64 which is about to grow basic HAVE_STATIC_CALL support. Good point. I read through the latest arm64 static call proposal and while it can fall back to an indirect call, it doesn't look like that would cause issues with CFI. > (and just in case you care about CFI on PPC32, they too grew basic > static_call support) We are currently targeting only x86_64 and arm64, but I'll keep that in mind in case we want to add more platforms. Sami
On Tue, Oct 05, 2021 at 01:29:02PM -0700, Sami Tolvanen wrote: > On Mon, Oct 4, 2021 at 11:59 PM Peter Zijlstra <peterz@infradead.org> wrote: > > For x86_64 it should indeed never get called, however if you plan on > > supporting i386 then you need the annotation. Also, it might get called > > on arm64 which is about to grow basic HAVE_STATIC_CALL support. > > Good point. I read through the latest arm64 static call proposal and > while it can fall back to an indirect call, it doesn't look like that > would cause issues with CFI. Because that call is outside of compiler control? Same will be true for any HAVE_STATIC_CALL implementation I suppose. The trampoline will be outside of compiler control.
On Tue, Oct 5, 2021 at 1:58 PM Peter Zijlstra <peterz@infradead.org> wrote: > > On Tue, Oct 05, 2021 at 01:29:02PM -0700, Sami Tolvanen wrote: > > On Mon, Oct 4, 2021 at 11:59 PM Peter Zijlstra <peterz@infradead.org> wrote: > > > > For x86_64 it should indeed never get called, however if you plan on > > > supporting i386 then you need the annotation. Also, it might get called > > > on arm64 which is about to grow basic HAVE_STATIC_CALL support. > > > > Good point. I read through the latest arm64 static call proposal and > > while it can fall back to an indirect call, it doesn't look like that > > would cause issues with CFI. > > Because that call is outside of compiler control? Correct. > Same will be true for > any HAVE_STATIC_CALL implementation I suppose. The trampoline will be > outside of compiler control. True, so it shouldn't be a problem. Sami
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index f2984af2b85b..5b77284f7221 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -407,6 +407,16 @@ KEEP(*(.static_call_tramp_key)) \ __stop_static_call_tramp_key = .; +#ifdef CONFIG_CFI_CLANG +#define CFI_EXCLUDED_DATA \ + . = ALIGN(8); \ + __start_cfi_excluded = .; \ + KEEP(*(.cfi_excluded_stubs)) \ + __stop_cfi_excluded = .; +#else +#define CFI_EXCLUDED_DATA +#endif + /* * Allow architectures to handle ro_after_init data on their * own by defining an empty RO_AFTER_INIT_DATA. @@ -430,6 +440,7 @@ __start_rodata = .; \ *(.rodata) *(.rodata.*) \ SCHED_DATA \ + CFI_EXCLUDED_DATA \ RO_AFTER_INIT_DATA /* Read only after init */ \ . = ALIGN(8); \ __start___tracepoints_ptrs = .; \ diff --git a/include/linux/cfi.h b/include/linux/cfi.h index 879744aaa6e0..19f74af8eac2 100644 --- a/include/linux/cfi.h +++ b/include/linux/cfi.h @@ -20,6 +20,17 @@ extern void __cfi_check(uint64_t id, void *ptr, void *diag); #define __CFI_ADDRESSABLE(fn, __attr) \ const void *__cfi_jt_ ## fn __visible __attr = (void *)&fn +/* + * Defines a stub function that returns immediately, and when defined and + * referenced in the core kernel, always passes CFI checking. This should + * be used only for stubs that cannot be called using the correct function + * pointer type, which should be rare. + */ +#define DEFINE_CFI_IMMEDIATE_RETURN_STUB(fn) \ + void fn(void) { return; } \ + const void *__cfi_excl_ ## fn __visible \ + __section(".cfi_excluded_stubs") = (void *)&fn + #ifdef CONFIG_CFI_CLANG_SHADOW extern void cfi_module_add(struct module *mod, unsigned long base_addr); @@ -35,6 +46,8 @@ static inline void cfi_module_remove(struct module *mod, unsigned long base_addr #else /* !CONFIG_CFI_CLANG */ #define __CFI_ADDRESSABLE(fn, __attr) +#define DEFINE_CFI_IMMEDIATE_RETURN_STUB(fn) \ + void fn(void) { return; } #endif /* CONFIG_CFI_CLANG */ diff --git a/kernel/cfi.c b/kernel/cfi.c index 9594cfd1cf2c..8d931089141b 100644 --- a/kernel/cfi.c +++ b/kernel/cfi.c @@ -278,12 +278,34 @@ static inline cfi_check_fn find_module_check_fn(unsigned long ptr) return fn; } +extern unsigned long __start_cfi_excluded[]; +extern unsigned long __stop_cfi_excluded[]; + +static inline bool is_cfi_excluded(unsigned long ptr) +{ + unsigned long *p = __start_cfi_excluded; + + for ( ; p < __stop_cfi_excluded; ++p) + if (*p == ptr) + return true; + + return false; +} + +static void __cfi_pass(uint64_t id, void *ptr, void *diag) +{ +} + static inline cfi_check_fn find_check_fn(unsigned long ptr) { cfi_check_fn fn = NULL; - if (is_kernel_text(ptr)) + if (is_kernel_text(ptr)) { + if (unlikely(is_cfi_excluded(ptr))) + return __cfi_pass; + return __cfi_check; + } /* * Indirect call checks can happen when RCU is not watching. Both
This change introduces the DEFINE_CFI_IMMEDIATE_RETURN_STUB macro, which defines a stub function that immediately returns and when defined in the core kernel, always passes indirect call checking with CONFIG_CFI_CLANG. Note that this macro should only be used when a stub cannot be called using the correct function type. Signed-off-by: Sami Tolvanen <samitolvanen@google.com> --- include/asm-generic/vmlinux.lds.h | 11 +++++++++++ include/linux/cfi.h | 13 +++++++++++++ kernel/cfi.c | 24 +++++++++++++++++++++++- 3 files changed, 47 insertions(+), 1 deletion(-)