| Message ID | 20231114-clone3-shadow-stack-v2-5-b613f8681155@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fork: Support shadow stacks in clone3() | expand |
On Tue, 2023-11-14 at 20:05 +0000, Mark Brown wrote: > +static void test_shadow_stack_supported(void) > +{ > + long shadow_stack; > + > + shadow_stack = syscall(__NR_map_shadow_stack, 0, > getpagesize(), 0); Hmm, x86 fails this call if user shadow stack is not supported in the HW or the kernel, but doesn't care if it is enabled on the thread or not. If shadow stack is not enabled (or not yet enabled), shadow stacks are allowed to be mapped. Should it fail if shadow stack is not yet enabled? Since shadow stack is per thread, map_shadow_stack could still be called on another thread that has it enabled. Basically I don't think blocking it will reduce the possible states the kernel has to handle. The traditional way to check if shadow stack is enabled on x86 is the check for a non zero return from the _get_ssp() intrinsic: https://gcc.gnu.org/onlinedocs/gcc-9.2.0/gcc/x86-control-flow-protection-intrinsics.html It seems like there will be a need for some generic method of checking if shadow stack is enabled. Maybe a more generic compiler intrinsic/builtin or glibc API (something unrelated to SSP)? > + { > + .name = "Shadow stack on system with shadow stack", > + .flags = 0, > + .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 = 0, > + .size = 0, > + .expected = -EINVAL, > + .e2big_valid = true, > + .test_mode = CLONE3_ARGS_SHADOW_STACK, > + .filter = have_shadow_stack, > + }, > }; > I changed x86's map_shadow_stack to return an error when shadow stack was not enabled to make the detection logic in the test work. Also changed the clone3 Makefile to generate the shadow stack bit in the tests. When running the 'clone3' test with shadow stack it passed, but there is a failure in the non-shadow stack case: ... # Shadow stack not supported ok 20 # SKIP Shadow stack on system with shadow stack # Running test 'Shadow stack on system without shadow stack' # [1333] Trying clone3() with flags 0 (size 0) # I am the parent (1333). My child's pid is 1342 # I am the child, my PID is 1342 # [1333] clone3() with flags says: 0 expected -22 # [1333] Result (0) is different than expected (-22) not ok 21 Shadow stack on system without shadow stack # Totals: pass:19 fail:1 xfail:0 xpass:0 skip:1 error:0 The other tests passed in both cases. I'm going to dig into the other parts now but can circle back if it's not obvious what's going on there.
On Tue, Nov 14, 2023 at 11:11:58PM +0000, Edgecombe, Rick P wrote: > On Tue, 2023-11-14 at 20:05 +0000, Mark Brown wrote: > > + shadow_stack = syscall(__NR_map_shadow_stack, 0, > > getpagesize(), 0); > Hmm, x86 fails this call if user shadow stack is not supported in the > HW or the kernel, but doesn't care if it is enabled on the thread or > not. If shadow stack is not enabled (or not yet enabled), shadow stacks > are allowed to be mapped. Should it fail if shadow stack is not yet > enabled? > Since shadow stack is per thread, map_shadow_stack could still be > called on another thread that has it enabled. Basically I don't think > blocking it will reduce the possible states the kernel has to handle. Indeed - I would expect map_shadow_stack() to always succeed if the system supports it since it could reasonably be called as part of the preparation for enabling it and even if someone calls it and never actually uses the resulting memory there's no more harm to that than any other memory allocation. The arm64 code wasn't explicitly caring if we actually had GCS enabled when we clone and just alloacating the stack if requested which on reflection is more just an opportunity for users to mess up than something we usefully want to support. > The traditional way to check if shadow stack is enabled on x86 is the > check for a non zero return from the _get_ssp() intrinsic: > https://gcc.gnu.org/onlinedocs/gcc-9.2.0/gcc/x86-control-flow-protection-intrinsics.html > It seems like there will be a need for some generic method of checking > if shadow stack is enabled. Maybe a more generic compiler > intrinsic/builtin or glibc API (something unrelated to SSP)? Some sort of feature check in libc would be nice, yes. That said since we really want the tests to run on systems without libc support for the feature (if only as a bootstrapping thing) we'll need to open code anyway. I'll add code to startup which ensures the feature is enabled, we can't rely on it for detection without pain though since it's possible that we might have features locked by the startup code.
On Tue, 2023-11-14 at 15:11 -0800, Rick Edgecombe wrote: > The other tests passed in both cases. I'm going to dig into the other > parts now but can circle back if it's not obvious what's going on > there. Finally got back to this. I think it's just a problem with the shadow stack detection logic in the test.
On Tue, Nov 14, 2023 at 11:11:58PM +0000, Edgecombe, Rick P wrote: >On Tue, 2023-11-14 at 20:05 +0000, Mark Brown wrote: >> +static void test_shadow_stack_supported(void) >> +{ >> + long shadow_stack; >> + >> + shadow_stack = syscall(__NR_map_shadow_stack, 0, >> getpagesize(), 0); > >Hmm, x86 fails this call if user shadow stack is not supported in the >HW or the kernel, but doesn't care if it is enabled on the thread or >not. If shadow stack is not enabled (or not yet enabled), shadow stacks >are allowed to be mapped. Should it fail if shadow stack is not yet >enabled? > >Since shadow stack is per thread, map_shadow_stack could still be >called on another thread that has it enabled. Basically I don't think >blocking it will reduce the possible states the kernel has to handle. > >The traditional way to check if shadow stack is enabled on x86 is the >check for a non zero return from the _get_ssp() intrinsic: >https://gcc.gnu.org/onlinedocs/gcc-9.2.0/gcc/x86-control-flow-protection-intrinsics.html > >It seems like there will be a need for some generic method of checking >if shadow stack is enabled. Maybe a more generic compiler >intrinsic/builtin or glibc API (something unrelated to SSP)? Exposing a new file under procfs would be useful? Something like "/proc/sys/vm/user_shadow_stack_supported" `map_shadow_stack` can return MAP_FAILED for other reasons. I think `kselftests` are fine but I don't want people to pick up this as test code and run with it in production :-) So kernel providing a way to indicate whether it supports shadow stack mappings in user mode via procfs would be useful and arch agnostic. > >> + { >> + .name = "Shadow stack on system with shadow stack", >> + .flags = 0, >> + .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 = 0, >> + .size = 0, >> + .expected = -EINVAL, >> + .e2big_valid = true, >> + .test_mode = CLONE3_ARGS_SHADOW_STACK, >> + .filter = have_shadow_stack, >> + }, >> }; >> >I changed x86's map_shadow_stack to return an error when shadow stack >was not enabled to make the detection logic in the test work. Also >changed the clone3 Makefile to generate the shadow stack bit in the >tests. When running the 'clone3' test with shadow stack it passed, but >there is a failure in the non-shadow stack case: >... ># Shadow stack not supported >ok 20 # SKIP Shadow stack on system with shadow stack ># Running test 'Shadow stack on system without shadow stack' ># [1333] Trying clone3() with flags 0 (size 0) ># I am the parent (1333). My child's pid is 1342 ># I am the child, my PID is 1342 ># [1333] clone3() with flags says: 0 expected -22 ># [1333] Result (0) is different than expected (-22) >not ok 21 Shadow stack on system without shadow stack ># Totals: pass:19 fail:1 xfail:0 xpass:0 skip:1 error:0 > >The other tests passed in both cases. I'm going to dig into the other >parts now but can circle back if it's not obvious what's going on >there.
On Fri, Nov 17, 2023 at 01:12:46PM -0800, Deepak Gupta wrote: > On Tue, Nov 14, 2023 at 11:11:58PM +0000, Edgecombe, Rick P wrote: > > It seems like there will be a need for some generic method of checking > > if shadow stack is enabled. Maybe a more generic compiler > > intrinsic/builtin or glibc API (something unrelated to SSP)? > Exposing a new file under procfs would be useful? > Something like "/proc/sys/vm/user_shadow_stack_supported" > `map_shadow_stack` can return MAP_FAILED for other reasons. > I think `kselftests` are fine but I don't want people to pick up this > as test code and run with it in production :-) > So kernel providing a way to indicate whether it supports shadow stack > mappings in user mode via procfs would be useful and arch agnostic. I can see that might be useful for some higher level code that wants to tune the size and nothing else. I'd be tempted to do it through adding a tuneable for the maximum default shadow stack size (x86 currently uses 4G) just so it's *vaguely* useful rather than just a file. I question the utility of that but just a plain file doesn't feel quite idiomatic. In any case it feels like it's off topic for this series and should be done separately.
diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c index 6adbfd14c841..10e0487c402a 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,9 @@ #include "../kselftest.h" #include "clone3_selftests.h" +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 +32,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 +49,27 @@ struct test { filter_function filter; }; +#ifndef __NR_map_shadow_stack +#define __NR_map_shadow_stack 453 +#endif + +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; + + 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 +115,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 +208,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,6 +371,24 @@ static const struct test tests[] = { .expected = -EINVAL, .test_mode = CLONE3_ARGS_NO_TEST, }, + { + .name = "Shadow stack on system with shadow stack", + .flags = 0, + .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 = 0, + .size = 0, + .expected = -EINVAL, + .e2big_valid = true, + .test_mode = CLONE3_ARGS_SHADOW_STACK, + .filter = have_shadow_stack, + }, }; int main(int argc, char *argv[]) @@ -332,6 +399,7 @@ int main(int argc, char *argv[]) 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. We detect support for shadow stacks on the running system by attempting to allocate a shadow stack page during initialisation using map_shadow_stack(). Signed-off-by: Mark Brown <broonie@kernel.org> --- tools/testing/selftests/clone3/clone3.c | 68 +++++++++++++++++++++++ tools/testing/selftests/clone3/clone3_selftests.h | 7 +++ 2 files changed, 75 insertions(+)