| Message ID | 20231128-clone3-shadow-stack-v4-5-8b28ffe4f676@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFT,v4,1/5] mm: Introduce ARCH_HAS_USER_SHADOW_STACK | expand |
On Tue, 2023-11-28 at 18:22 +0000, Mark Brown wrote: > + > +#define ENABLE_SHADOW_STACK > +static inline void enable_shadow_stack(void) > +{ > + int ret = ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK); > + if (ret == 0) > + shadow_stack_enabled = true; > +} > + > +#endif > + > +#ifndef ENABLE_SHADOW_STACK > +static void enable_shadow_stack(void) > +{ > +} > +#endif Without this diff, the test crashed for me on a shadow stack system: diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c index dbe52582573c..3236d97ed261 100644 --- a/tools/testing/selftests/clone3/clone3.c +++ b/tools/testing/selftests/clone3/clone3.c @@ -423,7 +423,7 @@ static const struct test tests[] = { }) #define ENABLE_SHADOW_STACK -static inline void enable_shadow_stack(void) +static inline __attribute__((always_inline)) void enable_shadow_stack(void) { int ret = ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK); if (ret == 0) The fix works by making sure control flow never returns to before the point shadow stack was enabled. Otherwise it will underflow the shadow stack. But I wonder if the clone3 test should get its shadow stack enabled the conventional elf bit way. So if it's all there (HW, kernel, glibc) then the test will run with shadow stack. Otherwise the test will run without shadow stack. The other reason is that the shadow stack test in the x86 selftest manual enabling is designed to work without a shadow stack enabled glibc and has to be specially crafted to work around the missing support. I'm not sure the more generic selftests should have to know how to do this. So what about something like this instead: diff --git a/tools/testing/selftests/clone3/Makefile b/tools/testing/selftests/clone3/Makefile index 84832c369a2e..792bc9685c82 100644 --- a/tools/testing/selftests/clone3/Makefile +++ b/tools/testing/selftests/clone3/Makefile @@ -2,6 +2,13 @@ CFLAGS += -g -std=gnu99 $(KHDR_INCLUDES) LDLIBS += -lcap +ifeq ($(shell uname -m),x86_64) +CAN_BUILD_WITH_SHSTK := $(shell ../x86/check_cc.sh gcc ../x86/trivial_program.c -mshstk -fcf-protection) +ifeq ($(CAN_BUILD_WITH_SHSTK),1) +CFLAGS += -mshstk -fcf-protection=return +endif +endif + TEST_GEN_PROGS := clone3 clone3_clear_sighand clone3_set_tid \ clone3_cap_checkpoint_restore diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c index dbe52582573c..eff5e8d5a5a6 100644 --- a/tools/testing/selftests/clone3/clone3.c +++ b/tools/testing/selftests/clone3/clone3.c @@ -23,7 +23,6 @@ #include "clone3_selftests.h" static bool shadow_stack_enabled; -static bool shadow_stack_supported; static size_t max_supported_args_size; enum test_mode { @@ -50,36 +49,6 @@ struct test { filter_function filter; }; -#ifndef __NR_map_shadow_stack -#define __NR_map_shadow_stack 453 -#endif - -/* - * We check for shadow stack support by attempting to use - * map_shadow_stack() since features may have been locked by the - * dynamic linker resulting in spurious errors when we attempt to - * enable on startup. We warn if the enable failed. - */ -static void test_shadow_stack_supported(void) -{ - long shadow_stack; - - shadow_stack = syscall(__NR_map_shadow_stack, 0, getpagesize(), 0); - if (shadow_stack == -1) { - ksft_print_msg("map_shadow_stack() not supported\n"); - } else if ((void *)shadow_stack == MAP_FAILED) { - ksft_print_msg("Failed to map shadow stack\n"); - } else { - ksft_print_msg("Shadow stack supportd\n"); - shadow_stack_supported = true; - - if (!shadow_stack_enabled) - ksft_print_msg("Mapped but did not enable shadow stack\n"); - - munmap((void *)shadow_stack, getpagesize()); - } -} - static int call_clone3(uint64_t flags, size_t size, enum test_mode test_mode) { struct __clone_args args = { @@ -220,7 +189,7 @@ static bool no_timenamespace(void) static bool have_shadow_stack(void) { - if (shadow_stack_supported) { + if (shadow_stack_enabled) { ksft_print_msg("Shadow stack supported\n"); return true; } @@ -230,7 +199,7 @@ static bool have_shadow_stack(void) static bool no_shadow_stack(void) { - if (!shadow_stack_supported) { + if (!shadow_stack_enabled) { ksft_print_msg("Shadow stack not supported\n"); return true; } @@ -402,38 +371,18 @@ static const struct test tests[] = { }; #ifdef __x86_64__ -#define ARCH_SHSTK_ENABLE 0x5001 +#define ARCH_SHSTK_STATUS 0x5005 #define ARCH_SHSTK_SHSTK (1ULL << 0) -#define ARCH_PRCTL(arg1, arg2) \ -({ \ - long _ret; \ - register long _num asm("eax") = __NR_arch_prctl; \ - register long _arg1 asm("rdi") = (long)(arg1); \ - register long _arg2 asm("rsi") = (long)(arg2); \ - \ - asm volatile ( \ - "syscall\n" \ - : "=a"(_ret) \ - : "r"(_arg1), "r"(_arg2), \ - "0"(_num) \ - : "rcx", "r11", "memory", "cc" \ - ); \ - _ret; \ -}) - -#define ENABLE_SHADOW_STACK -static inline void enable_shadow_stack(void) +static inline __attribute__((always_inline)) void check_shadow_stack(void) { - int ret = ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK); - if (ret == 0) - shadow_stack_enabled = true; + unsigned long status = 0; + + syscall(SYS_arch_prctl, ARCH_SHSTK_STATUS, &status); + shadow_stack_enabled = status & ARCH_SHSTK_SHSTK; } - -#endif - -#ifndef ENABLE_SHADOW_STACK -static void enable_shadow_stack(void) +#else /* __x86_64__ */ +static void check_shadow_stack(void) { } #endif @@ -443,12 +392,11 @@ int main(int argc, char *argv[]) size_t size; int i; - enable_shadow_stack(); + check_shadow_stack(); ksft_print_header(); ksft_set_plan(ARRAY_SIZE(tests)); test_clone3_supported(); - test_shadow_stack_supported(); for (i = 0; i < ARRAY_SIZE(tests); i++) test_clone3(&tests[i]);
On Tue, Dec 05, 2023 at 12:10:20AM +0000, Edgecombe, Rick P wrote: > Without this diff, the test crashed for me on a shadow stack system: > -static inline void enable_shadow_stack(void) > +static inline __attribute__((always_inline)) void doh. > But I wonder if the clone3 test should get its shadow stack enabled the > conventional elf bit way. So if it's all there (HW, kernel, glibc) then > the test will run with shadow stack. Otherwise the test will run > without shadow stack. This creates bootstrapping issues if we do it for arm64 where nothing is merged yet except for the model and EL3 support - in order to get any test coverage you need to be using an OS with the libc and toolchain support available and that's not going to be something we can rely on for a while (and even when things are merged a lot of the CI systems use Debian). There is a small risk that the toolchain will generate incompatible code if it doesn't know it's specifically targetting shadow stacks but the toolchain people didn't seem concerned about that risk and we've not been running into problems. It looks x86 is in better shape here with the userspace having run ahead of the kernel support though I'm not 100% clear if everything is fully lined up? -mshstk -fcf-protection appears to build fine with gcc 8 but I'm a bit less clear on glibc and any ABI variations. > The other reason is that the shadow stack test in the x86 selftest > manual enabling is designed to work without a shadow stack enabled > glibc and has to be specially crafted to work around the missing > support. I'm not sure the more generic selftests should have to know > how to do this. So what about something like this instead: What's the issue with working around the missing support? My understanding was that there should be no ill effects from repeated attempts to enable. We could add a check for things already being enabled
On Tue, 2023-12-05 at 15:05 +0000, Mark Brown wrote: > > But I wonder if the clone3 test should get its shadow stack enabled > > the > > conventional elf bit way. So if it's all there (HW, kernel, glibc) > > then > > the test will run with shadow stack. Otherwise the test will run > > without shadow stack. > > This creates bootstrapping issues if we do it for arm64 where nothing > is > merged yet except for the model and EL3 support - in order to get any > test coverage you need to be using an OS with the libc and toolchain > support available and that's not going to be something we can rely on > for a while (and even when things are merged a lot of the CI systems > use > Debian). There is a small risk that the toolchain will generate > incompatible code if it doesn't know it's specifically targetting > shadow > stacks but the toolchain people didn't seem concerned about that risk > and we've not been running into problems. > > It looks x86 is in better shape here with the userspace having run > ahead > of the kernel support though I'm not 100% clear if everything is > fully > lined up? -mshstk -fcf-protection appears to build fine with gcc 8 > but > I'm a bit less clear on glibc and any ABI variations. Right, you would need a shadow stack enabled compiler too. The check_cc.sh piece in the Makefile will detect that. Hmm, I didn't realize you were planning to have the kernel support upstream before the libc support was in testable shape. > > > The other reason is that the shadow stack test in the x86 selftest > > manual enabling is designed to work without a shadow stack enabled > > glibc and has to be specially crafted to work around the missing > > support. I'm not sure the more generic selftests should have to > > know > > how to do this. So what about something like this instead: > > What's the issue with working around the missing support? My > understanding was that there should be no ill effects from repeated > attempts to enable. We could add a check for things already being > enabled Normally the loader enables shadow stack and glibc then knows to do things in special ways when it is successful. If it instead manually enables in the app: - The app can't return from main() without disabling shadow stack beforehand. Luckily this test directly calls exit() - The app can't do longjmp() - The app can't do ucontext stuff - The enabling code needs to be carefully crafted (the inline problem you hit) I guess it's not a huge list, and mostly tests will run ok. But it doesn't seem right to add somewhat hacky shadow stack crud into generic tests. So you were planning to enable GCS in this test manually as well? How many tests were you planning to add it like this?
On Tue, Dec 05, 2023 at 04:01:50PM +0000, Edgecombe, Rick P wrote: > Hmm, I didn't realize you were planning to have the kernel support > upstream before the libc support was in testable shape. It's not a "could someone run it" thing - it's about trying ensure that we get coverage from people who are just running the selftests as part of general testing coverage rather than with the specific goal of testing this one feature. Even when things start to land there will be a considerable delay before they filter out so that all the enablement is in CI systems off the shelf and it'd be good to have coverage in that interval. > > What's the issue with working around the missing support? My > > understanding was that there should be no ill effects from repeated > > attempts to enable. We could add a check for things already being > > enabled > Normally the loader enables shadow stack and glibc then knows to do > things in special ways when it is successful. If it instead manually > enables in the app: > - The app can't return from main() without disabling shadow stack > beforehand. Luckily this test directly calls exit() > - The app can't do longjmp() > - The app can't do ucontext stuff > - The enabling code needs to be carefully crafted (the inline problem > you hit) > I guess it's not a huge list, and mostly tests will run ok. But it > doesn't seem right to add somewhat hacky shadow stack crud into generic > tests. Right, it's a small and fairly easily auditable list - it's more about the app than the double enable which was what I thought your concern was. It's a bit annoying definitely and not something we want to do in general but for something like this where we're adding specific coverage for API extensions for the feature it seems like a reasonable tradeoff. If the x86 toolchain/libc support is widely enough deployed (or you just don't mind any missing coverage) we could use the toolchain support there and only have the manual enable for arm64, it'd be inconsistent but not wildly so. > So you were planning to enable GCS in this test manually as well? How > many tests were you planning to add it like this? Yes, the current version of the arm64 series has the equivalent support for GCS. I was only planning to do this along with adding specific coverage for shadow stacks/GCS, general stuff that doesn't have any specific support can get covered as part of system testing with the toolchain and libc support. The only case beyond that I've done is some arm64 specific stress tests which are written as standalone assembler programs, those wouldn't get enabled by the toolchain anyway and have some chance of catching context switch or signal handling issues should they occur. It seemed worth it for the few lines of assembly it takes.
On Tue, 2023-12-05 at 16:43 +0000, Mark Brown wrote: > Right, it's a small and fairly easily auditable list - it's more > about > the app than the double enable which was what I thought your concern > was. It's a bit annoying definitely and not something we want to do > in > general but for something like this where we're adding specific > coverage > for API extensions for the feature it seems like a reasonable > tradeoff. > > If the x86 toolchain/libc support is widely enough deployed (or you > just > don't mind any missing coverage) we could use the toolchain support > there and only have the manual enable for arm64, it'd be inconsistent > but not wildly so. > > > I'm hoping there is not too much of a gap before the glibc support starts filtering out. Long term, elf bit enabling is probably the right thing for the generic tests. Short term, manual enabling is ok with me if no one else minds. Maybe we could add my "don't do" list as a comment if we do manual enabling? I'll have to check your new series, but I also wonder if we could cram the manual enabling and status checking pieces into some headers and not have to have "if x86" "if arm" logic in the test themselves.
diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c index 6adbfd14c841..dbe52582573c 100644 --- a/tools/testing/selftests/clone3/clone3.c +++ b/tools/testing/selftests/clone3/clone3.c @@ -11,6 +11,7 @@ #include <stdint.h> #include <stdio.h> #include <stdlib.h> +#include <sys/mman.h> #include <sys/syscall.h> #include <sys/types.h> #include <sys/un.h> @@ -21,6 +22,10 @@ #include "../kselftest.h" #include "clone3_selftests.h" +static bool shadow_stack_enabled; +static bool shadow_stack_supported; +static size_t max_supported_args_size; + enum test_mode { CLONE3_ARGS_NO_TEST, CLONE3_ARGS_ALL_0, @@ -28,6 +33,7 @@ enum test_mode { CLONE3_ARGS_INVAL_EXIT_SIGNAL_NEG, CLONE3_ARGS_INVAL_EXIT_SIGNAL_CSIG, CLONE3_ARGS_INVAL_EXIT_SIGNAL_NSIG, + CLONE3_ARGS_SHADOW_STACK, }; typedef bool (*filter_function)(void); @@ -44,6 +50,36 @@ struct test { filter_function filter; }; +#ifndef __NR_map_shadow_stack +#define __NR_map_shadow_stack 453 +#endif + +/* + * We check for shadow stack support by attempting to use + * map_shadow_stack() since features may have been locked by the + * dynamic linker resulting in spurious errors when we attempt to + * enable on startup. We warn if the enable failed. + */ +static void test_shadow_stack_supported(void) +{ + long shadow_stack; + + shadow_stack = syscall(__NR_map_shadow_stack, 0, getpagesize(), 0); + if (shadow_stack == -1) { + ksft_print_msg("map_shadow_stack() not supported\n"); + } else if ((void *)shadow_stack == MAP_FAILED) { + ksft_print_msg("Failed to map shadow stack\n"); + } else { + ksft_print_msg("Shadow stack supportd\n"); + shadow_stack_supported = true; + + if (!shadow_stack_enabled) + ksft_print_msg("Mapped but did not enable shadow stack\n"); + + munmap((void *)shadow_stack, getpagesize()); + } +} + static int call_clone3(uint64_t flags, size_t size, enum test_mode test_mode) { struct __clone_args args = { @@ -89,6 +125,9 @@ static int call_clone3(uint64_t flags, size_t size, enum test_mode test_mode) case CLONE3_ARGS_INVAL_EXIT_SIGNAL_NSIG: args.exit_signal = 0x00000000000000f0ULL; break; + case CLONE3_ARGS_SHADOW_STACK: + args.shadow_stack_size = getpagesize(); + break; } memcpy(&args_ext.args, &args, sizeof(struct __clone_args)); @@ -179,6 +218,26 @@ static bool no_timenamespace(void) return true; } +static bool have_shadow_stack(void) +{ + if (shadow_stack_supported) { + ksft_print_msg("Shadow stack supported\n"); + return true; + } + + return false; +} + +static bool no_shadow_stack(void) +{ + if (!shadow_stack_supported) { + ksft_print_msg("Shadow stack not supported\n"); + return true; + } + + return false; +} + static size_t page_size_plus_8(void) { return getpagesize() + 8; @@ -322,16 +381,74 @@ static const struct test tests[] = { .expected = -EINVAL, .test_mode = CLONE3_ARGS_NO_TEST, }, + { + .name = "Shadow stack on system with shadow stack", + .flags = CLONE_VM, + .size = 0, + .expected = 0, + .e2big_valid = true, + .test_mode = CLONE3_ARGS_SHADOW_STACK, + .filter = no_shadow_stack, + }, + { + .name = "Shadow stack on system without shadow stack", + .flags = CLONE_VM, + .size = 0, + .expected = -EINVAL, + .e2big_valid = true, + .test_mode = CLONE3_ARGS_SHADOW_STACK, + .filter = have_shadow_stack, + }, }; +#ifdef __x86_64__ +#define ARCH_SHSTK_ENABLE 0x5001 +#define ARCH_SHSTK_SHSTK (1ULL << 0) + +#define ARCH_PRCTL(arg1, arg2) \ +({ \ + long _ret; \ + register long _num asm("eax") = __NR_arch_prctl; \ + register long _arg1 asm("rdi") = (long)(arg1); \ + register long _arg2 asm("rsi") = (long)(arg2); \ + \ + asm volatile ( \ + "syscall\n" \ + : "=a"(_ret) \ + : "r"(_arg1), "r"(_arg2), \ + "0"(_num) \ + : "rcx", "r11", "memory", "cc" \ + ); \ + _ret; \ +}) + +#define ENABLE_SHADOW_STACK +static inline void enable_shadow_stack(void) +{ + int ret = ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK); + if (ret == 0) + shadow_stack_enabled = true; +} + +#endif + +#ifndef ENABLE_SHADOW_STACK +static void enable_shadow_stack(void) +{ +} +#endif + int main(int argc, char *argv[]) { size_t size; int i; + enable_shadow_stack(); + ksft_print_header(); ksft_set_plan(ARRAY_SIZE(tests)); test_clone3_supported(); + test_shadow_stack_supported(); for (i = 0; i < ARRAY_SIZE(tests); i++) test_clone3(&tests[i]); diff --git a/tools/testing/selftests/clone3/clone3_selftests.h b/tools/testing/selftests/clone3/clone3_selftests.h index 3d2663fe50ba..2e06127091f5 100644 --- a/tools/testing/selftests/clone3/clone3_selftests.h +++ b/tools/testing/selftests/clone3/clone3_selftests.h @@ -31,6 +31,13 @@ struct __clone_args { __aligned_u64 set_tid; __aligned_u64 set_tid_size; __aligned_u64 cgroup; +#ifndef CLONE_ARGS_SIZE_VER2 +#define CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */ +#endif + __aligned_u64 shadow_stack_size; +#ifndef CLONE_ARGS_SIZE_VER3 +#define CLONE_ARGS_SIZE_VER3 96 /* sizeof fourth published struct */ +#endif }; static pid_t sys_clone3(struct __clone_args *args, size_t size)
Add basic test coverage for specifying the shadow stack for a newly created thread via clone3(), including coverage of the newly extended argument structure. In order to facilitate testing on systems without userspace shadow stack support we manually enable shadow stacks on startup, this is architecture specific due to the use of an arch_prctl() on x86. Due to interactions with potential userspace locking of features we actually detect support for shadow stacks on the running system by attempting to allocate a shadow stack page during initialisation using map_shadow_stack(), warning if this succeeds when the enable failed. Signed-off-by: Mark Brown <broonie@kernel.org> --- tools/testing/selftests/clone3/clone3.c | 117 ++++++++++++++++++++++ tools/testing/selftests/clone3/clone3_selftests.h | 7 ++ 2 files changed, 124 insertions(+)