diff mbox series

[RFC,RFT,v2,5/5] kselftest/clone3: Test shadow stack support

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

Commit Message

Mark Brown Nov. 14, 2023, 8:05 p.m. UTC
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(+)

Comments

Edgecombe, Rick P Nov. 14, 2023, 11:11 p.m. UTC | #1
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.
Mark Brown Nov. 15, 2023, 12:53 p.m. UTC | #2
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.
Edgecombe, Rick P Nov. 17, 2023, 6:16 p.m. UTC | #3
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.
Deepak Gupta Nov. 17, 2023, 9:12 p.m. UTC | #4
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.
Mark Brown Nov. 20, 2023, 3:47 p.m. UTC | #5
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 mbox series

Patch

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)