diff mbox series

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

Message ID 20231128-clone3-shadow-stack-v4-5-8b28ffe4f676@kernel.org (mailing list archive)
State New
Headers show
Series fork: Support shadow stacks in clone3() | expand

Commit Message

Mark Brown Nov. 28, 2023, 6:22 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.

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(+)

Comments

Edgecombe, Rick P Dec. 5, 2023, 12:10 a.m. UTC | #1
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]);
Mark Brown Dec. 5, 2023, 3:05 p.m. UTC | #2
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
Edgecombe, Rick P Dec. 5, 2023, 4:01 p.m. UTC | #3
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?
Mark Brown Dec. 5, 2023, 4:43 p.m. UTC | #4
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.
Edgecombe, Rick P Dec. 5, 2023, 10:31 p.m. UTC | #5
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.
Mark Brown Dec. 6, 2023, 6:42 p.m. UTC | #6
On Tue, Dec 05, 2023 at 10:31:09PM +0000, Edgecombe, Rick P wrote:
> On Tue, 2023-12-05 at 16:43 +0000, Mark Brown wrote:

> > 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?

Probably good to write it up somewhere, yes - it'd also be useful for
anyone off doing their own non-libc things.  It did cross my mind to
try to make a document for the generic bit of the ABI for shadow stacks.

> 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.

I did think about that but was worried that a header might encourage
more users doing the hacky thing.  OTOH it would mean the arch specific
tests could share the header though so perhaps you're right, I'll take a
look.
diff mbox series

Patch

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)