| Message ID | 20220303074339.86337-1-ashimida@linux.alibaba.com |
|---|---|
| State | New |
| Headers | show |
| Series | AARCH64: Enable GCC-based Shadow Call Stack | expand |
On Wed, Mar 02, 2022 at 11:43:39PM -0800, Dan Li wrote: > Add tests for SCS (Shadow Call Stack) based > backward CFI (as implemented by Clang and GCC). Cool; thanks for writing these! > > Signed-off-by: Dan Li <ashimida@linux.alibaba.com> > --- > drivers/misc/lkdtm/Makefile | 1 + > drivers/misc/lkdtm/core.c | 2 + > drivers/misc/lkdtm/lkdtm.h | 4 ++ > drivers/misc/lkdtm/scs.c | 67 +++++++++++++++++++++++++ > tools/testing/selftests/lkdtm/tests.txt | 2 + > 5 files changed, 76 insertions(+) > create mode 100644 drivers/misc/lkdtm/scs.c > > diff --git a/drivers/misc/lkdtm/Makefile b/drivers/misc/lkdtm/Makefile > index 2e0aa74ac185..e2fb17868af2 100644 > --- a/drivers/misc/lkdtm/Makefile > +++ b/drivers/misc/lkdtm/Makefile > @@ -10,6 +10,7 @@ lkdtm-$(CONFIG_LKDTM) += rodata_objcopy.o > lkdtm-$(CONFIG_LKDTM) += usercopy.o > lkdtm-$(CONFIG_LKDTM) += stackleak.o > lkdtm-$(CONFIG_LKDTM) += cfi.o > +lkdtm-$(CONFIG_LKDTM) += scs.o I'd expect these to be in cfi.c, rather than making a new source file. > lkdtm-$(CONFIG_LKDTM) += fortify.o > lkdtm-$(CONFIG_PPC_64S_HASH_MMU) += powerpc.o > > diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c > index f69b964b9952..d0ce0bec117c 100644 > --- a/drivers/misc/lkdtm/core.c > +++ b/drivers/misc/lkdtm/core.c > @@ -178,6 +178,8 @@ static const struct crashtype crashtypes[] = { > CRASHTYPE(USERCOPY_KERNEL), > CRASHTYPE(STACKLEAK_ERASING), > CRASHTYPE(CFI_FORWARD_PROTO), > + CRASHTYPE(CFI_BACKWARD_SHADOW), > + CRASHTYPE(CFI_BACKWARD_SHADOW_WITH_NOSCS), > CRASHTYPE(FORTIFIED_OBJECT), > CRASHTYPE(FORTIFIED_SUBOBJECT), > CRASHTYPE(FORTIFIED_STRSCPY), > diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h > index d6137c70ebbe..a23d32dfc10b 100644 > --- a/drivers/misc/lkdtm/lkdtm.h > +++ b/drivers/misc/lkdtm/lkdtm.h > @@ -158,6 +158,10 @@ void lkdtm_STACKLEAK_ERASING(void); > /* cfi.c */ > void lkdtm_CFI_FORWARD_PROTO(void); > > +/* scs.c */ > +void lkdtm_CFI_BACKWARD_SHADOW(void); > +void lkdtm_CFI_BACKWARD_SHADOW_WITH_NOSCS(void); > + > /* fortify.c */ > void lkdtm_FORTIFIED_OBJECT(void); > void lkdtm_FORTIFIED_SUBOBJECT(void); > diff --git a/drivers/misc/lkdtm/scs.c b/drivers/misc/lkdtm/scs.c > new file mode 100644 > index 000000000000..5922a55a8844 > --- /dev/null > +++ b/drivers/misc/lkdtm/scs.c > @@ -0,0 +1,67 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * This is for all the tests relating directly to Shadow Call Stack. > + */ > +#include "lkdtm.h" > + > +#ifdef CONFIG_ARM64 > +/* Function clears its return address. */ > +static noinline void lkdtm_scs_clear_lr(void) > +{ > + unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1; > + > + asm volatile("str xzr, [%0]\n\t" : : "r"(lr) : "x30"); Is the asm needed here? Why not: unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1; *lr = 0; > +} > + > +/* Function with __noscs attribute clears its return address. */ > +static noinline void __noscs lkdtm_noscs_clear_lr(void) > +{ > + unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1; > + > + asm volatile("str xzr, [%0]\n\t" : : "r"(lr) : "x30"); > +} > +#endif > + > +/* > + * This tries to call a function protected by Shadow Call Stack, > + * which corrupts its own return address during execution. > + * Due to the protection, the corruption will not take effect > + * when the function returns. > + */ > +void lkdtm_CFI_BACKWARD_SHADOW(void) I think these two tests should be collapsed into a single one. > +{ > +#ifdef CONFIG_ARM64 > + if (!IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) { > + pr_err("FAIL: kernel not built with CONFIG_SHADOW_CALL_STACK\n"); > + return; > + } > + > + pr_info("Trying to corrupt lr in a function with scs protection ...\n"); > + lkdtm_scs_clear_lr(); > + > + pr_err("ok: scs takes effect.\n"); > +#else > + pr_err("XFAIL: this test is arm64-only\n"); > +#endif This is slightly surprising -- we have no detection when a function has its non-shadow-stack return address corrupted: it just _ignores_ the value stored there. That seems like a missed opportunity for warning about an unexpected state. > +} > + > +/* > + * This tries to call a function not protected by Shadow Call Stack, > + * which corrupts its own return address during execution. > + */ > +void lkdtm_CFI_BACKWARD_SHADOW_WITH_NOSCS(void) > +{ > +#ifdef CONFIG_ARM64 > + if (!IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) { > + pr_err("FAIL: kernel not built with CONFIG_SHADOW_CALL_STACK\n"); > + return; Other tests try to give some hints about failures, e.g.: pr_err("FAIL: cannot change for SCS\n"); pr_expected_config(CONFIG_SHADOW_CALL_STACK); Though, having the IS_ENABLED in there makes me wonder if this test should instead be made _survivable_ on failure. Something like this, completely untested: #ifdef CONFIG_ARM64 static noinline void lkdtm_scs_set_lr(unsigned long *addr) { unsigned long **lr = (unsigned long **)__builtin_frame_address(0) + 1; *lr = addr; } /* Function with __noscs attribute clears its return address. */ static noinline void __noscs lkdtm_noscs_set_lr(unsigned long *addr) { unsigned long **lr = (unsigned long **)__builtin_frame_address(0) + 1; *lr = addr; } #endif void lkdtm_CFI_BACKWARD_SHADOW(void) { #ifdef CONFIG_ARM64 /* Verify the "normal" condition of LR corruption working. */ do { /* Keep label in scope to avoid compiler warning. */ if ((volatile int)0) goto unexpected; pr_info("Trying to corrupt lr in a function without scs protection ...\n"); lkdtm_noscs_set_lr(&&expected); unexpected: pr_err("XPASS: Unexpectedly survived lr corruption without scs?!\n"); break; expected: pr_err("ok: lr corruption redirected without scs.\n"); } while (0); do { /* Keep labe in scope to avoid compiler warning. */ if ((volatile int)0) goto good_scs; pr_info("Trying to corrupt lr in a function with scs protection ...\n"); lkdtm_scs_set_lr(&&bad_scs); good_scs: pr_info("ok: scs takes effect.\n"); break; bad_scs: pr_err("FAIL: return address rewritten!\n"); pr_expected_config(CONFIG_SHADOW_CALL_STACK); } while (0); #else pr_err("XFAIL: this test is arm64-only\n"); #endif } And we should, actually, be able to make the "set_lr" functions be arch-specific, leaving the test itself arch-agnostic....
On 3/3/22 10:42, Kees Cook wrote: > On Wed, Mar 02, 2022 at 11:43:39PM -0800, Dan Li wrote: >> Add tests for SCS (Shadow Call Stack) based >> backward CFI (as implemented by Clang and GCC). > > Cool; thanks for writing these! > >> +lkdtm-$(CONFIG_LKDTM) += scs.o > > I'd expect these to be in cfi.c, rather than making a new source file. > Got it. >> +static noinline void lkdtm_scs_clear_lr(void) >> +{ >> + unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1; >> + >> + asm volatile("str xzr, [%0]\n\t" : : "r"(lr) : "x30"); > > Is the asm needed here? Why not: > > unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1; > > *lr = 0; > Yeah, with "volatile", this one looks better. >> + >> +/* >> + * This tries to call a function protected by Shadow Call Stack, >> + * which corrupts its own return address during execution. >> + * Due to the protection, the corruption will not take effect >> + * when the function returns. >> + */ >> +void lkdtm_CFI_BACKWARD_SHADOW(void) > > I think these two tests should be collapsed into a single one. > It seems that there is currently no cross-line matching in selftests/lkdtm/run.sh, if we put these two into one function and assume we could make noscs_set_lr _survivable_ (like in your example). Then we could only match "CFI_BACKWARD_SHADOW ok: scs takes effect." in texts.txt But if the test result is: XPASS: Unexpectedly survived lr corruption without scs? ok: scs takes effect. It may not be a real pass, but the xxx_set_lr function doesn't work. >> +{ >> +#ifdef CONFIG_ARM64 >> + if (!IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) { >> + pr_err("FAIL: kernel not built with CONFIG_SHADOW_CALL_STACK\n"); >> + return; >> + } >> + >> + pr_info("Trying to corrupt lr in a function with scs protection ...\n"); >> + lkdtm_scs_clear_lr(); >> + >> + pr_err("ok: scs takes effect.\n"); >> +#else >> + pr_err("XFAIL: this test is arm64-only\n"); >> +#endif > > This is slightly surprising -- we have no detection when a function has > its non-shadow-stack return address corrupted: it just _ignores_ the > value stored there. That seems like a missed opportunity for warning > about an unexpected state. > Yes. Actually I used to try in the plugin to add a detection before the function returns, and call a callback when a mismatch is found. But since almost every function has to be instrumented, the performance penalty is improved from <3% to ~20% (rough calculation, should still be optimized). >> +} >> + >> +/* >> + * This tries to call a function not protected by Shadow Call Stack, >> + * which corrupts its own return address during execution. >> + */ >> +void lkdtm_CFI_BACKWARD_SHADOW_WITH_NOSCS(void) >> +{ >> +#ifdef CONFIG_ARM64 >> + if (!IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) { >> + pr_err("FAIL: kernel not built with CONFIG_SHADOW_CALL_STACK\n"); >> + return; > > Other tests try to give some hints about failures, e.g.: > > pr_err("FAIL: cannot change for SCS\n"); > pr_expected_config(CONFIG_SHADOW_CALL_STACK); > > Though, having the IS_ENABLED in there makes me wonder if this test > should instead be made _survivable_ on failure. Something like this, > completely untested: > > > #ifdef CONFIG_ARM64 > static noinline void lkdtm_scs_set_lr(unsigned long *addr) > { > unsigned long **lr = (unsigned long **)__builtin_frame_address(0) + 1; > *lr = addr; > } > > /* Function with __noscs attribute clears its return address. */ > static noinline void __noscs lkdtm_noscs_set_lr(unsigned long *addr) > { > unsigned long **lr = (unsigned long **)__builtin_frame_address(0) + 1; > *lr = addr; > } > #endif > > > void lkdtm_CFI_BACKWARD_SHADOW(void) > { > #ifdef CONFIG_ARM64 > > /* Verify the "normal" condition of LR corruption working. */ > do { > /* Keep label in scope to avoid compiler warning. */ > if ((volatile int)0) > goto unexpected; > > pr_info("Trying to corrupt lr in a function without scs protection ...\n"); > lkdtm_noscs_set_lr(&&expected); > > unexpected: > pr_err("XPASS: Unexpectedly survived lr corruption without scs?!\n"); > break; > > expected: > pr_err("ok: lr corruption redirected without scs.\n"); > } while (0); > > > do { > /* Keep labe in scope to avoid compiler warning. */ > if ((volatile int)0) > goto good_scs; > > pr_info("Trying to corrupt lr in a function with scs protection ...\n"); > lkdtm_scs_set_lr(&&bad_scs); > > good_scs: > pr_info("ok: scs takes effect.\n"); > break; > > bad_scs: > pr_err("FAIL: return address rewritten!\n"); > pr_expected_config(CONFIG_SHADOW_CALL_STACK); > } while (0); > #else > pr_err("XFAIL: this test is arm64-only\n"); > #endif > } > Thanks for the example, Kees :) This code (with a little modification) works correctly with clang 12, but to make sure it's always correct, I think we might need to add the __attribute__((optnone)) attribute to it, because under -O2 the result doesn't seem to be "very stable" (as in your example in the next email). > And we should, actually, be able to make the "set_lr" functions be > arch-specific, leaving the test itself arch-agnostic.... > I'm not sure if my understanding is correct, do it means we should remove the "#ifdef CONFIG_ARM64" in lkdtm_CFI_BACKWARD_SHADOW? Then we may not be able to distinguish between failures caused by platform unsupported (XFAIL) and features not enabled (or not working properly). Thanks, Dan.
On 3/3/22 11:09, Kees Cook wrote: > On Thu, Mar 03, 2022 at 10:42:45AM -0800, Kees Cook wrote: >> Though, having the IS_ENABLED in there makes me wonder if this test >> should instead be made _survivable_ on failure. Something like this, >> completely untested: >> >> >> And we should, actually, be able to make the "set_lr" functions be >> arch-specific, leaving the test itself arch-agnostic.... > > Yeah, as a tested example, this works for x86_64, and based on what you > had, I'd expect it to work on arm64 too: > > #include <stdio.h> > > static __attribute__((noinline)) > void set_return_addr(unsigned long *expected, unsigned long *addr) > { > /* Use of volatile is to make sure final write isn't seen as a dead store. */ > unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1; > > /* Make sure we've found the right place on the stack before writing it. */ > if (*ret_addr == expected) > *ret_addr = addr; > } > > volatile int force_label; > int main(void) > { > do { > /* Keep labels in scope. */ > if (force_label) > goto normal; > if (force_label) > goto redirected; > > set_return_addr(&&normal, &&redirected); > normal: > printf("I should be skipped\n"); > break; From the assembly code, it seems that "&&normal" does't always equal to the address of label "normal" when we use clang with -O2. > redirected: > printf("Redirected\n"); > } while (0); > The address of "&&redirected" may appear in the middle of the assembly instructions of the printf. If we unconditionally jump to "&&normal", it may crash directly because x0 is not set correctly. > return 0; > } > > > It does _not_ work under Clang, though, which I'm still looking at. > AFAICT, maybe we could specify -O0 optimization to bypass this. BTW: Occasionally found, the following code works correctly, but i think it doesn't solve the issue :) #include <stdio.h> static __attribute__((noinline)) void set_return_addr(unsigned long *expected, unsigned long *addr) { /* Use of volatile is to make sure final write isn't seen as a dead store. */ unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1; /* Make sure we've found the right place on the stack before writing it. */ // if (*ret_addr == expected) *ret_addr = addr; } volatile int force_label; int main(void) { do { /* Keep labels in scope. */ if (force_label) goto normal; if (force_label) goto redirected; set_return_addr(&&normal, &&redirected); normal: printf("I should be skipped\n"); break; redirected: printf("Redirected\n"); printf("\n"); //add a new printf } while (0); return 0; }
On 3/4/22 06:54, Dan Li wrote: > > > On 3/3/22 11:09, Kees Cook wrote: >> On Thu, Mar 03, 2022 at 10:42:45AM -0800, Kees Cook wrote: >>> Though, having the IS_ENABLED in there makes me wonder if this test >>> should instead be made _survivable_ on failure. Something like this, >>> completely untested: >>> >>> >>> And we should, actually, be able to make the "set_lr" functions be >>> arch-specific, leaving the test itself arch-agnostic.... >> >> Yeah, as a tested example, this works for x86_64, and based on what you >> had, I'd expect it to work on arm64 too: >> >> #include <stdio.h> >> >> static __attribute__((noinline)) >> void set_return_addr(unsigned long *expected, unsigned long *addr) >> { >> /* Use of volatile is to make sure final write isn't seen as a dead store. */ >> unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1; >> >> /* Make sure we've found the right place on the stack before writing it. */ >> if (*ret_addr == expected) >> *ret_addr = addr; >> } >> >> volatile int force_label; >> int main(void) >> { >> do { >> /* Keep labels in scope. */ >> if (force_label) >> goto normal; >> if (force_label) >> goto redirected; >> >> set_return_addr(&&normal, &&redirected); >> normal: >> printf("I should be skipped\n"); >> break; > > From the assembly code, it seems that "&&normal" does't always equal > to the address of label "normal" when we use clang with -O2. > >> redirected: >> printf("Redirected\n"); >> } while (0); >> > > The address of "&&redirected" may appear in the middle of the assembly > instructions of the printf. If we unconditionally jump to "&&normal",> it may crash directly because x0 is not set correctly. Sorry, it should be: The address of "&&redirected" may appear in the middle of the assembly instructions of the printf. If we unconditionally jump to "&&redirected", it may crash directly because x0 of printf is not set correctly. Thanks, Dan. > >> return 0; >> } >> >> >> It does _not_ work under Clang, though, which I'm still looking at. >> > > AFAICT, maybe we could specify -O0 optimization to bypass this. > > > BTW: > Occasionally found, the following code works correctly, but i think > it doesn't solve the issue :) > > #include <stdio.h> > > static __attribute__((noinline)) > void set_return_addr(unsigned long *expected, unsigned long *addr) > { > /* Use of volatile is to make sure final write isn't seen as a dead store. */ > unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1; > > /* Make sure we've found the right place on the stack before writing it. */ > // if (*ret_addr == expected) > *ret_addr = addr; > } > volatile int force_label; > int main(void) > { > do { > /* Keep labels in scope. */ > if (force_label) > goto normal; > if (force_label) > goto redirected; > > set_return_addr(&&normal, &&redirected); > normal: > printf("I should be skipped\n"); > break; > > redirected: > printf("Redirected\n"); > printf("\n"); //add a new printf > } while (0); > > return 0; > }
On 3/3/22 11:09, Kees Cook wrote: > On Thu, Mar 03, 2022 at 10:42:45AM -0800, Kees Cook wrote: >> And we should, actually, be able to make the "set_lr" functions be >> arch-specific, leaving the test itself arch-agnostic.... > > Yeah, as a tested example, this works for x86_64, and based on what you > had, I'd expect it to work on arm64 too: > > #include <stdio.h> > > static __attribute__((noinline)) > void set_return_addr(unsigned long *expected, unsigned long *addr) > { > /* Use of volatile is to make sure final write isn't seen as a dead store. */ > unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1; > > /* Make sure we've found the right place on the stack before writing it. */ > if (*ret_addr == expected) > *ret_addr = addr; > } > > volatile int force_label; > int main(void) > { > do { > /* Keep labels in scope. */ > if (force_label) > goto normal; > if (force_label) > goto redirected; > > set_return_addr(&&normal, &&redirected); > normal: > printf("I should be skipped\n"); > break; > redirected: > printf("Redirected\n"); > } while (0); > > return 0; > } > > > It does _not_ work under Clang, though, which I'm still looking at. > The following code seems to work fine under clang/gcc, x86_64/aarch64 (also tested in lkdtm_CFI_BACKWARD_SHADOW): #include <stdio.h> static __attribute__((noinline)) void set_return_addr(unsigned long *expected, unsigned long *addr) { /* Use of volatile is to make sure final write isn't seen as a dead store. */ unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1; /* Make sure we've found the right place on the stack before writing it. */ if(*ret_addr == expected) *ret_addr = (addr); } static volatile int force_label; int main(void) { void *array[] = {0, &&normal, &&redirected}; if (force_label) { /* Call it with a NULL to avoid parameters being treated as constants in -02. */ set_return_addr(NULL, NULL); goto * array[force_label]; } do { set_return_addr(&&normal, &&redirected); normal: printf("I should be skipped\n"); break; redirected: printf("Redirected\n"); } while (0); return 0; } But currently it still crashes when I try to enable "-mbranch-protection=pac-ret+leaf+bti". Because the address of "&&redirected" is not encrypted under pac, the autiasp check will fail when set_return_addr returns, and eventually cause the function to crash when it returns to "&&redirected" ("&&redirected" as a reserved label always seems to start with a bti j insn). For lkdtm, if we're going to handle both cases in one function, maybe it would be better to turn off the -mbranch-protection=pac-ret+leaf+bti and maybe also turn off -O2 options for the function :) Thanks, Dan.
On Mon, Mar 07, 2022 at 07:16:36AM -0800, Dan Li wrote: > The following code seems to work fine under clang/gcc, x86_64/aarch64 > (also tested in lkdtm_CFI_BACKWARD_SHADOW): > > #include <stdio.h> > > static __attribute__((noinline)) > void set_return_addr(unsigned long *expected, unsigned long *addr) > { > /* Use of volatile is to make sure final write isn't seen as a dead store. */ > unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1; > > /* Make sure we've found the right place on the stack before writing it. */ > if(*ret_addr == expected) > *ret_addr = (addr); > } > > static volatile int force_label; > > int main(void) > { > void *array[] = {0, &&normal, &&redirected}; > > if (force_label) { > /* Call it with a NULL to avoid parameters being treated as constants in -02. */ > set_return_addr(NULL, NULL); > goto * array[force_label]; > } Hah! I like that. :) I had a weird switch statement that was working for me; this is cleaner. > > do { > > set_return_addr(&&normal, &&redirected); > > normal: > printf("I should be skipped\n"); > break; > > redirected: > printf("Redirected\n"); > > } while (0); > > return 0; > } > > But currently it still crashes when I try to enable > "-mbranch-protection=pac-ret+leaf+bti". > > Because the address of "&&redirected" is not encrypted under pac, > the autiasp check will fail when set_return_addr returns, and > eventually cause the function to crash when it returns to "&&redirected" > ("&&redirected" as a reserved label always seems to start with a bti j > insn). Strictly speaking, this is entirely correct. :) > For lkdtm, if we're going to handle both cases in one function, maybe > it would be better to turn off the -mbranch-protection=pac-ret+leaf+bti > and maybe also turn off -O2 options for the function :) If we can apply a function attribute to turn off pac for the "does this work without protections", that should be sufficient.
On 3/9/22 12:16, Kees Cook wrote: > On Mon, Mar 07, 2022 at 07:16:36AM -0800, Dan Li wrote: >> But currently it still crashes when I try to enable >> "-mbranch-protection=pac-ret+leaf+bti". >> >> Because the address of "&&redirected" is not encrypted under pac, >> the autiasp check will fail when set_return_addr returns, and >> eventually cause the function to crash when it returns to "&&redirected" >> ("&&redirected" as a reserved label always seems to start with a bti j >> insn). > > Strictly speaking, this is entirely correct. :) > >> For lkdtm, if we're going to handle both cases in one function, maybe >> it would be better to turn off the -mbranch-protection=pac-ret+leaf+bti >> and maybe also turn off -O2 options for the function :) > > If we can apply a function attribute to turn off pac for the "does this > work without protections", that should be sufficient. > Got it, will do in the next version :) Thanks, Dan.
On 3/14/22 06:53, Dan Li wrote: > Add tests for SCS (Shadow Call Stack) based backward CFI. > > + > +#ifdef CONFIG_ARM64 > +/* > + * This function is used to modify its return address. The PAC needs to be turned > + * off here to ensure that the modification of the return address will not be blocked. > + */ > +static noinline __no_ptrauth > +void lkdtm_scs_set_lr(unsigned long *expected, unsigned long *addr) > +{ > + /* Use of volatile is to make sure final write isn't seen as a dead store. */ > + unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1; > + > + /* Make sure we've found the right place on the stack before writing it. */ > + if (*ret_addr == expected) > + *ret_addr = addr; > +} > + > +/* Function with __noscs attribute attempts to modify its return address. */ > +static noinline __no_ptrauth __noscs > +void lkdtm_noscs_set_lr(unsigned long *expected, unsigned long *addr) > +{ > + /* Use of volatile is to make sure final write isn't seen as a dead store. */ > + unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1; > + > + /* Make sure we've found the right place on the stack before writing it. */ > + if (*ret_addr == expected) > + *ret_addr = addr; > +} > +#else > +static inline void lkdtm_noscs_set_lr(unsigned long *expected, unsigned long *addr) { } > +static inline void lkdtm_scs_set_lr(unsigned long *expected, unsigned long *addr) { } > +#endif > + > +static volatile unsigned int force_label; > + > +/* > + * This first checks whether a function with the __noscs attribute under > + * the current platform can directly modify its return address, and if so, > + * checks whether scs takes effect. > + */ > +void __no_optimize lkdtm_CFI_BACKWARD_SHADOW(void) > +{ > + void *array[] = {&&unexpected, &&expected, &&good_scs, &&bad_scs}; > + > + if (force_label && (force_label < sizeof(array))) { > + /* > + * Call them with "NULL" first to avoid > + * arguments being treated as constants in -02. > + */ > + lkdtm_noscs_set_lr(NULL, NULL); > + lkdtm_scs_set_lr(NULL, NULL); > + goto *array[force_label]; > + } > + > + /* Keep labels in scope to avoid compiler warnings. */ > + do { > + /* Verify the "normal" condition of LR corruption working. */ > + pr_info("Trying to corrupt lr in a function without scs protection ...\n"); > + lkdtm_noscs_set_lr(&&unexpected, &&expected); > + > +unexpected: Hi, Kees, With the default -O2, this code tests fine in gcc 11/clang 12, but doesn't work in gcc 7.5.0. In 7.5.0, the generated code is as follows: bl ffff8000088335c0 <lkdtm_noscs_set_lr> nop ## return address of lkdtm_noscs_set_lr adrp x0, ffff80000962b000 <kallsyms_token_index+0xe5908> ## address of "&&unexpected" The address of "&&unexpected" is still not guaranteed to always be the same as the return address of lkdtm_noscs_set_lr, so I had to add __no_optimize attribute here. The code compiled under __no_optimize in above versions works fine, but I saw the following description in the gcc user manual: "You may not use this mechanism to jump to code in a different function. If you do that, totally unpredictable things happen." So there might be some risk here that we might not be able to run this test case stably across all compiler versions, probably we still have to use two test cases to complete this. link: https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Labels-as-Values.html#Labels-as-Values Thanks, Dan. > + /* > + * If lr cannot be modified, the following check is meaningless, > + * returns directly. > + */ > + pr_err("XPASS: Unexpectedly survived lr corruption without scs?!\n"); > + break; > + > +expected: > + pr_info("ok: lr corruption redirected without scs.\n"); > + > + /* Verify that SCS is in effect. */ > + pr_info("Trying to corrupt lr in a function with scs protection ...\n"); > + lkdtm_scs_set_lr(&&good_scs, &&bad_scs); > + > +good_scs: > + pr_info("ok: scs takes effect.\n"); > + break; > + > +bad_scs: > + pr_err("FAIL: return address rewritten!\n"); > + pr_expected_config(CONFIG_SHADOW_CALL_STACK); > + } while (0); > +}
diff --git a/drivers/misc/lkdtm/Makefile b/drivers/misc/lkdtm/Makefile index 2e0aa74ac185..e2fb17868af2 100644 --- a/drivers/misc/lkdtm/Makefile +++ b/drivers/misc/lkdtm/Makefile @@ -10,6 +10,7 @@ lkdtm-$(CONFIG_LKDTM) += rodata_objcopy.o lkdtm-$(CONFIG_LKDTM) += usercopy.o lkdtm-$(CONFIG_LKDTM) += stackleak.o lkdtm-$(CONFIG_LKDTM) += cfi.o +lkdtm-$(CONFIG_LKDTM) += scs.o lkdtm-$(CONFIG_LKDTM) += fortify.o lkdtm-$(CONFIG_PPC_64S_HASH_MMU) += powerpc.o diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c index f69b964b9952..d0ce0bec117c 100644 --- a/drivers/misc/lkdtm/core.c +++ b/drivers/misc/lkdtm/core.c @@ -178,6 +178,8 @@ static const struct crashtype crashtypes[] = { CRASHTYPE(USERCOPY_KERNEL), CRASHTYPE(STACKLEAK_ERASING), CRASHTYPE(CFI_FORWARD_PROTO), + CRASHTYPE(CFI_BACKWARD_SHADOW), + CRASHTYPE(CFI_BACKWARD_SHADOW_WITH_NOSCS), CRASHTYPE(FORTIFIED_OBJECT), CRASHTYPE(FORTIFIED_SUBOBJECT), CRASHTYPE(FORTIFIED_STRSCPY), diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h index d6137c70ebbe..a23d32dfc10b 100644 --- a/drivers/misc/lkdtm/lkdtm.h +++ b/drivers/misc/lkdtm/lkdtm.h @@ -158,6 +158,10 @@ void lkdtm_STACKLEAK_ERASING(void); /* cfi.c */ void lkdtm_CFI_FORWARD_PROTO(void); +/* scs.c */ +void lkdtm_CFI_BACKWARD_SHADOW(void); +void lkdtm_CFI_BACKWARD_SHADOW_WITH_NOSCS(void); + /* fortify.c */ void lkdtm_FORTIFIED_OBJECT(void); void lkdtm_FORTIFIED_SUBOBJECT(void); diff --git a/drivers/misc/lkdtm/scs.c b/drivers/misc/lkdtm/scs.c new file mode 100644 index 000000000000..5922a55a8844 --- /dev/null +++ b/drivers/misc/lkdtm/scs.c @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * This is for all the tests relating directly to Shadow Call Stack. + */ +#include "lkdtm.h" + +#ifdef CONFIG_ARM64 +/* Function clears its return address. */ +static noinline void lkdtm_scs_clear_lr(void) +{ + unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1; + + asm volatile("str xzr, [%0]\n\t" : : "r"(lr) : "x30"); +} + +/* Function with __noscs attribute clears its return address. */ +static noinline void __noscs lkdtm_noscs_clear_lr(void) +{ + unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1; + + asm volatile("str xzr, [%0]\n\t" : : "r"(lr) : "x30"); +} +#endif + +/* + * This tries to call a function protected by Shadow Call Stack, + * which corrupts its own return address during execution. + * Due to the protection, the corruption will not take effect + * when the function returns. + */ +void lkdtm_CFI_BACKWARD_SHADOW(void) +{ +#ifdef CONFIG_ARM64 + if (!IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) { + pr_err("FAIL: kernel not built with CONFIG_SHADOW_CALL_STACK\n"); + return; + } + + pr_info("Trying to corrupt lr in a function with scs protection ...\n"); + lkdtm_scs_clear_lr(); + + pr_err("ok: scs takes effect.\n"); +#else + pr_err("XFAIL: this test is arm64-only\n"); +#endif +} + +/* + * This tries to call a function not protected by Shadow Call Stack, + * which corrupts its own return address during execution. + */ +void lkdtm_CFI_BACKWARD_SHADOW_WITH_NOSCS(void) +{ +#ifdef CONFIG_ARM64 + if (!IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) { + pr_err("FAIL: kernel not built with CONFIG_SHADOW_CALL_STACK\n"); + return; + } + + pr_info("Trying to corrupt lr in a function with attribute __noscs ...\n"); + lkdtm_noscs_clear_lr(); + + pr_err("FAIL: __noscs attribute does not take effect!\n"); +#else + pr_err("XFAIL: this test is arm64-only\n"); +#endif +} diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt index 6b36b7f5dcf9..c849765c8dcc 100644 --- a/tools/testing/selftests/lkdtm/tests.txt +++ b/tools/testing/selftests/lkdtm/tests.txt @@ -73,6 +73,8 @@ USERCOPY_STACK_BEYOND USERCOPY_KERNEL STACKLEAK_ERASING OK: the rest of the thread stack is properly erased CFI_FORWARD_PROTO +CFI_BACKWARD_SHADOW ok: scs takes effect +CFI_BACKWARD_SHADOW_WITH_NOSCS FORTIFIED_STRSCPY FORTIFIED_OBJECT FORTIFIED_SUBOBJECT
Add tests for SCS (Shadow Call Stack) based backward CFI (as implemented by Clang and GCC). Signed-off-by: Dan Li <ashimida@linux.alibaba.com> --- drivers/misc/lkdtm/Makefile | 1 + drivers/misc/lkdtm/core.c | 2 + drivers/misc/lkdtm/lkdtm.h | 4 ++ drivers/misc/lkdtm/scs.c | 67 +++++++++++++++++++++++++ tools/testing/selftests/lkdtm/tests.txt | 2 + 5 files changed, 76 insertions(+) create mode 100644 drivers/misc/lkdtm/scs.c