A security research paper was recently published detailing Catch Handler
Oriented Programming (CHOP) attacks.
https://download.vusec.net/papers/chop_ndss23.pdf
The TL;DR being that C++ structured exception handling runtimes are
attractive gadgets for Jump Oriented Programming (JOP) attacks.
In response to this, a mitigation was developed under embargo in
clang-16 to check the stack canary before calling noreturn functions.
https://bugs.chromium.org/p/llvm/issues/detail?id=30
This started causing boot failures in Android kernel trees downstream of
stable linux-4.14.y that had proto-LTO support, as reported by Nathan
Chancellor.
https://github.com/ClangBuiltLinux/linux/issues/1815
Josh Poimboeuf recently sent a series to explicitly annotate more
functions as noreturn. Nathan noticed the series, and tested it finding
that it now caused boot failures with clang-16+ on mainline (raising the
visibility and urgency of the issue).
https://lore.kernel.org/[email protected]/
Once the embargo was lifted, I asked questions such as "what does C++
structured exception handling have to do with C code" and "surely GCC
didn't ship the same mitigation for C code (narrator: 'They did not:
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=a25982ada523689c8745d7fb4b1b93c8f5dab2e7')?"
I now have a patch out for LLVM to undo this mess (or at least limit it
to C++ functions that may throw, similar to GCC's mitigation); it hasn't
landed yet but we're close to consensus and I expect it to land
imminently.
https://reviews.llvm.org/D147975
Remember this thread? (Pepperidge farms remembers...)
https://lore.kernel.org/all/[email protected]/
That reminded me that years ago we discussed a function attribute for
no_stack_protector.
https://lore.kernel.org/all/[email protected]/
GCC didn't have one at the time, it now does. In addition to the LLVM
fix, I'd like to introduce this in the kernel so that we might start
using it in additional places:
* https://lore.kernel.org/linux-pm/[email protected]/
* https://lore.kernel.org/lkml/[email protected]/
And eventually remove the final macro expansion site of
prevent_tail_call_optimization.
With the LLVM fix, this series isn't required, but I'd like to start
paving the way to use these function attributes since I think they are a
sweet spot in terms of granularity (as opposed to trying to move
start_kernel to its own TU compiled with -fno-stack-protector).
(This is my first time using b4 to send a series, as per
https://people.kernel.org/monsieuricon/sending-a-kernel-patch-with-b4-part-1.
Here goes nothing!)
Signed-off-by: Nick Desaulniers <[email protected]>
---
Nick Desaulniers (2):
start_kernel: add no_stack_protector fn attr
start_kernel: omit prevent_tail_call_optimization for newer toolchains
arch/powerpc/kernel/smp.c | 1 +
include/linux/compiler_attributes.h | 12 ++++++++++++
init/main.c | 8 +++++++-
3 files changed, 20 insertions(+), 1 deletion(-)
---
base-commit: 0bcc4025550403ae28d2984bddacafbca0a2f112
change-id: 20230412-no_stackp-a98168a2bb0a
Best regards,
--
Nick Desaulniers <[email protected]>
prevent_tail_call_optimization was added in
commit a9a3ed1eff36 ("x86: Fix early boot crash on gcc-10, third try")
to work around stack canaries getting inserted into functions that would
initialize the stack canary in the first place.
Now that we have no_stack_protector function attribute (gcc-11+,
clang-7+) and use it on start_kernel, remove the call to
prevent_tail_call_optimization such that we may one day remove it
outright.
Signed-off-by: Nick Desaulniers <[email protected]>
---
init/main.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/init/main.c b/init/main.c
index 213baf7b8cb1..c8503d02dfa6 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1152,7 +1152,12 @@ void start_kernel(void)
/* Do the rest non-__init'ed, we're now alive */
arch_call_rest_init();
+ /* Avoid stack canaries in callers of boot_init_stack_canary for gcc-10
+ * and older.
+ */
+#if !__has_attribute(__no_stack_protector__)
prevent_tail_call_optimization();
+#endif
}
/* Call all constructor functions linked into the kernel. */
--
2.40.0.577.gac1e443424-goog
Back during the discussion of
commit a9a3ed1eff36 ("x86: Fix early boot crash on gcc-10, third try")
we discussed the need for a function attribute to control the omission
of stack protectors on a per-function basis; at the time Clang had
support for no_stack_protector but GCC did not. This was fixed in
gcc-11. Now that the function attribute is available, let's start using
it.
Callers of boot_init_stack_canary need to use this function attribute
unless they're compiled with -fno-stack-protector, otherwise the canary
stored in the stack slot of the caller will differ upon the call to
boot_init_stack_canary. This will lead to a call to __stack_chk_fail
then panic.
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94722
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Nick Desaulniers <[email protected]>
---
arch/powerpc/kernel/smp.c | 1 +
include/linux/compiler_attributes.h | 12 ++++++++++++
init/main.c | 3 ++-
3 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 6b90f10a6c81..7d4c12b1abb7 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -1603,6 +1603,7 @@ static void add_cpu_to_masks(int cpu)
}
/* Activate a secondary processor. */
+__no_stack_protector
void start_secondary(void *unused)
{
unsigned int cpu = raw_smp_processor_id();
diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index e659cb6fded3..84864767a56a 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -255,6 +255,18 @@
*/
#define __noreturn __attribute__((__noreturn__))
+/*
+ * Optional: only supported since GCC >= 11.1, clang >= 7.0.
+ *
+ * gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-no_005fstack_005fprotector-function-attribute
+ * clang: https://clang.llvm.org/docs/AttributeReference.html#no-stack-protector-safebuffers
+ */
+#if __has_attribute(__no_stack_protector__)
+# define __no_stack_protector __attribute__((__no_stack_protector__))
+#else
+# define __no_stack_protector
+#endif
+
/*
* Optional: not supported by gcc.
*
diff --git a/init/main.c b/init/main.c
index bb87b789c543..213baf7b8cb1 100644
--- a/init/main.c
+++ b/init/main.c
@@ -941,7 +941,8 @@ static void __init print_unknown_bootoptions(void)
memblock_free(unknown_options, len);
}
-asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
+asmlinkage __visible __init __no_sanitize_address __no_stack_protector
+void start_kernel(void)
{
char *command_line;
char *after_dashes;
--
2.40.0.577.gac1e443424-goog
On Wed, Apr 12, 2023 at 8:32 PM <[email protected]> wrote:
>
> include/linux/compiler_attributes.h | 12 ++++++++++++
Acked-by: Miguel Ojeda <[email protected]>
Cheers,
Miguel
On Wed, Apr 12, 2023 at 11:32:12AM -0700, [email protected] wrote:
> Back during the discussion of
> commit a9a3ed1eff36 ("x86: Fix early boot crash on gcc-10, third try")
> we discussed the need for a function attribute to control the omission
> of stack protectors on a per-function basis; at the time Clang had
> support for no_stack_protector but GCC did not. This was fixed in
> gcc-11. Now that the function attribute is available, let's start using
> it.
>
> Callers of boot_init_stack_canary need to use this function attribute
> unless they're compiled with -fno-stack-protector, otherwise the canary
> stored in the stack slot of the caller will differ upon the call to
> boot_init_stack_canary. This will lead to a call to __stack_chk_fail
> then panic.
>
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94722
> Link: https://lore.kernel.org/all/[email protected]/
> Signed-off-by: Nick Desaulniers <[email protected]>
I applied this in front of Josh's series and defconfig no longer panics
on boot :)
Tested-by: Nathan Chancellor <[email protected]>
> ---
> arch/powerpc/kernel/smp.c | 1 +
> include/linux/compiler_attributes.h | 12 ++++++++++++
> init/main.c | 3 ++-
> 3 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 6b90f10a6c81..7d4c12b1abb7 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -1603,6 +1603,7 @@ static void add_cpu_to_masks(int cpu)
> }
>
> /* Activate a secondary processor. */
> +__no_stack_protector
> void start_secondary(void *unused)
> {
> unsigned int cpu = raw_smp_processor_id();
> diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
> index e659cb6fded3..84864767a56a 100644
> --- a/include/linux/compiler_attributes.h
> +++ b/include/linux/compiler_attributes.h
> @@ -255,6 +255,18 @@
> */
> #define __noreturn __attribute__((__noreturn__))
>
> +/*
> + * Optional: only supported since GCC >= 11.1, clang >= 7.0.
> + *
> + * gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-no_005fstack_005fprotector-function-attribute
> + * clang: https://clang.llvm.org/docs/AttributeReference.html#no-stack-protector-safebuffers
> + */
> +#if __has_attribute(__no_stack_protector__)
> +# define __no_stack_protector __attribute__((__no_stack_protector__))
> +#else
> +# define __no_stack_protector
> +#endif
> +
> /*
> * Optional: not supported by gcc.
> *
> diff --git a/init/main.c b/init/main.c
> index bb87b789c543..213baf7b8cb1 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -941,7 +941,8 @@ static void __init print_unknown_bootoptions(void)
> memblock_free(unknown_options, len);
> }
>
> -asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
> +asmlinkage __visible __init __no_sanitize_address __no_stack_protector
> +void start_kernel(void)
> {
> char *command_line;
> char *after_dashes;
>
> --
> 2.40.0.577.gac1e443424-goog
>
>
On Wed, Apr 12, 2023 at 11:32:13AM -0700, [email protected] wrote:
> prevent_tail_call_optimization was added in
> commit a9a3ed1eff36 ("x86: Fix early boot crash on gcc-10, third try")
> to work around stack canaries getting inserted into functions that would
> initialize the stack canary in the first place.
>
> Now that we have no_stack_protector function attribute (gcc-11+,
> clang-7+) and use it on start_kernel, remove the call to
> prevent_tail_call_optimization such that we may one day remove it
> outright.
>
> Signed-off-by: Nick Desaulniers <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
> ---
> init/main.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/init/main.c b/init/main.c
> index 213baf7b8cb1..c8503d02dfa6 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1152,7 +1152,12 @@ void start_kernel(void)
> /* Do the rest non-__init'ed, we're now alive */
> arch_call_rest_init();
>
> + /* Avoid stack canaries in callers of boot_init_stack_canary for gcc-10
> + * and older.
> + */
> +#if !__has_attribute(__no_stack_protector__)
> prevent_tail_call_optimization();
> +#endif
> }
>
> /* Call all constructor functions linked into the kernel. */
>
> --
> 2.40.0.577.gac1e443424-goog
>
On Wed, Apr 12, 2023 at 11:32:11AM -0700, [email protected] wrote:
> Nick Desaulniers (2):
> start_kernel: add no_stack_protector fn attr
> start_kernel: omit prevent_tail_call_optimization for newer toolchains
>
Your second patch has a vile comment style :-)
Other than that,
Acked-by: Peter Zijlstra (Intel) <[email protected]>
[email protected] writes:
> Back during the discussion of
> commit a9a3ed1eff36 ("x86: Fix early boot crash on gcc-10, third try")
> we discussed the need for a function attribute to control the omission
> of stack protectors on a per-function basis; at the time Clang had
> support for no_stack_protector but GCC did not. This was fixed in
> gcc-11. Now that the function attribute is available, let's start using
> it.
>
> Callers of boot_init_stack_canary need to use this function attribute
> unless they're compiled with -fno-stack-protector, otherwise the canary
> stored in the stack slot of the caller will differ upon the call to
> boot_init_stack_canary. This will lead to a call to __stack_chk_fail
> then panic.
>
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94722
> Link: https://lore.kernel.org/all/[email protected]/
> Signed-off-by: Nick Desaulniers <[email protected]>
> ---
> arch/powerpc/kernel/smp.c | 1 +
> include/linux/compiler_attributes.h | 12 ++++++++++++
> init/main.c | 3 ++-
> 3 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 6b90f10a6c81..7d4c12b1abb7 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -1603,6 +1603,7 @@ static void add_cpu_to_masks(int cpu)
> }
>
> /* Activate a secondary processor. */
> +__no_stack_protector
> void start_secondary(void *unused)
> {
> unsigned int cpu = raw_smp_processor_id();
start_secondary() doesn't return, so it won't actually crash, but it
obviously makes sense for it to be marked with __no_stack_protector.
There's quite a few other places we could add __no_stack_protector
annotations in powerpc code, and then make the changes to CFLAGS to
disable stack protector conditional on GCC < 11.
So I guess this patch is fine, but there's more that could be done.
Acked-by: Michael Ellerman <[email protected]> (powerpc)
cheers