Shadow call stacks will be available in GCC >= 12, this series makes the corresponding kernel configuration available when compiling the kernel with the gcc and adds corresponding tests in lkdtm.
Dan Li (2): AARCH64: Add gcc Shadow Call Stack support lkdtm: Add Shadow Call Stack tests
arch/Kconfig | 19 +++---- arch/arm64/Kconfig | 2 +- drivers/misc/lkdtm/Makefile | 1 + drivers/misc/lkdtm/core.c | 2 + drivers/misc/lkdtm/lkdtm.h | 4 ++ drivers/misc/lkdtm/scs.c | 67 +++++++++++++++++++++++++ include/linux/compiler-gcc.h | 4 ++ tools/testing/selftests/lkdtm/tests.txt | 2 + 8 files changed, 91 insertions(+), 10 deletions(-) create mode 100644 drivers/misc/lkdtm/scs.c
Shadow call stacks will be available in GCC >= 12, this patch makes the corresponding kernel configuration available when compiling the kernel with the gcc.
Note that the implementation in GCC is slightly different from Clang. With SCS enabled, functions will only pop x30 once in the epilogue, like:
str x30, [x18], #8 stp x29, x30, [sp, #-16]! ...... - ldp x29, x30, [sp], #16 //clang + ldr x29, [sp], #16 //GCC ldr x30, [x18, #-8]!
Link: https://gcc.gnu.org/git/?p=gcc.git%3Ba=commit%3Bh=ce09ab17ddd21f73ff2caf6eec...
Reviewed-by: Nathan Chancellor nathan@kernel.org Reviewed-by: Kees Cook keescook@chromium.org Reviewed-by: Nick Desaulniers ndesaulniers@google.com Signed-off-by: Dan Li ashimida@linux.alibaba.com --- arch/Kconfig | 19 ++++++++++--------- arch/arm64/Kconfig | 2 +- include/linux/compiler-gcc.h | 4 ++++ 3 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig index 678a80713b21..cbbe824fe8b2 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -599,21 +599,22 @@ config STACKPROTECTOR_STRONG config ARCH_SUPPORTS_SHADOW_CALL_STACK bool help - An architecture should select this if it supports Clang's Shadow - Call Stack and implements runtime support for shadow stack + An architecture should select this if it supports the compiler's + Shadow Call Stack and implements runtime support for shadow stack switching.
config SHADOW_CALL_STACK - bool "Clang Shadow Call Stack" - depends on CC_IS_CLANG && ARCH_SUPPORTS_SHADOW_CALL_STACK + bool "Shadow Call Stack" + depends on ARCH_SUPPORTS_SHADOW_CALL_STACK depends on DYNAMIC_FTRACE_WITH_REGS || !FUNCTION_GRAPH_TRACER help - This option enables Clang's Shadow Call Stack, which uses a - shadow stack to protect function return addresses from being - overwritten by an attacker. More information can be found in - Clang's documentation: + This option enables the compiler's Shadow Call Stack, which + uses a shadow stack to protect function return addresses from + being overwritten by an attacker. More information can be found + in the compiler's documentation:
- https://clang.llvm.org/docs/ShadowCallStack.html + - Clang: https://clang.llvm.org/docs/ShadowCallStack.html + - GCC: https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#Instrumentat...
Note that security guarantees in the kernel differ from the ones documented for user space. The kernel must store addresses diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 09b885cc4db5..b7145337efae 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -1255,7 +1255,7 @@ config HW_PERF_EVENTS config ARCH_HAS_FILTER_PGPROT def_bool y
-# Supported by clang >= 7.0 +# Supported by clang >= 7.0 or GCC >= 12.0.0 config CC_HAVE_SHADOW_CALL_STACK def_bool $(cc-option, -fsanitize=shadow-call-stack -ffixed-x18)
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index ccbbd31b3aae..deff5b308470 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -97,6 +97,10 @@ #define KASAN_ABI_VERSION 4 #endif
+#ifdef CONFIG_SHADOW_CALL_STACK +#define __noscs __attribute__((__no_sanitize__("shadow-call-stack"))) +#endif + #if __has_attribute(__no_sanitize_address__) #define __no_sanitize_address __attribute__((no_sanitize_address)) #else
On Wed, 2 Mar 2022 23:43:23 -0800, Dan Li wrote:
Shadow call stacks will be available in GCC >= 12, this patch makes the corresponding kernel configuration available when compiling the kernel with the gcc.
Note that the implementation in GCC is slightly different from Clang. With SCS enabled, functions will only pop x30 once in the epilogue, like:
[...]
I'm taking this one now so it'll make the merge window. We can hammer out the lkdtm test after that.
Applied to for-next/hardening, thanks!
[1/2] arm64: Add gcc Shadow Call Stack support https://git.kernel.org/kees/c/afcf5441b9ff
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
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
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 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:
#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....
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.
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/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.
Add tests for SCS (Shadow Call Stack) based backward CFI.
Signed-off-by: Dan Li ashimida@linux.alibaba.com --- arch/arm64/include/asm/compiler.h | 18 ++++++ drivers/misc/lkdtm/cfi.c | 84 +++++++++++++++++++++++++ drivers/misc/lkdtm/core.c | 1 + drivers/misc/lkdtm/lkdtm.h | 1 + include/linux/compiler-clang.h | 1 + include/linux/compiler-gcc.h | 2 + include/linux/compiler_types.h | 4 ++ tools/testing/selftests/lkdtm/tests.txt | 1 + 8 files changed, 112 insertions(+)
diff --git a/arch/arm64/include/asm/compiler.h b/arch/arm64/include/asm/compiler.h index dc3ea4080e2e..96590fb4a8de 100644 --- a/arch/arm64/include/asm/compiler.h +++ b/arch/arm64/include/asm/compiler.h @@ -8,6 +8,24 @@ #define ARM64_ASM_PREAMBLE #endif
+#ifndef __ASSEMBLY__ +#ifdef __KERNEL__ + +#ifdef CONFIG_ARM64_PTR_AUTH_KERNEL + +#ifdef CONFIG_ARM64_BTI_KERNEL +# define __no_ptrauth __attribute__((target("branch-protection=bti"))) +#elif defined(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) +# define __no_ptrauth __attribute__((target("branch-protection=none"))) +#elif defined(CONFIG_CC_HAS_SIGN_RETURN_ADDRESS) +# define __no_ptrauth __attribute__((target("sign-return-address=none"))) +#endif + +#endif /* CONFIG_ARM64_PTR_AUTH_KERNEL */ + +#endif /* __KERNEL__ */ +#endif /* __ASSEMBLY__ */ + /* * The EL0/EL1 pointer bits used by a pointer authentication code. * This is dependent on TBI0/TBI1 being enabled, or bits 63:56 would also apply. diff --git a/drivers/misc/lkdtm/cfi.c b/drivers/misc/lkdtm/cfi.c index c9aeddef1044..468ba2f26f74 100644 --- a/drivers/misc/lkdtm/cfi.c +++ b/drivers/misc/lkdtm/cfi.c @@ -41,3 +41,87 @@ void lkdtm_CFI_FORWARD_PROTO(void) pr_err("FAIL: survived mismatched prototype function call!\n"); pr_expected_config(CONFIG_CFI_CLANG); } + +#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: + /* + * 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/core.c b/drivers/misc/lkdtm/core.c index f69b964b9952..7af7268b82e4 100644 --- a/drivers/misc/lkdtm/core.c +++ b/drivers/misc/lkdtm/core.c @@ -178,6 +178,7 @@ static const struct crashtype crashtypes[] = { CRASHTYPE(USERCOPY_KERNEL), CRASHTYPE(STACKLEAK_ERASING), CRASHTYPE(CFI_FORWARD_PROTO), + CRASHTYPE(CFI_BACKWARD_SHADOW), 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..a66fba949ab5 100644 --- a/drivers/misc/lkdtm/lkdtm.h +++ b/drivers/misc/lkdtm/lkdtm.h @@ -157,6 +157,7 @@ void lkdtm_STACKLEAK_ERASING(void);
/* cfi.c */ void lkdtm_CFI_FORWARD_PROTO(void); +void lkdtm_CFI_BACKWARD_SHADOW(void);
/* fortify.c */ void lkdtm_FORTIFIED_OBJECT(void); diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h index 3c4de9b6c6e3..2db37db36651 100644 --- a/include/linux/compiler-clang.h +++ b/include/linux/compiler-clang.h @@ -68,3 +68,4 @@
#define __nocfi __attribute__((__no_sanitize__("cfi"))) #define __cficanonical __attribute__((__cfi_canonical_jump_table__)) +#define __no_optimize __attribute__((optnone)) diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index deff5b308470..28d1b0ec6656 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -162,3 +162,5 @@ #if GCC_VERSION < 90100 #undef __alloc_size__ #endif + +#define __no_optimize __attribute__((optimize("-O0"))) diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index 3c1795fdb568..f5ad83f7ea2f 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -257,6 +257,10 @@ struct ftrace_likely_data { # define __nocfi #endif
+#ifndef __no_ptrauth +# define __no_ptrauth +#endif + #ifndef __cficanonical # define __cficanonical #endif diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt index 6b36b7f5dcf9..12df67a3b419 100644 --- a/tools/testing/selftests/lkdtm/tests.txt +++ b/tools/testing/selftests/lkdtm/tests.txt @@ -73,6 +73,7 @@ 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 FORTIFIED_STRSCPY FORTIFIED_OBJECT FORTIFIED_SUBOBJECT
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-a...
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);
+}
Hi Kees,
Gentile ping for this :).
I also saw the discussion on llvm-project, use address of labels as a parameter doesn't seem to be stable.
Do we need to split it into two cases here?
Link: https://github.com/llvm/llvm-project/issues/54328
Thanks, Dan
On 3/14/22 06:53, Dan Li wrote:
Add tests for SCS (Shadow Call Stack) based backward CFI.
Signed-off-by: Dan Li ashimida@linux.alibaba.com
arch/arm64/include/asm/compiler.h | 18 ++++++ drivers/misc/lkdtm/cfi.c | 84 +++++++++++++++++++++++++ drivers/misc/lkdtm/core.c | 1 + drivers/misc/lkdtm/lkdtm.h | 1 + include/linux/compiler-clang.h | 1 + include/linux/compiler-gcc.h | 2 + include/linux/compiler_types.h | 4 ++ tools/testing/selftests/lkdtm/tests.txt | 1 + 8 files changed, 112 insertions(+)
diff --git a/arch/arm64/include/asm/compiler.h b/arch/arm64/include/asm/compiler.h index dc3ea4080e2e..96590fb4a8de 100644 --- a/arch/arm64/include/asm/compiler.h +++ b/arch/arm64/include/asm/compiler.h @@ -8,6 +8,24 @@ #define ARM64_ASM_PREAMBLE #endif +#ifndef __ASSEMBLY__ +#ifdef __KERNEL__
+#ifdef CONFIG_ARM64_PTR_AUTH_KERNEL
+#ifdef CONFIG_ARM64_BTI_KERNEL +# define __no_ptrauth __attribute__((target("branch-protection=bti"))) +#elif defined(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) +# define __no_ptrauth __attribute__((target("branch-protection=none"))) +#elif defined(CONFIG_CC_HAS_SIGN_RETURN_ADDRESS) +# define __no_ptrauth __attribute__((target("sign-return-address=none"))) +#endif
+#endif /* CONFIG_ARM64_PTR_AUTH_KERNEL */
+#endif /* __KERNEL__ */ +#endif /* __ASSEMBLY__ */
- /*
- The EL0/EL1 pointer bits used by a pointer authentication code.
- This is dependent on TBI0/TBI1 being enabled, or bits 63:56 would also apply.
diff --git a/drivers/misc/lkdtm/cfi.c b/drivers/misc/lkdtm/cfi.c index c9aeddef1044..468ba2f26f74 100644 --- a/drivers/misc/lkdtm/cfi.c +++ b/drivers/misc/lkdtm/cfi.c @@ -41,3 +41,87 @@ void lkdtm_CFI_FORWARD_PROTO(void) pr_err("FAIL: survived mismatched prototype function call!\n"); pr_expected_config(CONFIG_CFI_CLANG); }
+#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:
/** 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/core.c b/drivers/misc/lkdtm/core.c index f69b964b9952..7af7268b82e4 100644 --- a/drivers/misc/lkdtm/core.c +++ b/drivers/misc/lkdtm/core.c @@ -178,6 +178,7 @@ static const struct crashtype crashtypes[] = { CRASHTYPE(USERCOPY_KERNEL), CRASHTYPE(STACKLEAK_ERASING), CRASHTYPE(CFI_FORWARD_PROTO),
- CRASHTYPE(CFI_BACKWARD_SHADOW), 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..a66fba949ab5 100644 --- a/drivers/misc/lkdtm/lkdtm.h +++ b/drivers/misc/lkdtm/lkdtm.h @@ -157,6 +157,7 @@ void lkdtm_STACKLEAK_ERASING(void); /* cfi.c */ void lkdtm_CFI_FORWARD_PROTO(void); +void lkdtm_CFI_BACKWARD_SHADOW(void); /* fortify.c */ void lkdtm_FORTIFIED_OBJECT(void); diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h index 3c4de9b6c6e3..2db37db36651 100644 --- a/include/linux/compiler-clang.h +++ b/include/linux/compiler-clang.h @@ -68,3 +68,4 @@ #define __nocfi __attribute__((__no_sanitize__("cfi"))) #define __cficanonical __attribute__((__cfi_canonical_jump_table__)) +#define __no_optimize __attribute__((optnone)) diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index deff5b308470..28d1b0ec6656 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -162,3 +162,5 @@ #if GCC_VERSION < 90100 #undef __alloc_size__ #endif
+#define __no_optimize __attribute__((optimize("-O0"))) diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index 3c1795fdb568..f5ad83f7ea2f 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -257,6 +257,10 @@ struct ftrace_likely_data { # define __nocfi #endif +#ifndef __no_ptrauth +# define __no_ptrauth +#endif
- #ifndef __cficanonical # define __cficanonical #endif
diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt index 6b36b7f5dcf9..12df67a3b419 100644 --- a/tools/testing/selftests/lkdtm/tests.txt +++ b/tools/testing/selftests/lkdtm/tests.txt @@ -73,6 +73,7 @@ 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 FORTIFIED_STRSCPY FORTIFIED_OBJECT FORTIFIED_SUBOBJECT
Hi Kees,
Gentile ping for this :).
I also saw the discussion on llvm-project, use address of labels as a parameter doesn't seem to be stable.
Do we need to split it into two cases here?
Link: https://github.com/llvm/llvm-project/issues/54328
Thanks, Dan
On 3/14/22 06:53, Dan Li wrote:
Add tests for SCS (Shadow Call Stack) based backward CFI.
Signed-off-by: Dan Li ashimida@linux.alibaba.com
arch/arm64/include/asm/compiler.h | 18 ++++++ drivers/misc/lkdtm/cfi.c | 84 +++++++++++++++++++++++++ drivers/misc/lkdtm/core.c | 1 + drivers/misc/lkdtm/lkdtm.h | 1 + include/linux/compiler-clang.h | 1 + include/linux/compiler-gcc.h | 2 + include/linux/compiler_types.h | 4 ++ tools/testing/selftests/lkdtm/tests.txt | 1 + 8 files changed, 112 insertions(+)
diff --git a/arch/arm64/include/asm/compiler.h b/arch/arm64/include/asm/compiler.h index dc3ea4080e2e..96590fb4a8de 100644 --- a/arch/arm64/include/asm/compiler.h +++ b/arch/arm64/include/asm/compiler.h @@ -8,6 +8,24 @@ #define ARM64_ASM_PREAMBLE #endif +#ifndef __ASSEMBLY__ +#ifdef __KERNEL__
+#ifdef CONFIG_ARM64_PTR_AUTH_KERNEL
+#ifdef CONFIG_ARM64_BTI_KERNEL +# define __no_ptrauth __attribute__((target("branch-protection=bti"))) +#elif defined(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) +# define __no_ptrauth __attribute__((target("branch-protection=none"))) +#elif defined(CONFIG_CC_HAS_SIGN_RETURN_ADDRESS) +# define __no_ptrauth __attribute__((target("sign-return-address=none"))) +#endif
+#endif /* CONFIG_ARM64_PTR_AUTH_KERNEL */
+#endif /* __KERNEL__ */ +#endif /* __ASSEMBLY__ */
- /*
- The EL0/EL1 pointer bits used by a pointer authentication code.
- This is dependent on TBI0/TBI1 being enabled, or bits 63:56 would also apply.
diff --git a/drivers/misc/lkdtm/cfi.c b/drivers/misc/lkdtm/cfi.c index c9aeddef1044..468ba2f26f74 100644 --- a/drivers/misc/lkdtm/cfi.c +++ b/drivers/misc/lkdtm/cfi.c @@ -41,3 +41,87 @@ void lkdtm_CFI_FORWARD_PROTO(void) pr_err("FAIL: survived mismatched prototype function call!\n"); pr_expected_config(CONFIG_CFI_CLANG); }
+#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:
/** 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/core.c b/drivers/misc/lkdtm/core.c index f69b964b9952..7af7268b82e4 100644 --- a/drivers/misc/lkdtm/core.c +++ b/drivers/misc/lkdtm/core.c @@ -178,6 +178,7 @@ static const struct crashtype crashtypes[] = { CRASHTYPE(USERCOPY_KERNEL), CRASHTYPE(STACKLEAK_ERASING), CRASHTYPE(CFI_FORWARD_PROTO),
- CRASHTYPE(CFI_BACKWARD_SHADOW), 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..a66fba949ab5 100644 --- a/drivers/misc/lkdtm/lkdtm.h +++ b/drivers/misc/lkdtm/lkdtm.h @@ -157,6 +157,7 @@ void lkdtm_STACKLEAK_ERASING(void); /* cfi.c */ void lkdtm_CFI_FORWARD_PROTO(void); +void lkdtm_CFI_BACKWARD_SHADOW(void); /* fortify.c */ void lkdtm_FORTIFIED_OBJECT(void); diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h index 3c4de9b6c6e3..2db37db36651 100644 --- a/include/linux/compiler-clang.h +++ b/include/linux/compiler-clang.h @@ -68,3 +68,4 @@ #define __nocfi __attribute__((__no_sanitize__("cfi"))) #define __cficanonical __attribute__((__cfi_canonical_jump_table__)) +#define __no_optimize __attribute__((optnone)) diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index deff5b308470..28d1b0ec6656 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -162,3 +162,5 @@ #if GCC_VERSION < 90100 #undef __alloc_size__ #endif
+#define __no_optimize __attribute__((optimize("-O0"))) diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index 3c1795fdb568..f5ad83f7ea2f 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -257,6 +257,10 @@ struct ftrace_likely_data { # define __nocfi #endif +#ifndef __no_ptrauth +# define __no_ptrauth +#endif
- #ifndef __cficanonical # define __cficanonical #endif
diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt index 6b36b7f5dcf9..12df67a3b419 100644 --- a/tools/testing/selftests/lkdtm/tests.txt +++ b/tools/testing/selftests/lkdtm/tests.txt @@ -73,6 +73,7 @@ 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 FORTIFIED_STRSCPY FORTIFIED_OBJECT FORTIFIED_SUBOBJECT
linux-kselftest-mirror@lists.linaro.org
-
Dan Li -
Kees Cook