| Message ID | 20221026150457.36957-3-joey.gouly@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: In-kernel support for memory-deny-write-execute (MDWE) | expand |
On Wed, Oct 26, 2022 at 04:04:57PM +0100, Joey Gouly wrote: > Add some tests to cover the new PR_SET_MDWE prctl. Some comments below but they're all stylistic and let's not make perfect be the enemy of the good here so Reviewed-by: Mark Brown <broonie@kernel.org> and we can iterate later rather than blocking anything on the testcase. > +#ifdef __aarch64__ > +#define PROT_BTI 0x10 /* BTI guarded page */ > +#endif We should get this from the kernel headers shouldn't we? We generally rely on things getting pulled in from there rather than locally defining. > +#define TEST1 "mmap(PROT_WRITE | PROT_EXEC)\n" > +#define TEST2 "mmap(PROT_WRITE); mprotect(PROT_EXEC)\n" > +#define TEST3 "mmap(PROT_EXEC); mprotect(PROT_EXEC | PROT_READ)\n" > +#define TEST4 "mmap(PROT_EXEC); mprotect(PROT_EXEC | PROT_BTI)\n" > +int test1(int mdwe_enabled) > +{ It feels like we could usefully make an array of struct test { int (*run)(bool mdwe_enabled); char *name; } then we'd need fewer ifdefs, things could be more usefully named and it'd be a bit easier to add new cases. > +#ifdef __aarch64__ > + ksft_set_plan(12); > +#else > + ksft_set_plan(9); > +#endif That'd just be ksft_test_plan(3 * ARRAY_SIZE(tests). > + // First run the tests without MDWE > + test_result(test1(0), TEST1); > + test_result(test2(0), TEST2); > + test_result(test3(0), TEST3); > +#ifdef __aarch64__ > + test_result(test4(0), TEST4); > +#endif and these calls to the tests would all be iterating over the array.
On Wed, Oct 26, 2022 at 04:04:57PM +0100, Joey Gouly wrote:
> +#include "../kselftest.h"
I recommend using kselftest_harness.h instead; it provides much of the
infrastructure that is open-coded here. But yes, testing is good; thank
you! :)
Here's an alternative rewritten to use kselftest_harness.h, with tests for the prctl() flags, and the missed Makefile addition. This should be much easier to add more variants and tests to, I hope. -Kees From bc442a99ebd9852bfaa7444b521bd55fdbb4d369 Mon Sep 17 00:00:00 2001 From: Kees Cook <keescook@chromium.org> Date: Fri, 28 Oct 2022 13:10:45 -0700 Subject: [PATCH] selftests/vm: add tests for memory-deny-write-execute Add tests for new prctl() commands, including flag values. Add tests for new denials based on PROT_EXEC across mmap() and mprotect() with MDWE. Co-developed-by: Joey Gouly <joey.gouly@arm.com> Signed-off-by: Joey Gouly <joey.gouly@arm.com> Signed-off-by: Kees Cook <keescook@chromium.org> --- tools/testing/selftests/vm/Makefile | 1 + tools/testing/selftests/vm/mdwe_test.c | 201 +++++++++++++++++++++++++ 2 files changed, 202 insertions(+) create mode 100644 tools/testing/selftests/vm/mdwe_test.c diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile index 163c2fde3cb3..8dd4d4910fa5 100644 --- a/tools/testing/selftests/vm/Makefile +++ b/tools/testing/selftests/vm/Makefile @@ -52,6 +52,7 @@ TEST_GEN_FILES += userfaultfd TEST_GEN_PROGS += soft-dirty TEST_GEN_PROGS += split_huge_page_test TEST_GEN_FILES += ksm_tests +TEST_GEN_PROGS += mdwe_test ifeq ($(MACHINE),x86_64) CAN_BUILD_I386 := $(shell ./../x86/check_cc.sh "$(CC)" ../x86/trivial_32bit_program.c -m32) diff --git a/tools/testing/selftests/vm/mdwe_test.c b/tools/testing/selftests/vm/mdwe_test.c new file mode 100644 index 000000000000..d6f6b751bcd6 --- /dev/null +++ b/tools/testing/selftests/vm/mdwe_test.c @@ -0,0 +1,201 @@ +// SPDX-License-Identifier: GPL-2.0 +#ifdef __aarch64__ +#include <asm/hwcap.h> +#endif +#include <stdio.h> +#include <stdlib.h> +#include <sys/auxv.h> +#include <sys/mman.h> +#include <sys/prctl.h> +#include <sys/wait.h> +#include <unistd.h> + +#include <linux/prctl.h> + +#include "../kselftest_harness.h" + +#define PR_SET_MDWE 65 +# define PR_MDWE_FLAG_MMAP 1 + +#define PR_GET_MDWE 66 + +#ifdef __aarch64__ +# define PROT_BTI 0x10 /* BTI guarded page */ +#else +# define PROT_BTI 0 +#endif + +TEST(prctl_flags) +{ + EXPECT_LT(prctl(PR_SET_MDWE, 7, 0, 0, 0), 0); + EXPECT_LT(prctl(PR_SET_MDWE, 0, 7, 0, 0), 0); + EXPECT_LT(prctl(PR_SET_MDWE, 0, 0, 7, 0), 0); + EXPECT_LT(prctl(PR_SET_MDWE, 0, 0, 0, 7), 0); + + EXPECT_LT(prctl(PR_GET_MDWE, 7, 0, 0, 0), 0); + EXPECT_LT(prctl(PR_GET_MDWE, 0, 7, 0, 0), 0); + EXPECT_LT(prctl(PR_GET_MDWE, 0, 0, 7, 0), 0); + EXPECT_LT(prctl(PR_GET_MDWE, 0, 0, 0, 7), 0); +} + +FIXTURE(mdwe) +{ + void *p; + int flags; + size_t size; + pid_t pid; +}; + +FIXTURE_VARIANT(mdwe) +{ + bool enabled; + bool forked; +}; + +FIXTURE_VARIANT_ADD(mdwe, stock) +{ + .enabled = false, + .forked = false, +}; + +FIXTURE_VARIANT_ADD(mdwe, enabled) +{ + .enabled = true, + .forked = false, +}; + +FIXTURE_VARIANT_ADD(mdwe, forked) +{ + .enabled = true, + .forked = true, +}; + +FIXTURE_SETUP(mdwe) +{ + int ret, status; + + self->p = NULL; + self->flags = MAP_SHARED | MAP_ANONYMOUS; + self->size = getpagesize(); + + if (!variant->enabled) + return; + + ret = prctl(PR_SET_MDWE, PR_MDWE_FLAG_MMAP, 0, 0, 0); + ASSERT_EQ(ret, 0) { + TH_LOG("PR_SET_MDWE failed or unsupported"); + } + + ret = prctl(PR_GET_MDWE, 0, 0, 0, 0); + ASSERT_EQ(ret, 1); + + if (variant->forked) { + self->pid = fork(); + ASSERT_GE(self->pid, 0) { + TH_LOG("fork failed\n"); + } + + if (self->pid > 0) { + ret = waitpid(self->pid, &status, 0); + ASSERT_TRUE(WIFEXITED(status)); + exit(WEXITSTATUS(status)); + } + } +} + +FIXTURE_TEARDOWN(mdwe) +{ + if (self->p && self->p != MAP_FAILED) + munmap(self->p, self->size); +} + +TEST_F(mdwe, mmap_READ_EXEC) +{ + self->p = mmap(NULL, self->size, PROT_READ | PROT_EXEC, self->flags, 0, 0); + EXPECT_NE(self->p, MAP_FAILED); +} + +TEST_F(mdwe, mmap_WRITE_EXEC) +{ + self->p = mmap(NULL, self->size, PROT_WRITE | PROT_EXEC, self->flags, 0, 0); + if (variant->enabled) { + EXPECT_EQ(self->p, MAP_FAILED); + } else { + EXPECT_NE(self->p, MAP_FAILED); + } +} + +TEST_F(mdwe, mprotect_stay_EXEC) +{ + int ret; + + self->p = mmap(NULL, self->size, PROT_READ | PROT_EXEC, self->flags, 0, 0); + ASSERT_NE(self->p, MAP_FAILED); + + ret = mprotect(self->p, self->size, PROT_READ | PROT_EXEC); + EXPECT_EQ(ret, 0); +} + +TEST_F(mdwe, mprotect_add_EXEC) +{ + int ret; + + self->p = mmap(NULL, self->size, PROT_READ, self->flags, 0, 0); + ASSERT_NE(self->p, MAP_FAILED); + + ret = mprotect(self->p, self->size, PROT_READ | PROT_EXEC); + if (variant->enabled) { + EXPECT_LT(ret, 0); + } else { + EXPECT_EQ(ret, 0); + } +} + +TEST_F(mdwe, mprotect_WRITE_EXEC) +{ + int ret; + + self->p = mmap(NULL, self->size, PROT_WRITE, self->flags, 0, 0); + ASSERT_NE(self->p, MAP_FAILED); + + ret = mprotect(self->p, self->size, PROT_WRITE | PROT_EXEC); + if (variant->enabled) { + EXPECT_LT(ret, 0); + } else { + EXPECT_EQ(ret, 0); + } +} + +TEST_F(mdwe, mmap_FIXED) +{ + void *p; + + self->p = mmap(NULL, self->size, PROT_READ, self->flags, 0, 0); + ASSERT_NE(self->p, MAP_FAILED); + + p = mmap(self->p, self->size, PROT_READ | PROT_EXEC, + self->flags | MAP_FIXED, 0, 0); + if (variant->enabled) { + EXPECT_EQ(p, MAP_FAILED); + } else { + EXPECT_EQ(p, self->p); + } +} + +TEST_F(mdwe, arm64_BTI) +{ + int ret; + +#ifdef __aarch64__ + if (!(getauxval(AT_HWCAP2) & HWCAP2_BTI)) +#endif + SKIP(return, "HWCAP2_BTI not supported"); + + self->p = mmap(NULL, self->size, PROT_EXEC, self->flags, 0, 0); + ASSERT_NE(self->p, MAP_FAILED); + + ret = mprotect(self->p, self->size, PROT_EXEC | PROT_BTI); + EXPECT_EQ(ret, 0); +} + +TEST_HARNESS_MAIN
On Wed, Oct 26, 2022 at 04:04:57PM +0100, Joey Gouly wrote: > [...] > +# define PR_MDWE_FLAG_MMAP 1 > [...] > + // Enable MDWE and then run the tests again. > + ret = prctl(PR_SET_MDWE, PR_MDWE_FLAG_MMAP, 0, 0, 0); > + if (ret < 0) { > + ksft_print_msg("PR_SET_MDWE failed or unsupported!\n"); > + goto exit; > + } > + > + ret = prctl(PR_GET_MDWE, PR_MDWE_FLAG_MMAP, 0, 0, 0); > + if (ret == 0) > + ksft_exit_fail_msg("PR_GET_MDWE failed!"); This flag (PR_MDWE_FLAG_MMAP), while defined in uapi, wasn't actually being used in the proposed prctl() api. :)
The 10/28/2022 13:16, Kees Cook wrote: > +++ b/tools/testing/selftests/vm/mdwe_test.c > @@ -0,0 +1,201 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#ifdef __aarch64__ > +#include <asm/hwcap.h> > +#endif > +#include <stdio.h> > +#include <stdlib.h> > +#include <sys/auxv.h> > +#include <sys/mman.h> > +#include <sys/prctl.h> > +#include <sys/wait.h> > +#include <unistd.h> > + > +#include <linux/prctl.h> > + > +#include "../kselftest_harness.h" > + > +#define PR_SET_MDWE 65 > +# define PR_MDWE_FLAG_MMAP 1 > + > +#define PR_GET_MDWE 66 > + > +#ifdef __aarch64__ > +# define PROT_BTI 0x10 /* BTI guarded page */ > +#else > +# define PROT_BTI 0 > +#endif > + > +TEST(prctl_flags) > +{ > + EXPECT_LT(prctl(PR_SET_MDWE, 7, 0, 0, 0), 0); > + EXPECT_LT(prctl(PR_SET_MDWE, 0, 7, 0, 0), 0); > + EXPECT_LT(prctl(PR_SET_MDWE, 0, 0, 7, 0), 0); > + EXPECT_LT(prctl(PR_SET_MDWE, 0, 0, 0, 7), 0); note that prctl is declared as int prctl(int, ...); and all 4 arguments are documented to be unsigned long in the linux man pages (even though some are pointers: this is already a problem for the libc as it does not know if it should use va_arg(ap, unsigned long) or va_arg(ap, void *), in practice the call abi rules are the same for those on linux, so either works unless the compiler deliberately breaks the code due to the type mismatch ub). passing an int where an unsigned long is needed is wrong: it breaks va_arg rules on the c language level (posix rules too) but more importantly it breaks abi rules: on most LP64 abis it is not required to be signextended so arbitrary top 32bits may be passed down. so e.g. prctl(option, 0, 0, 0, 0); should be written as prctl(option, 0L, 0L, 0L, 0L); or similar (int signedness does not matter according to c rules), otherwise non-zero top bits may be passed that the kernel has to ignore, which it currently does not always do. ideally the kernel updated all the prctl arg macros to have type long or unsigned long. or explicitly masked out the top bits when it only uses an int. see my related rant at https://lore.kernel.org/linux-api/Y1%2FDS6uoWP7OSkmd@arm.com/
Hi, On Fri, Oct 28, 2022 at 06:03:18PM +0100, Mark Brown wrote: > On Wed, Oct 26, 2022 at 04:04:57PM +0100, Joey Gouly wrote: > > > Add some tests to cover the new PR_SET_MDWE prctl. > > Some comments below but they're all stylistic and let's not make perfect > be the enemy of the good here so > > Reviewed-by: Mark Brown <broonie@kernel.org> Thanks for the review, however I won't keep your R-b tag because I'm going to move forward with Kees' approach from: https://lore.kernel.org/linux-arm-kernel/202210281314.C5D3414722@keescook/T/#m45ac9de6c205b560d072a65e4e67e2a7ee363588 Thanks to Kees for rewriting that. > > and we can iterate later rather than blocking anything on the testcase. > > > +#ifdef __aarch64__ > > +#define PROT_BTI 0x10 /* BTI guarded page */ > > +#endif > > We should get this from the kernel headers shouldn't we? We generally > rely on things getting pulled in from there rather than locally > defining. I believe the mman.h included is from the toolchain, not the kernel's uapi headers. The toolchain I was using didn't have PROT_BTI defined in its mman.h > > > +#define TEST1 "mmap(PROT_WRITE | PROT_EXEC)\n" > > +#define TEST2 "mmap(PROT_WRITE); mprotect(PROT_EXEC)\n" > > +#define TEST3 "mmap(PROT_EXEC); mprotect(PROT_EXEC | PROT_READ)\n" > > +#define TEST4 "mmap(PROT_EXEC); mprotect(PROT_EXEC | PROT_BTI)\n" > > > +int test1(int mdwe_enabled) > > +{ > > It feels like we could usefully make an array of > > struct test { > int (*run)(bool mdwe_enabled); > char *name; > } > > then we'd need fewer ifdefs, things could be more usefully named and > it'd be a bit easier to add new cases. > > > +#ifdef __aarch64__ > > + ksft_set_plan(12); > > +#else > > + ksft_set_plan(9); > > +#endif > > That'd just be ksft_test_plan(3 * ARRAY_SIZE(tests). > > > + // First run the tests without MDWE > > + test_result(test1(0), TEST1); > > + test_result(test2(0), TEST2); > > + test_result(test3(0), TEST3); > > +#ifdef __aarch64__ > > + test_result(test4(0), TEST4); > > +#endif > > and these calls to the tests would all be iterating over the array. These comments are solved by the kselftest_harness approach that Kees suggested. Thanks, Joey
On Tue, Nov 08, 2022 at 05:33:03PM +0000, Joey Gouly wrote: > On Fri, Oct 28, 2022 at 06:03:18PM +0100, Mark Brown wrote: > > On Wed, Oct 26, 2022 at 04:04:57PM +0100, Joey Gouly wrote: > > > +#ifdef __aarch64__ > > > +#define PROT_BTI 0x10 /* BTI guarded page */ > > > +#endif > > We should get this from the kernel headers shouldn't we? We generally > > rely on things getting pulled in from there rather than locally > > defining. > I believe the mman.h included is from the toolchain, not the kernel's uapi headers. > The toolchain I was using didn't have PROT_BTI defined in its mman.h I'd expect that whatever we're doing in the build process ought to be overriding the default headers provided by the toolchain, that's kind of the point here...
diff --git a/tools/testing/selftests/vm/mdwe_test.c b/tools/testing/selftests/vm/mdwe_test.c new file mode 100644 index 000000000000..67f3fc06d069 --- /dev/null +++ b/tools/testing/selftests/vm/mdwe_test.c @@ -0,0 +1,194 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <asm/hwcap.h> +#include <stdio.h> +#include <stdlib.h> +#include <sys/auxv.h> +#include <sys/mman.h> +#include <sys/prctl.h> +#include <sys/wait.h> +#include <unistd.h> + +#include <linux/prctl.h> + +#include "../kselftest.h" + +#define PR_SET_MDWE 65 +# define PR_MDWE_FLAG_MMAP 1 + +#define PR_GET_MDWE 66 + +#ifdef __aarch64__ +#define PROT_BTI 0x10 /* BTI guarded page */ +#endif + +#define TEST1 "mmap(PROT_WRITE | PROT_EXEC)\n" +#define TEST2 "mmap(PROT_WRITE); mprotect(PROT_EXEC)\n" +#define TEST3 "mmap(PROT_EXEC); mprotect(PROT_EXEC | PROT_READ)\n" +#define TEST4 "mmap(PROT_EXEC); mprotect(PROT_EXEC | PROT_BTI)\n" + +int fork_test(int (*func)(int)) +{ + pid_t pid; + int status; + + pid = fork(); + if (pid < 0) { + printf("fork failed\n"); + return KSFT_FAIL; + } + + if (pid == 0) + exit(func(1)); + + waitpid(pid, &status, 0); + + if (WIFEXITED(status)) + return WEXITSTATUS(status); + + return 0; +} + +static inline void test_result(int err, const char *msg) +{ + switch (err) { + case KSFT_PASS: + ksft_test_result_pass(msg); + break; + case KSFT_FAIL: + ksft_test_result_fail(msg); + break; + case KSFT_SKIP: + ksft_test_result_skip(msg); + break; + default: + ksft_test_result_error("Unknown return code %d from %s", + err, msg); + break; + } +} + +int test1(int mdwe_enabled) +{ + void *p; + + int size = getpagesize(); + int mmap_flags = MAP_SHARED | MAP_ANONYMOUS; + + p = mmap(0, size, PROT_WRITE | PROT_EXEC, mmap_flags, 0, 0); + + if (mdwe_enabled) + return p == MAP_FAILED ? KSFT_PASS : KSFT_FAIL; + else + return p != MAP_FAILED ? KSFT_PASS : KSFT_FAIL; +} + +int test2(int mdwe_enabled) +{ + void *p; + int ret; + + int size = getpagesize(); + int mmap_flags = MAP_SHARED | MAP_ANONYMOUS; + + p = mmap(0, size, PROT_WRITE, mmap_flags, 0, 0); + if (p == MAP_FAILED) + return 0; + ret = mprotect(p, size, PROT_EXEC); + + if (mdwe_enabled) + return ret < 0 ? KSFT_PASS : KSFT_FAIL; + else + return ret == 0 ? KSFT_PASS : KSFT_FAIL; +} + +int test3(int mdwe_enabled) +{ + void *p; + int ret; + + int size = getpagesize(); + int mmap_flags = MAP_SHARED | MAP_ANONYMOUS; + + p = mmap(0, size, PROT_EXEC, mmap_flags, 0, 0); + if (p == MAP_FAILED) + return 0; + + ret = mprotect(p, size, PROT_EXEC | PROT_READ); + + return ret == 0 ? KSFT_PASS : KSFT_FAIL; +} + +#ifdef __aarch64__ +int test4(int mdwe_enabled) +{ + void *p; + int ret; + + int size = getpagesize(); + int mmap_flags = MAP_SHARED | MAP_ANONYMOUS; + + if (!(getauxval(AT_HWCAP2) & HWCAP2_BTI)) + return KSFT_SKIP; + + p = mmap(0, size, PROT_EXEC, mmap_flags, 0, 0); + if (p == MAP_FAILED) + return KSFT_FAIL; + + ret = mprotect(p, size, PROT_EXEC | PROT_BTI); + + return ret == 0 ? KSFT_PASS : KSFT_FAIL; +} +#endif + +int main(void) +{ + int ret; + + ksft_print_header(); +#ifdef __aarch64__ + ksft_set_plan(12); +#else + ksft_set_plan(9); +#endif + + // First run the tests without MDWE + test_result(test1(0), TEST1); + test_result(test2(0), TEST2); + test_result(test3(0), TEST3); +#ifdef __aarch64__ + test_result(test4(0), TEST4); +#endif + + // Enable MDWE and then run the tests again. + ret = prctl(PR_SET_MDWE, PR_MDWE_FLAG_MMAP, 0, 0, 0); + if (ret < 0) { + ksft_print_msg("PR_SET_MDWE failed or unsupported!\n"); + goto exit; + } + + ret = prctl(PR_GET_MDWE, PR_MDWE_FLAG_MMAP, 0, 0, 0); + if (ret == 0) + ksft_exit_fail_msg("PR_GET_MDWE failed!"); + + test_result(test1(1), "MDWE: " TEST1); + test_result(test2(1), "MDWE: " TEST2); + test_result(test3(1), "MDWE: " TEST3); +#ifdef __aarch64__ + test_result(test4(1), "MDWE: " TEST4); +#endif + + // Verify the MDWE setting is transferred when fork()ing + test_result(fork_test(test1), "MDWE+fork: " TEST1); + test_result(fork_test(test2), "MDWE+fork: " TEST2); + test_result(fork_test(test3), "MDWE+fork: " TEST3); +#ifdef __aarch64__ + test_result(fork_test(test4), "MDWE+fork: " TEST4); +#endif + +exit: + ksft_finished(); + + return 0; +} +
Add some tests to cover the new PR_SET_MDWE prctl. Signed-off-by: Joey Gouly <joey.gouly@arm.com> Cc: Shuah Khan <shuah@kernel.org> --- tools/testing/selftests/vm/mdwe_test.c | 194 +++++++++++++++++++++++++ 1 file changed, 194 insertions(+) create mode 100644 tools/testing/selftests/vm/mdwe_test.c