Add some tests to cover the new PR_SET_MDWE prctl.
Signed-off-by: Joey Gouly <[email protected]>
Cc: Shuah Khan <[email protected]>
---
tools/testing/selftests/vm/mdwe_test.c | 194 +++++++++++++++++++++++++
1 file changed, 194 insertions(+)
create mode 100644 tools/testing/selftests/vm/mdwe_test.c
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;
+}
+
--
2.17.1
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 <[email protected]>
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! :)
--
Kees Cook
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. :)
--
Kees Cook
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Joey Gouly <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
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
--
2.34.1
--
Kees Cook
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%[email protected]/
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 <[email protected]>
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...