[PATCH v3 0/5] MDWE without inheritance

[PATCH v3 0/5] MDWE without inheritance

[Florent Revest]

Joey recently introduced a Memory-Deny-Write-Executable (MDWE) prctl which tags
current with a flag that prevents pages that were previously not executable from
becoming executable.
This tag always gets inherited by children tasks. (it's in MMF_INIT_MASK)

At Google, we've been using a somewhat similar downstream patch for a few years
now. To make the adoption of this feature easier, we've had it support a mode in
which the W^X flag does not propagate to children. For example, this is handy if
a C process which wants W^X protection suspects it could start children
processes that would use a JIT.

I'd like to align our features with the upstream prctl. This series proposes a
new NO_INHERIT flag to the MDWE prctl to make this kind of adoption easier. It
sets a different flag in current that is not in MMF_INIT_MASK and which does not
propagate.

As part of looking into MDWE, I also fixed a couple of things in the MDWE test.

This series applies on the mm-everything-2023-05-16-23-30 tag of the mm tree:
  https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/

Diff since v2:
- Turned the MMF_INIT_FLAGS macro into a mmf_init_flags function as suggested by
  David Hildenbrand
- Removed the ability to transition from to PR_MDWE_REFUSE_EXEC_GAIN from
  (PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT) which also significantly
  simplifies the prctl_set_mdwe logic
- Cc-ed -stable on patch 3 as suggested by Alexey Izbyshev
- Added a handful of Reviewed-by/Acked-by trailers

Diff since v1:
- MMF_HAS_MDWE_NO_INHERIT clears MMF_HAS_MDWE in the fork path as part of a
  MMF_INIT_FLAGS macro (suggested by Catalin)
- PR_MDWE_* are defined as unsigned long rather than int (suggested by Andrey)

Florent Revest (5):
  kselftest: vm: Fix tabs/spaces inconsistency in the mdwe test
  kselftest: vm: Fix mdwe's mmap_FIXED test case
  mm: Make PR_MDWE_REFUSE_EXEC_GAIN an unsigned long
  mm: Add a NO_INHERIT flag to the PR_SET_MDWE prctl
  kselftest: vm: Add tests for no-inherit memory-deny-write-execute

 include/linux/sched/coredump.h         |  10 +++
 include/uapi/linux/prctl.h             |   3 +-
 kernel/fork.c                          |   2 +-
 kernel/sys.c                           |  32 +++++--
 tools/include/uapi/linux/prctl.h       |   3 +-
 tools/testing/selftests/mm/mdwe_test.c | 113 +++++++++++++++++++++----
 6 files changed, 139 insertions(+), 24 deletions(-)

-- 
2.41.0.255.g8b1d071c50-goog

[PATCH v3 1/5] kselftest: vm: Fix tabs/spaces inconsistency in the mdwe test

[Florent Revest]

Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Florent Revest <revest@chromium.org>
---
 tools/testing/selftests/mm/mdwe_test.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/mm/mdwe_test.c b/tools/testing/selftests/mm/mdwe_test.c
index bc91bef5d254..d0954c657feb 100644
--- a/tools/testing/selftests/mm/mdwe_test.c
+++ b/tools/testing/selftests/mm/mdwe_test.c
@@ -49,19 +49,19 @@ FIXTURE_VARIANT(mdwe)
 
 FIXTURE_VARIANT_ADD(mdwe, stock)
 {
-        .enabled = false,
+	.enabled = false,
 	.forked = false,
 };
 
 FIXTURE_VARIANT_ADD(mdwe, enabled)
 {
-        .enabled = true,
+	.enabled = true,
 	.forked = false,
 };
 
 FIXTURE_VARIANT_ADD(mdwe, forked)
 {
-        .enabled = true,
+	.enabled = true,
 	.forked = true,
 };
 
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH v3 2/5] kselftest: vm: Fix mdwe's mmap_FIXED test case

[Florent Revest]

I checked with the original author, the mmap_FIXED test case wasn't
properly tested and fails. Currently, it maps two consecutive (non
overlapping) pages and expects the second mapping to be denied by MDWE
but these two pages have nothing to do with each other so MDWE is
actually out of the picture here.

What the test actually intended to do was to remap a virtual address
using MAP_FIXED. However, this operation unmaps the existing mapping and
creates a new one so the va is backed by a new page and MDWE is again
out of the picture, all remappings should succeed.

This patch keeps the test case to make it clear that this situation is
expected to work.

Signed-off-by: Florent Revest <revest@chromium.org>
Reviewed-by: David Hildenbrand <david@redhat.com>
Fixes: 4cf1fe34fd18 ("kselftest: vm: add tests for memory-deny-write-execute")
---
 tools/testing/selftests/mm/mdwe_test.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/mm/mdwe_test.c b/tools/testing/selftests/mm/mdwe_test.c
index d0954c657feb..91aa9c3099e7 100644
--- a/tools/testing/selftests/mm/mdwe_test.c
+++ b/tools/testing/selftests/mm/mdwe_test.c
@@ -168,13 +168,10 @@ TEST_F(mdwe, mmap_FIXED)
 	self->p = mmap(NULL, self->size, PROT_READ, self->flags, 0, 0);
 	ASSERT_NE(self->p, MAP_FAILED);
 
-	p = mmap(self->p + self->size, self->size, PROT_READ | PROT_EXEC,
+	/* MAP_FIXED unmaps the existing page before mapping which is allowed */
+	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);
-	}
+	EXPECT_EQ(p, self->p);
 }
 
 TEST_F(mdwe, arm64_BTI)
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH v3 3/5] mm: Make PR_MDWE_REFUSE_EXEC_GAIN an unsigned long

[Florent Revest]

Defining a prctl flag as an int is a footgun because on a 64 bit machine
and with a variadic implementation of prctl (like in musl and glibc),
when used directly as a prctl argument, it can get casted to long with
garbage upper bits which would result in unexpected behaviors.

This patch changes the constant to an unsigned long to eliminate that
possibilities. This does not break UAPI.

Fixes: b507808ebce2 ("mm: implement memory-deny-write-execute as a prctl")
Cc: linux-stable@vger.kernel.org
Signed-off-by: Florent Revest <revest@chromium.org>
Suggested-by: Alexey Izbyshev <izbyshev@ispras.ru>
Reviewed-by: David Hildenbrand <david@redhat.com>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
---
 include/uapi/linux/prctl.h       | 2 +-
 tools/include/uapi/linux/prctl.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index f23d9a16507f..6e9af6cbc950 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -283,7 +283,7 @@ struct prctl_mm_map {
 
 /* Memory deny write / execute */
 #define PR_SET_MDWE			65
-# define PR_MDWE_REFUSE_EXEC_GAIN	1
+# define PR_MDWE_REFUSE_EXEC_GAIN	(1UL << 0)
 
 #define PR_GET_MDWE			66
 
diff --git a/tools/include/uapi/linux/prctl.h b/tools/include/uapi/linux/prctl.h
index f23d9a16507f..6e9af6cbc950 100644
--- a/tools/include/uapi/linux/prctl.h
+++ b/tools/include/uapi/linux/prctl.h
@@ -283,7 +283,7 @@ struct prctl_mm_map {
 
 /* Memory deny write / execute */
 #define PR_SET_MDWE			65
-# define PR_MDWE_REFUSE_EXEC_GAIN	1
+# define PR_MDWE_REFUSE_EXEC_GAIN	(1UL << 0)
 
 #define PR_GET_MDWE			66
 
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH v3 4/5] mm: Add a NO_INHERIT flag to the PR_SET_MDWE prctl

[Florent Revest]

This extends the current PR_SET_MDWE prctl arg with a bit to indicate
that the process doesn't want MDWE protection to propagate to children.

To implement this no-inherit mode, the tag in current->mm->flags must be
absent from MMF_INIT_MASK. This means that the encoding for "MDWE but
without inherit" is different in the prctl than in the mm flags. This
leads to a bit of bit-mangling in the prctl implementation.

Signed-off-by: Florent Revest <revest@chromium.org>
---
 include/linux/sched/coredump.h   | 10 ++++++++++
 include/uapi/linux/prctl.h       |  1 +
 kernel/fork.c                    |  2 +-
 kernel/sys.c                     | 32 ++++++++++++++++++++++++++------
 tools/include/uapi/linux/prctl.h |  1 +
 5 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/include/linux/sched/coredump.h b/include/linux/sched/coredump.h
index 0ee96ea7a0e9..1b37fa8fc723 100644
--- a/include/linux/sched/coredump.h
+++ b/include/linux/sched/coredump.h
@@ -91,4 +91,14 @@ static inline int get_dumpable(struct mm_struct *mm)
 				 MMF_DISABLE_THP_MASK | MMF_HAS_MDWE_MASK)
 
 #define MMF_VM_MERGE_ANY	29
+#define MMF_HAS_MDWE_NO_INHERIT	30
+
+static inline unsigned long mmf_init_flags(unsigned long flags)
+{
+	if (flags & (1UL << MMF_HAS_MDWE_NO_INHERIT))
+		flags &= ~((1UL << MMF_HAS_MDWE) |
+			   (1UL << MMF_HAS_MDWE_NO_INHERIT));
+	return flags & MMF_INIT_MASK;
+}
+
 #endif /* _LINUX_SCHED_COREDUMP_H */
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index 6e9af6cbc950..dacbe824e7c3 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -284,6 +284,7 @@ struct prctl_mm_map {
 /* Memory deny write / execute */
 #define PR_SET_MDWE			65
 # define PR_MDWE_REFUSE_EXEC_GAIN	(1UL << 0)
+# define PR_MDWE_NO_INHERIT		(1UL << 1)
 
 #define PR_GET_MDWE			66
 
diff --git a/kernel/fork.c b/kernel/fork.c
index d17995934eb4..bc3c762d378f 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1284,7 +1284,7 @@ static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p,
 	hugetlb_count_init(mm);
 
 	if (current->mm) {
-		mm->flags = current->mm->flags & MMF_INIT_MASK;
+		mm->flags = mmf_init_flags(current->mm->flags);
 		mm->def_flags = current->mm->def_flags & VM_INIT_DEF_MASK;
 	} else {
 		mm->flags = default_dump_filter;
diff --git a/kernel/sys.c b/kernel/sys.c
index 339fee3eff6a..1a2dc3da43ea 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2362,19 +2362,41 @@ static int prctl_set_vma(unsigned long opt, unsigned long start,
 }
 #endif /* CONFIG_ANON_VMA_NAME */
 
+static inline unsigned long get_current_mdwe(void)
+{
+	unsigned long ret = 0;
+
+	if (test_bit(MMF_HAS_MDWE, &current->mm->flags))
+		ret |= PR_MDWE_REFUSE_EXEC_GAIN;
+	if (test_bit(MMF_HAS_MDWE_NO_INHERIT, &current->mm->flags))
+		ret |= PR_MDWE_NO_INHERIT;
+
+	return ret;
+}
+
 static inline int prctl_set_mdwe(unsigned long bits, unsigned long arg3,
 				 unsigned long arg4, unsigned long arg5)
 {
+	unsigned long current_bits;
+
 	if (arg3 || arg4 || arg5)
 		return -EINVAL;
 
-	if (bits & ~(PR_MDWE_REFUSE_EXEC_GAIN))
+	if (bits & ~(PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT))
+		return -EINVAL;
+
+	/* NO_INHERIT only makes sense with REFUSE_EXEC_GAIN */
+	if (bits & PR_MDWE_NO_INHERIT && !(bits & PR_MDWE_REFUSE_EXEC_GAIN))
 		return -EINVAL;
 
+	current_bits = get_current_mdwe();
+	if (current_bits && current_bits != bits)
+		return -EPERM; /* Cannot unset the flags */
+
+	if (bits & PR_MDWE_NO_INHERIT)
+		set_bit(MMF_HAS_MDWE_NO_INHERIT, &current->mm->flags);
 	if (bits & PR_MDWE_REFUSE_EXEC_GAIN)
 		set_bit(MMF_HAS_MDWE, &current->mm->flags);
-	else if (test_bit(MMF_HAS_MDWE, &current->mm->flags))
-		return -EPERM; /* Cannot unset the flag */
 
 	return 0;
 }
@@ -2384,9 +2406,7 @@ static inline int prctl_get_mdwe(unsigned long arg2, unsigned long arg3,
 {
 	if (arg2 || arg3 || arg4 || arg5)
 		return -EINVAL;
-
-	return test_bit(MMF_HAS_MDWE, &current->mm->flags) ?
-		PR_MDWE_REFUSE_EXEC_GAIN : 0;
+	return (int)get_current_mdwe();
 }
 
 static int prctl_get_auxv(void __user *addr, unsigned long len)
diff --git a/tools/include/uapi/linux/prctl.h b/tools/include/uapi/linux/prctl.h
index 6e9af6cbc950..dacbe824e7c3 100644
--- a/tools/include/uapi/linux/prctl.h
+++ b/tools/include/uapi/linux/prctl.h
@@ -284,6 +284,7 @@ struct prctl_mm_map {
 /* Memory deny write / execute */
 #define PR_SET_MDWE			65
 # define PR_MDWE_REFUSE_EXEC_GAIN	(1UL << 0)
+# define PR_MDWE_NO_INHERIT		(1UL << 1)
 
 #define PR_GET_MDWE			66
 
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH v3 5/5] kselftest: vm: Add tests for no-inherit memory-deny-write-execute

[Florent Revest]

Add some tests to cover the new PR_MDWE_NO_INHERIT flag of the
PR_SET_MDWE prctl.

Check that:
- it can't be set without PR_SET_MDWE
- MDWE flags can't be unset
- when set, PR_SET_MDWE doesn't propagate to children

Signed-off-by: Florent Revest <revest@chromium.org>
---
 tools/testing/selftests/mm/mdwe_test.c | 98 ++++++++++++++++++++++++--
 1 file changed, 92 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/mm/mdwe_test.c b/tools/testing/selftests/mm/mdwe_test.c
index 91aa9c3099e7..7bfc98bf9baa 100644
--- a/tools/testing/selftests/mm/mdwe_test.c
+++ b/tools/testing/selftests/mm/mdwe_test.c
@@ -22,6 +22,8 @@
 
 TEST(prctl_flags)
 {
+	EXPECT_LT(prctl(PR_SET_MDWE, PR_MDWE_NO_INHERIT, 0L, 0L, 7L), 0);
+
 	EXPECT_LT(prctl(PR_SET_MDWE, 7L, 0L, 0L, 0L), 0);
 	EXPECT_LT(prctl(PR_SET_MDWE, 0L, 7L, 0L, 0L), 0);
 	EXPECT_LT(prctl(PR_SET_MDWE, 0L, 0L, 7L, 0L), 0);
@@ -33,6 +35,69 @@ TEST(prctl_flags)
 	EXPECT_LT(prctl(PR_GET_MDWE, 0L, 0L, 0L, 7L), 0);
 }
 
+FIXTURE(consecutive_prctl_flags) {};
+FIXTURE_SETUP(consecutive_prctl_flags) {}
+FIXTURE_TEARDOWN(consecutive_prctl_flags) {}
+
+FIXTURE_VARIANT(consecutive_prctl_flags)
+{
+	unsigned long first_flags;
+	unsigned long second_flags;
+	bool should_work;
+};
+
+FIXTURE_VARIANT_ADD(consecutive_prctl_flags, same)
+{
+	.first_flags = PR_MDWE_REFUSE_EXEC_GAIN,
+	.second_flags = PR_MDWE_REFUSE_EXEC_GAIN,
+	.should_work = true,
+};
+
+FIXTURE_VARIANT_ADD(consecutive_prctl_flags, cant_disable_mdwe)
+{
+	.first_flags = PR_MDWE_REFUSE_EXEC_GAIN,
+	.second_flags = 0,
+	.should_work = false,
+};
+
+FIXTURE_VARIANT_ADD(consecutive_prctl_flags, cant_disable_mdwe_no_inherit)
+{
+	.first_flags = PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT,
+	.second_flags = 0,
+	.should_work = false,
+};
+
+FIXTURE_VARIANT_ADD(consecutive_prctl_flags, cant_disable_no_inherit)
+{
+	.first_flags = PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT,
+	.second_flags = PR_MDWE_REFUSE_EXEC_GAIN,
+	.should_work = false,
+};
+
+FIXTURE_VARIANT_ADD(consecutive_prctl_flags, cant_enable_no_inherit)
+{
+	.first_flags = PR_MDWE_REFUSE_EXEC_GAIN,
+	.second_flags = PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT,
+	.should_work = false,
+};
+
+TEST_F(consecutive_prctl_flags, two_prctls)
+{
+	int ret;
+
+	EXPECT_EQ(prctl(PR_SET_MDWE, variant->first_flags, 0L, 0L, 0L), 0);
+
+	ret = prctl(PR_SET_MDWE, variant->second_flags, 0L, 0L, 0L);
+	if (variant->should_work) {
+		EXPECT_EQ(ret, 0);
+
+		ret = prctl(PR_GET_MDWE, 0L, 0L, 0L, 0L);
+		ASSERT_EQ(ret, variant->second_flags);
+	} else {
+		EXPECT_NE(ret, 0);
+	}
+}
+
 FIXTURE(mdwe)
 {
 	void *p;
@@ -45,28 +110,45 @@ FIXTURE_VARIANT(mdwe)
 {
 	bool enabled;
 	bool forked;
+	bool inherit;
 };
 
 FIXTURE_VARIANT_ADD(mdwe, stock)
 {
 	.enabled = false,
 	.forked = false,
+	.inherit = false,
 };
 
 FIXTURE_VARIANT_ADD(mdwe, enabled)
 {
 	.enabled = true,
 	.forked = false,
+	.inherit = true,
 };
 
-FIXTURE_VARIANT_ADD(mdwe, forked)
+FIXTURE_VARIANT_ADD(mdwe, inherited)
 {
 	.enabled = true,
 	.forked = true,
+	.inherit = true,
 };
 
+FIXTURE_VARIANT_ADD(mdwe, not_inherited)
+{
+	.enabled = true,
+	.forked = true,
+	.inherit = false,
+};
+
+static bool executable_map_should_fail(const FIXTURE_VARIANT(mdwe) *variant)
+{
+	return variant->enabled && (!variant->forked || variant->inherit);
+}
+
 FIXTURE_SETUP(mdwe)
 {
+	unsigned long mdwe_flags;
 	int ret, status;
 
 	self->p = NULL;
@@ -76,13 +158,17 @@ FIXTURE_SETUP(mdwe)
 	if (!variant->enabled)
 		return;
 
-	ret = prctl(PR_SET_MDWE, PR_MDWE_REFUSE_EXEC_GAIN, 0L, 0L, 0L);
+	mdwe_flags = PR_MDWE_REFUSE_EXEC_GAIN;
+	if (!variant->inherit)
+		mdwe_flags |= PR_MDWE_NO_INHERIT;
+
+	ret = prctl(PR_SET_MDWE, mdwe_flags, 0L, 0L, 0L);
 	ASSERT_EQ(ret, 0) {
 		TH_LOG("PR_SET_MDWE failed or unsupported");
 	}
 
 	ret = prctl(PR_GET_MDWE, 0L, 0L, 0L, 0L);
-	ASSERT_EQ(ret, 1);
+	ASSERT_EQ(ret, mdwe_flags);
 
 	if (variant->forked) {
 		self->pid = fork();
@@ -113,7 +199,7 @@ TEST_F(mdwe, mmap_READ_EXEC)
 TEST_F(mdwe, mmap_WRITE_EXEC)
 {
 	self->p = mmap(NULL, self->size, PROT_WRITE | PROT_EXEC, self->flags, 0, 0);
-	if (variant->enabled) {
+	if (executable_map_should_fail(variant)) {
 		EXPECT_EQ(self->p, MAP_FAILED);
 	} else {
 		EXPECT_NE(self->p, MAP_FAILED);
@@ -139,7 +225,7 @@ TEST_F(mdwe, mprotect_add_EXEC)
 	ASSERT_NE(self->p, MAP_FAILED);
 
 	ret = mprotect(self->p, self->size, PROT_READ | PROT_EXEC);
-	if (variant->enabled) {
+	if (executable_map_should_fail(variant)) {
 		EXPECT_LT(ret, 0);
 	} else {
 		EXPECT_EQ(ret, 0);
@@ -154,7 +240,7 @@ TEST_F(mdwe, mprotect_WRITE_EXEC)
 	ASSERT_NE(self->p, MAP_FAILED);
 
 	ret = mprotect(self->p, self->size, PROT_WRITE | PROT_EXEC);
-	if (variant->enabled) {
+	if (executable_map_should_fail(variant)) {
 		EXPECT_LT(ret, 0);
 	} else {
 		EXPECT_EQ(ret, 0);
-- 
2.41.0.255.g8b1d071c50-goog

Re: [PATCH v3 3/5] mm: Make PR_MDWE_REFUSE_EXEC_GAIN an unsigned long

[Florent Revest]

On Tue, Jul 4, 2023 at 5:37 PM Florent Revest <revest@chromium.org> wrote:
>
> Defining a prctl flag as an int is a footgun because on a 64 bit machine
> and with a variadic implementation of prctl (like in musl and glibc),
> when used directly as a prctl argument, it can get casted to long with
> garbage upper bits which would result in unexpected behaviors.
>
> This patch changes the constant to an unsigned long to eliminate that
> possibilities. This does not break UAPI.
>
> Fixes: b507808ebce2 ("mm: implement memory-deny-write-execute as a prctl")
> Cc: linux-stable@vger.kernel.org

Oops, this was supposed to be stable@vger.kernel.org... I'll fix that
tag as part of v4.

Re: [PATCH v3 2/5] kselftest: vm: Fix mdwe's mmap_FIXED test case

[Ryan Roberts]

On 04/07/2023 16:36, Florent Revest wrote:
> I checked with the original author, the mmap_FIXED test case wasn't
> properly tested and fails. Currently, it maps two consecutive (non
> overlapping) pages and expects the second mapping to be denied by MDWE
> but these two pages have nothing to do with each other so MDWE is
> actually out of the picture here.
> 
> What the test actually intended to do was to remap a virtual address
> using MAP_FIXED. However, this operation unmaps the existing mapping and
> creates a new one so the va is backed by a new page and MDWE is again
> out of the picture, all remappings should succeed.
> 
> This patch keeps the test case to make it clear that this situation is
> expected to work.
> 
> Signed-off-by: Florent Revest <revest@chromium.org>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Fixes: 4cf1fe34fd18 ("kselftest: vm: add tests for memory-deny-write-execute")

I'm currently working to get all the mm selftests running (and ideally passing!)
on arm64 and saw these same spurious failures. Thanks for the patch!

Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
Tested-by: Ryan Roberts <ryan.roberts@arm.com>


> ---
>  tools/testing/selftests/mm/mdwe_test.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/mdwe_test.c b/tools/testing/selftests/mm/mdwe_test.c
> index d0954c657feb..91aa9c3099e7 100644
> --- a/tools/testing/selftests/mm/mdwe_test.c
> +++ b/tools/testing/selftests/mm/mdwe_test.c
> @@ -168,13 +168,10 @@ TEST_F(mdwe, mmap_FIXED)
>  	self->p = mmap(NULL, self->size, PROT_READ, self->flags, 0, 0);
>  	ASSERT_NE(self->p, MAP_FAILED);
>  
> -	p = mmap(self->p + self->size, self->size, PROT_READ | PROT_EXEC,
> +	/* MAP_FIXED unmaps the existing page before mapping which is allowed */
> +	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);
> -	}
> +	EXPECT_EQ(p, self->p);
>  }
>  
>  TEST_F(mdwe, arm64_BTI)

Re: [PATCH v3 2/5] kselftest: vm: Fix mdwe's mmap_FIXED test case

[Jain, Ayush]

On 7/4/2023 9:06 PM, Florent Revest wrote:
> I checked with the original author, the mmap_FIXED test case wasn't
> properly tested and fails. Currently, it maps two consecutive (non
> overlapping) pages and expects the second mapping to be denied by MDWE
> but these two pages have nothing to do with each other so MDWE is
> actually out of the picture here.
> 
> What the test actually intended to do was to remap a virtual address
> using MAP_FIXED. However, this operation unmaps the existing mapping and
> creates a new one so the va is backed by a new page and MDWE is again
> out of the picture, all remappings should succeed.
> 
> This patch keeps the test case to make it clear that this situation is
> expected to work.
> 
> Signed-off-by: Florent Revest <revest@chromium.org>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Fixes: 4cf1fe34fd18 ("kselftest: vm: add tests for memory-deny-write-execute")
> ---
>   tools/testing/selftests/mm/mdwe_test.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/mdwe_test.c b/tools/testing/selftests/mm/mdwe_test.c
> index d0954c657feb..91aa9c3099e7 100644
> --- a/tools/testing/selftests/mm/mdwe_test.c
> +++ b/tools/testing/selftests/mm/mdwe_test.c
> @@ -168,13 +168,10 @@ TEST_F(mdwe, mmap_FIXED)
>   	self->p = mmap(NULL, self->size, PROT_READ, self->flags, 0, 0);
>   	ASSERT_NE(self->p, MAP_FAILED);
>   
> -	p = mmap(self->p + self->size, self->size, PROT_READ | PROT_EXEC,
> +	/* MAP_FIXED unmaps the existing page before mapping which is allowed */
> +	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);
> -	}
> +	EXPECT_EQ(p, self->p);

Tested this patch on EPYC x86 platform and fixes the testcase.
Thanks for this patch!!

Tested-by: Ayush Jain <ayush.jain3@amd.com>

>   }
>   
>   TEST_F(mdwe, arm64_BTI)

Re: [PATCH v3 1/5] kselftest: vm: Fix tabs/spaces inconsistency in the mdwe test

[Kees Cook]

On Tue, Jul 04, 2023 at 05:36:25PM +0200, Florent Revest wrote:
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Florent Revest <revest@chromium.org>

I love a good trivial patch review! :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v3 2/5] kselftest: vm: Fix mdwe's mmap_FIXED test case

[Kees Cook]

On Tue, Jul 04, 2023 at 05:36:26PM +0200, Florent Revest wrote:
> I checked with the original author, the mmap_FIXED test case wasn't
> properly tested and fails. Currently, it maps two consecutive (non
> overlapping) pages and expects the second mapping to be denied by MDWE
> but these two pages have nothing to do with each other so MDWE is
> actually out of the picture here.
> 
> What the test actually intended to do was to remap a virtual address
> using MAP_FIXED. However, this operation unmaps the existing mapping and
> creates a new one so the va is backed by a new page and MDWE is again
> out of the picture, all remappings should succeed.
> 
> This patch keeps the test case to make it clear that this situation is
> expected to work.
> 
> Signed-off-by: Florent Revest <revest@chromium.org>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Fixes: 4cf1fe34fd18 ("kselftest: vm: add tests for memory-deny-write-execute")
> ---
>  tools/testing/selftests/mm/mdwe_test.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/mdwe_test.c b/tools/testing/selftests/mm/mdwe_test.c
> index d0954c657feb..91aa9c3099e7 100644
> --- a/tools/testing/selftests/mm/mdwe_test.c
> +++ b/tools/testing/selftests/mm/mdwe_test.c
> @@ -168,13 +168,10 @@ TEST_F(mdwe, mmap_FIXED)
>  	self->p = mmap(NULL, self->size, PROT_READ, self->flags, 0, 0);
>  	ASSERT_NE(self->p, MAP_FAILED);
>  
> -	p = mmap(self->p + self->size, self->size, PROT_READ | PROT_EXEC,
> +	/* MAP_FIXED unmaps the existing page before mapping which is allowed */
> +	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);
> -	}
> +	EXPECT_EQ(p, self->p);
>  }

This is just validating the MDWE doesn't block a MAP_FIXED replacement?

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

>  
>  TEST_F(mdwe, arm64_BTI)
> -- 
> 2.41.0.255.g8b1d071c50-goog
> 

-- 
Kees Cook

Re: [PATCH v3 3/5] mm: Make PR_MDWE_REFUSE_EXEC_GAIN an unsigned long

[Kees Cook]

On Tue, Jul 04, 2023 at 05:36:27PM +0200, Florent Revest wrote:
> Defining a prctl flag as an int is a footgun because on a 64 bit machine
> and with a variadic implementation of prctl (like in musl and glibc),
> when used directly as a prctl argument, it can get casted to long with
> garbage upper bits which would result in unexpected behaviors.
> 
> This patch changes the constant to an unsigned long to eliminate that
> possibilities. This does not break UAPI.
> 
> Fixes: b507808ebce2 ("mm: implement memory-deny-write-execute as a prctl")
> Cc: linux-stable@vger.kernel.org
> Signed-off-by: Florent Revest <revest@chromium.org>

Ah yes. I remember this pain with seccomp. :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v3 4/5] mm: Add a NO_INHERIT flag to the PR_SET_MDWE prctl

[Kees Cook]

On Tue, Jul 04, 2023 at 05:36:28PM +0200, Florent Revest wrote:
> This extends the current PR_SET_MDWE prctl arg with a bit to indicate
> that the process doesn't want MDWE protection to propagate to children.
> 
> To implement this no-inherit mode, the tag in current->mm->flags must be
> absent from MMF_INIT_MASK. This means that the encoding for "MDWE but
> without inherit" is different in the prctl than in the mm flags. This
> leads to a bit of bit-mangling in the prctl implementation.
> 
> Signed-off-by: Florent Revest <revest@chromium.org>
> ---
>  include/linux/sched/coredump.h   | 10 ++++++++++
>  include/uapi/linux/prctl.h       |  1 +
>  kernel/fork.c                    |  2 +-
>  kernel/sys.c                     | 32 ++++++++++++++++++++++++++------
>  tools/include/uapi/linux/prctl.h |  1 +
>  5 files changed, 39 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/sched/coredump.h b/include/linux/sched/coredump.h
> index 0ee96ea7a0e9..1b37fa8fc723 100644
> --- a/include/linux/sched/coredump.h
> +++ b/include/linux/sched/coredump.h
> @@ -91,4 +91,14 @@ static inline int get_dumpable(struct mm_struct *mm)
>  				 MMF_DISABLE_THP_MASK | MMF_HAS_MDWE_MASK)
>  
>  #define MMF_VM_MERGE_ANY	29
> +#define MMF_HAS_MDWE_NO_INHERIT	30
> +
> +static inline unsigned long mmf_init_flags(unsigned long flags)
> +{
> +	if (flags & (1UL << MMF_HAS_MDWE_NO_INHERIT))
> +		flags &= ~((1UL << MMF_HAS_MDWE) |
> +			   (1UL << MMF_HAS_MDWE_NO_INHERIT));
> +	return flags & MMF_INIT_MASK;
> +}
> +
>  #endif /* _LINUX_SCHED_COREDUMP_H */
> diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
> index 6e9af6cbc950..dacbe824e7c3 100644
> --- a/include/uapi/linux/prctl.h
> +++ b/include/uapi/linux/prctl.h
> @@ -284,6 +284,7 @@ struct prctl_mm_map {
>  /* Memory deny write / execute */
>  #define PR_SET_MDWE			65
>  # define PR_MDWE_REFUSE_EXEC_GAIN	(1UL << 0)
> +# define PR_MDWE_NO_INHERIT		(1UL << 1)
>  
>  #define PR_GET_MDWE			66
>  
> diff --git a/kernel/fork.c b/kernel/fork.c
> index d17995934eb4..bc3c762d378f 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1284,7 +1284,7 @@ static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p,
>  	hugetlb_count_init(mm);
>  
>  	if (current->mm) {
> -		mm->flags = current->mm->flags & MMF_INIT_MASK;
> +		mm->flags = mmf_init_flags(current->mm->flags);
>  		mm->def_flags = current->mm->def_flags & VM_INIT_DEF_MASK;
>  	} else {
>  		mm->flags = default_dump_filter;
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 339fee3eff6a..1a2dc3da43ea 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2362,19 +2362,41 @@ static int prctl_set_vma(unsigned long opt, unsigned long start,
>  }
>  #endif /* CONFIG_ANON_VMA_NAME */
>  
> +static inline unsigned long get_current_mdwe(void)
> +{
> +	unsigned long ret = 0;
> +
> +	if (test_bit(MMF_HAS_MDWE, &current->mm->flags))
> +		ret |= PR_MDWE_REFUSE_EXEC_GAIN;
> +	if (test_bit(MMF_HAS_MDWE_NO_INHERIT, &current->mm->flags))
> +		ret |= PR_MDWE_NO_INHERIT;
> +
> +	return ret;
> +}
> +
>  static inline int prctl_set_mdwe(unsigned long bits, unsigned long arg3,
>  				 unsigned long arg4, unsigned long arg5)
>  {
> +	unsigned long current_bits;
> +
>  	if (arg3 || arg4 || arg5)
>  		return -EINVAL;
>  
> -	if (bits & ~(PR_MDWE_REFUSE_EXEC_GAIN))
> +	if (bits & ~(PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT))
> +		return -EINVAL;
> +
> +	/* NO_INHERIT only makes sense with REFUSE_EXEC_GAIN */
> +	if (bits & PR_MDWE_NO_INHERIT && !(bits & PR_MDWE_REFUSE_EXEC_GAIN))
>  		return -EINVAL;
>  
> +	current_bits = get_current_mdwe();
> +	if (current_bits && current_bits != bits)
> +		return -EPERM; /* Cannot unset the flags */

I was pondering why PR_MDWE_NO_INHERIT can't be unset, but I guess it
doesn't matter. Anything forked with have it off, and any process
wanting to launch stuff before locking down can just skip running the
prctl() until later.

> +
> +	if (bits & PR_MDWE_NO_INHERIT)
> +		set_bit(MMF_HAS_MDWE_NO_INHERIT, &current->mm->flags);
>  	if (bits & PR_MDWE_REFUSE_EXEC_GAIN)
>  		set_bit(MMF_HAS_MDWE, &current->mm->flags);
> -	else if (test_bit(MMF_HAS_MDWE, &current->mm->flags))
> -		return -EPERM; /* Cannot unset the flag */
>  
>  	return 0;
>  }
> @@ -2384,9 +2406,7 @@ static inline int prctl_get_mdwe(unsigned long arg2, unsigned long arg3,
>  {
>  	if (arg2 || arg3 || arg4 || arg5)
>  		return -EINVAL;
> -
> -	return test_bit(MMF_HAS_MDWE, &current->mm->flags) ?
> -		PR_MDWE_REFUSE_EXEC_GAIN : 0;
> +	return (int)get_current_mdwe();
>  }
>  
>  static int prctl_get_auxv(void __user *addr, unsigned long len)
> diff --git a/tools/include/uapi/linux/prctl.h b/tools/include/uapi/linux/prctl.h
> index 6e9af6cbc950..dacbe824e7c3 100644
> --- a/tools/include/uapi/linux/prctl.h
> +++ b/tools/include/uapi/linux/prctl.h
> @@ -284,6 +284,7 @@ struct prctl_mm_map {
>  /* Memory deny write / execute */
>  #define PR_SET_MDWE			65
>  # define PR_MDWE_REFUSE_EXEC_GAIN	(1UL << 0)
> +# define PR_MDWE_NO_INHERIT		(1UL << 1)
>  
>  #define PR_GET_MDWE			66
>  
> -- 
> 2.41.0.255.g8b1d071c50-goog
> 

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v3 5/5] kselftest: vm: Add tests for no-inherit memory-deny-write-execute

[Kees Cook]

On Tue, Jul 04, 2023 at 05:36:29PM +0200, Florent Revest wrote:
> Add some tests to cover the new PR_MDWE_NO_INHERIT flag of the
> PR_SET_MDWE prctl.
> 
> Check that:
> - it can't be set without PR_SET_MDWE
> - MDWE flags can't be unset
> - when set, PR_SET_MDWE doesn't propagate to children

I love more self tests! :)

> 
> Signed-off-by: Florent Revest <revest@chromium.org>
> ---
>  tools/testing/selftests/mm/mdwe_test.c | 98 ++++++++++++++++++++++++--
>  1 file changed, 92 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/mdwe_test.c b/tools/testing/selftests/mm/mdwe_test.c
> index 91aa9c3099e7..7bfc98bf9baa 100644
> --- a/tools/testing/selftests/mm/mdwe_test.c
> +++ b/tools/testing/selftests/mm/mdwe_test.c
> @@ -22,6 +22,8 @@
>  
>  TEST(prctl_flags)
>  {
> +	EXPECT_LT(prctl(PR_SET_MDWE, PR_MDWE_NO_INHERIT, 0L, 0L, 7L), 0);
> +

An existing issue, but I think the errno should be checked for each
of these...

>  	EXPECT_LT(prctl(PR_SET_MDWE, 7L, 0L, 0L, 0L), 0);
>  	EXPECT_LT(prctl(PR_SET_MDWE, 0L, 7L, 0L, 0L), 0);
>  	EXPECT_LT(prctl(PR_SET_MDWE, 0L, 0L, 7L, 0L), 0);
> @@ -33,6 +35,69 @@ TEST(prctl_flags)
>  	EXPECT_LT(prctl(PR_GET_MDWE, 0L, 0L, 0L, 7L), 0);
>  }
>  
> +FIXTURE(consecutive_prctl_flags) {};
> +FIXTURE_SETUP(consecutive_prctl_flags) {}
> +FIXTURE_TEARDOWN(consecutive_prctl_flags) {}
> +
> +FIXTURE_VARIANT(consecutive_prctl_flags)
> +{
> +	unsigned long first_flags;
> +	unsigned long second_flags;
> +	bool should_work;
> +};
> +
> +FIXTURE_VARIANT_ADD(consecutive_prctl_flags, same)
> +{
> +	.first_flags = PR_MDWE_REFUSE_EXEC_GAIN,
> +	.second_flags = PR_MDWE_REFUSE_EXEC_GAIN,
> +	.should_work = true,
> +};

I think two more variants should be added to get all the combinations:

FIXTURE_VARIANT_ADD(consecutive_prctl_no_flags, same)
{
	.first_flags = 0,
	.second_flags = 0,
	.should_work = true,
};

FIXTURE_VARIANT_ADD(consecutive_prctl_both_flags, same)
{
	.first_flags = PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT,
	.second_flags = PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT,
	.should_work = true,
};

> +
> +FIXTURE_VARIANT_ADD(consecutive_prctl_flags, cant_disable_mdwe)
> +{
> +	.first_flags = PR_MDWE_REFUSE_EXEC_GAIN,
> +	.second_flags = 0,
> +	.should_work = false,
> +};
> +
> +FIXTURE_VARIANT_ADD(consecutive_prctl_flags, cant_disable_mdwe_no_inherit)
> +{
> +	.first_flags = PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT,
> +	.second_flags = 0,
> +	.should_work = false,
> +};
> +
> +FIXTURE_VARIANT_ADD(consecutive_prctl_flags, cant_disable_no_inherit)
> +{
> +	.first_flags = PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT,
> +	.second_flags = PR_MDWE_REFUSE_EXEC_GAIN,
> +	.should_work = false,
> +};
> +
> +FIXTURE_VARIANT_ADD(consecutive_prctl_flags, cant_enable_no_inherit)
> +{
> +	.first_flags = PR_MDWE_REFUSE_EXEC_GAIN,
> +	.second_flags = PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT,
> +	.should_work = false,
> +};
> +
> +TEST_F(consecutive_prctl_flags, two_prctls)
> +{
> +	int ret;
> +
> +	EXPECT_EQ(prctl(PR_SET_MDWE, variant->first_flags, 0L, 0L, 0L), 0);
> +
> +	ret = prctl(PR_SET_MDWE, variant->second_flags, 0L, 0L, 0L);
> +	if (variant->should_work) {
> +		EXPECT_EQ(ret, 0);
> +
> +		ret = prctl(PR_GET_MDWE, 0L, 0L, 0L, 0L);
> +		ASSERT_EQ(ret, variant->second_flags);
> +	} else {
> +		EXPECT_NE(ret, 0);

Please test the expected errno value here.

> +	}
> +}
> +
>  FIXTURE(mdwe)
>  {
>  	void *p;
> @@ -45,28 +110,45 @@ FIXTURE_VARIANT(mdwe)
>  {
>  	bool enabled;
>  	bool forked;
> +	bool inherit;
>  };
>  
>  FIXTURE_VARIANT_ADD(mdwe, stock)
>  {
>  	.enabled = false,
>  	.forked = false,
> +	.inherit = false,
>  };
>  
>  FIXTURE_VARIANT_ADD(mdwe, enabled)
>  {
>  	.enabled = true,
>  	.forked = false,
> +	.inherit = true,
>  };
>  
> -FIXTURE_VARIANT_ADD(mdwe, forked)
> +FIXTURE_VARIANT_ADD(mdwe, inherited)
>  {
>  	.enabled = true,
>  	.forked = true,
> +	.inherit = true,
>  };
>  
> +FIXTURE_VARIANT_ADD(mdwe, not_inherited)
> +{
> +	.enabled = true,
> +	.forked = true,
> +	.inherit = false,
> +};
> +
> +static bool executable_map_should_fail(const FIXTURE_VARIANT(mdwe) *variant)
> +{
> +	return variant->enabled && (!variant->forked || variant->inherit);
> +}
> +
>  FIXTURE_SETUP(mdwe)
>  {
> +	unsigned long mdwe_flags;
>  	int ret, status;
>  
>  	self->p = NULL;
> @@ -76,13 +158,17 @@ FIXTURE_SETUP(mdwe)
>  	if (!variant->enabled)
>  		return;
>  
> -	ret = prctl(PR_SET_MDWE, PR_MDWE_REFUSE_EXEC_GAIN, 0L, 0L, 0L);
> +	mdwe_flags = PR_MDWE_REFUSE_EXEC_GAIN;
> +	if (!variant->inherit)
> +		mdwe_flags |= PR_MDWE_NO_INHERIT;
> +
> +	ret = prctl(PR_SET_MDWE, mdwe_flags, 0L, 0L, 0L);
>  	ASSERT_EQ(ret, 0) {
>  		TH_LOG("PR_SET_MDWE failed or unsupported");
>  	}
>  
>  	ret = prctl(PR_GET_MDWE, 0L, 0L, 0L, 0L);
> -	ASSERT_EQ(ret, 1);
> +	ASSERT_EQ(ret, mdwe_flags);
>  
>  	if (variant->forked) {
>  		self->pid = fork();
> @@ -113,7 +199,7 @@ TEST_F(mdwe, mmap_READ_EXEC)
>  TEST_F(mdwe, mmap_WRITE_EXEC)
>  {
>  	self->p = mmap(NULL, self->size, PROT_WRITE | PROT_EXEC, self->flags, 0, 0);
> -	if (variant->enabled) {
> +	if (executable_map_should_fail(variant)) {
>  		EXPECT_EQ(self->p, MAP_FAILED);
>  	} else {
>  		EXPECT_NE(self->p, MAP_FAILED);
> @@ -139,7 +225,7 @@ TEST_F(mdwe, mprotect_add_EXEC)
>  	ASSERT_NE(self->p, MAP_FAILED);
>  
>  	ret = mprotect(self->p, self->size, PROT_READ | PROT_EXEC);
> -	if (variant->enabled) {
> +	if (executable_map_should_fail(variant)) {
>  		EXPECT_LT(ret, 0);
>  	} else {
>  		EXPECT_EQ(ret, 0);
> @@ -154,7 +240,7 @@ TEST_F(mdwe, mprotect_WRITE_EXEC)
>  	ASSERT_NE(self->p, MAP_FAILED);
>  
>  	ret = mprotect(self->p, self->size, PROT_WRITE | PROT_EXEC);
> -	if (variant->enabled) {
> +	if (executable_map_should_fail(variant)) {
>  		EXPECT_LT(ret, 0);
>  	} else {
>  		EXPECT_EQ(ret, 0);
> -- 
> 2.41.0.255.g8b1d071c50-goog
> 

Otherwise looks good to me!

-- 
Kees Cook

Re: [PATCH v3 4/5] mm: Add a NO_INHERIT flag to the PR_SET_MDWE prctl

[Catalin Marinas]

On Fri, Aug 25, 2023 at 03:38:36PM -0700, Kees Cook wrote:
> On Tue, Jul 04, 2023 at 05:36:28PM +0200, Florent Revest wrote:
> >  static inline int prctl_set_mdwe(unsigned long bits, unsigned long arg3,
> >  				 unsigned long arg4, unsigned long arg5)
> >  {
> > +	unsigned long current_bits;
> > +
> >  	if (arg3 || arg4 || arg5)
> >  		return -EINVAL;
> >  
> > -	if (bits & ~(PR_MDWE_REFUSE_EXEC_GAIN))
> > +	if (bits & ~(PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT))
> > +		return -EINVAL;
> > +
> > +	/* NO_INHERIT only makes sense with REFUSE_EXEC_GAIN */
> > +	if (bits & PR_MDWE_NO_INHERIT && !(bits & PR_MDWE_REFUSE_EXEC_GAIN))
> >  		return -EINVAL;
> >  
> > +	current_bits = get_current_mdwe();
> > +	if (current_bits && current_bits != bits)
> > +		return -EPERM; /* Cannot unset the flags */
> 
> I was pondering why PR_MDWE_NO_INHERIT can't be unset, but I guess it
> doesn't matter. Anything forked with have it off, and any process
> wanting to launch stuff before locking down can just skip running the
> prctl() until later.

I had a similar doubt initially but then realised that the no-inherit
mode won't be inherited and concluded it's ok.

-- 
Catalin

Re: [PATCH v3 1/5] kselftest: vm: Fix tabs/spaces inconsistency in the mdwe test

[Catalin Marinas]

On Tue, Jul 04, 2023 at 05:36:25PM +0200, Florent Revest wrote:
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Florent Revest <revest@chromium.org>

I forgot about this series until Kees started reviewing it. So:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v3 2/5] kselftest: vm: Fix mdwe's mmap_FIXED test case

[Catalin Marinas]

On Tue, Jul 04, 2023 at 05:36:26PM +0200, Florent Revest wrote:
> I checked with the original author, the mmap_FIXED test case wasn't
> properly tested and fails. Currently, it maps two consecutive (non
> overlapping) pages and expects the second mapping to be denied by MDWE
> but these two pages have nothing to do with each other so MDWE is
> actually out of the picture here.
> 
> What the test actually intended to do was to remap a virtual address
> using MAP_FIXED. However, this operation unmaps the existing mapping and
> creates a new one so the va is backed by a new page and MDWE is again
> out of the picture, all remappings should succeed.
> 
> This patch keeps the test case to make it clear that this situation is
> expected to work.
> 
> Signed-off-by: Florent Revest <revest@chromium.org>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Fixes: 4cf1fe34fd18 ("kselftest: vm: add tests for memory-deny-write-execute")

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v3 4/5] mm: Add a NO_INHERIT flag to the PR_SET_MDWE prctl

[Catalin Marinas]

On Tue, Jul 04, 2023 at 05:36:28PM +0200, Florent Revest wrote:
>  static inline int prctl_set_mdwe(unsigned long bits, unsigned long arg3,
>  				 unsigned long arg4, unsigned long arg5)
>  {
> +	unsigned long current_bits;
> +
>  	if (arg3 || arg4 || arg5)
>  		return -EINVAL;
>  
> -	if (bits & ~(PR_MDWE_REFUSE_EXEC_GAIN))
> +	if (bits & ~(PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT))
> +		return -EINVAL;
> +
> +	/* NO_INHERIT only makes sense with REFUSE_EXEC_GAIN */
> +	if (bits & PR_MDWE_NO_INHERIT && !(bits & PR_MDWE_REFUSE_EXEC_GAIN))
>  		return -EINVAL;
>  
> +	current_bits = get_current_mdwe();
> +	if (current_bits && current_bits != bits)
> +		return -EPERM; /* Cannot unset the flags */
> +
> +	if (bits & PR_MDWE_NO_INHERIT)
> +		set_bit(MMF_HAS_MDWE_NO_INHERIT, &current->mm->flags);
>  	if (bits & PR_MDWE_REFUSE_EXEC_GAIN)
>  		set_bit(MMF_HAS_MDWE, &current->mm->flags);
> -	else if (test_bit(MMF_HAS_MDWE, &current->mm->flags))
> -		return -EPERM; /* Cannot unset the flag */
>  
>  	return 0;
>  }

This works for me, it's easier to read. I don't expect a use-case where
the app starts with MDWE no-inherit, forks some new processes and wants
to turn inherit on for further forking.

> @@ -2384,9 +2406,7 @@ static inline int prctl_get_mdwe(unsigned long arg2, unsigned long arg3,
>  {
>  	if (arg2 || arg3 || arg4 || arg5)
>  		return -EINVAL;
> -
> -	return test_bit(MMF_HAS_MDWE, &current->mm->flags) ?
> -		PR_MDWE_REFUSE_EXEC_GAIN : 0;
> +	return (int)get_current_mdwe();

Nitpick: the type conversion should be handled by the compiler as
prctl_get_mdwe() returns an int already.

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v3 5/5] kselftest: vm: Add tests for no-inherit memory-deny-write-execute

[Catalin Marinas]

On Tue, Jul 04, 2023 at 05:36:29PM +0200, Florent Revest wrote:
> Add some tests to cover the new PR_MDWE_NO_INHERIT flag of the
> PR_SET_MDWE prctl.
> 
> Check that:
> - it can't be set without PR_SET_MDWE
> - MDWE flags can't be unset
> - when set, PR_SET_MDWE doesn't propagate to children
> 
> Signed-off-by: Florent Revest <revest@chromium.org>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v3 1/5] kselftest: vm: Fix tabs/spaces inconsistency in the mdwe test

[Florent Revest]

On Sun, Aug 27, 2023 at 3:09 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Tue, Jul 04, 2023 at 05:36:25PM +0200, Florent Revest wrote:
> > Reviewed-by: David Hildenbrand <david@redhat.com>
> > Signed-off-by: Florent Revest <revest@chromium.org>
>
> I forgot about this series until Kees started reviewing it. So:

No problem! I was hoping everyone was having a good summer break and
was waiting for September to re-send a v4 anyway... :)

> Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks everyone for the reviews and testing! Much appreciated.

Re: [PATCH v3 2/5] kselftest: vm: Fix mdwe's mmap_FIXED test case

[Florent Revest]

On Sat, Aug 26, 2023 at 12:28 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Jul 04, 2023 at 05:36:26PM +0200, Florent Revest wrote:
> > I checked with the original author, the mmap_FIXED test case wasn't
> > properly tested and fails. Currently, it maps two consecutive (non
> > overlapping) pages and expects the second mapping to be denied by MDWE
> > but these two pages have nothing to do with each other so MDWE is
> > actually out of the picture here.
> >
> > What the test actually intended to do was to remap a virtual address
> > using MAP_FIXED. However, this operation unmaps the existing mapping and
> > creates a new one so the va is backed by a new page and MDWE is again
> > out of the picture, all remappings should succeed.
> >
> > This patch keeps the test case to make it clear that this situation is
> > expected to work.
> >
> > Signed-off-by: Florent Revest <revest@chromium.org>
> > Reviewed-by: David Hildenbrand <david@redhat.com>
> > Fixes: 4cf1fe34fd18 ("kselftest: vm: add tests for memory-deny-write-execute")
> > ---
> >  tools/testing/selftests/mm/mdwe_test.c | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/testing/selftests/mm/mdwe_test.c b/tools/testing/selftests/mm/mdwe_test.c
> > index d0954c657feb..91aa9c3099e7 100644
> > --- a/tools/testing/selftests/mm/mdwe_test.c
> > +++ b/tools/testing/selftests/mm/mdwe_test.c
> > @@ -168,13 +168,10 @@ TEST_F(mdwe, mmap_FIXED)
> >       self->p = mmap(NULL, self->size, PROT_READ, self->flags, 0, 0);
> >       ASSERT_NE(self->p, MAP_FAILED);
> >
> > -     p = mmap(self->p + self->size, self->size, PROT_READ | PROT_EXEC,
> > +     /* MAP_FIXED unmaps the existing page before mapping which is allowed */
> > +     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);
> > -     }
> > +     EXPECT_EQ(p, self->p);
> >  }
>
> This is just validating the MDWE doesn't block a MAP_FIXED replacement?

Yes! :)

In v4 I modified the description just a little bit to state that out clearly.

Re: [PATCH v3 4/5] mm: Add a NO_INHERIT flag to the PR_SET_MDWE prctl

[Florent Revest]

On Sun, Aug 27, 2023 at 3:09 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Fri, Aug 25, 2023 at 03:38:36PM -0700, Kees Cook wrote:
> > On Tue, Jul 04, 2023 at 05:36:28PM +0200, Florent Revest wrote:
> > >  static inline int prctl_set_mdwe(unsigned long bits, unsigned long arg3,
> > >                              unsigned long arg4, unsigned long arg5)
> > >  {
> > > +   unsigned long current_bits;
> > > +
> > >     if (arg3 || arg4 || arg5)
> > >             return -EINVAL;
> > >
> > > -   if (bits & ~(PR_MDWE_REFUSE_EXEC_GAIN))
> > > +   if (bits & ~(PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT))
> > > +           return -EINVAL;
> > > +
> > > +   /* NO_INHERIT only makes sense with REFUSE_EXEC_GAIN */
> > > +   if (bits & PR_MDWE_NO_INHERIT && !(bits & PR_MDWE_REFUSE_EXEC_GAIN))
> > >             return -EINVAL;
> > >
> > > +   current_bits = get_current_mdwe();
> > > +   if (current_bits && current_bits != bits)
> > > +           return -EPERM; /* Cannot unset the flags */
> >
> > I was pondering why PR_MDWE_NO_INHERIT can't be unset, but I guess it
> > doesn't matter. Anything forked with have it off, and any process
> > wanting to launch stuff before locking down can just skip running the
> > prctl() until later.
>
> I had a similar doubt initially but then realised that the no-inherit
> mode won't be inherited and concluded it's ok.

Indeed. We previously discussed that in
https://lore.kernel.org/all/CABRcYmLt2KsCoD8WzyCTxuY=6ppuWEqyLSDRXSsmXSxPLHtEzQ@mail.gmail.com/
and I agreed this doesn't matter for our use case and this keeps the
code a lot more maintainable :)

Re: [PATCH v3 4/5] mm: Add a NO_INHERIT flag to the PR_SET_MDWE prctl

[Florent Revest]

On Sun, Aug 27, 2023 at 3:10 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Tue, Jul 04, 2023 at 05:36:28PM +0200, Florent Revest wrote:
> > @@ -2384,9 +2406,7 @@ static inline int prctl_get_mdwe(unsigned long arg2, unsigned long arg3,
> >  {
> >       if (arg2 || arg3 || arg4 || arg5)
> >               return -EINVAL;
> > -
> > -     return test_bit(MMF_HAS_MDWE, &current->mm->flags) ?
> > -             PR_MDWE_REFUSE_EXEC_GAIN : 0;
> > +     return (int)get_current_mdwe();
>
> Nitpick: the type conversion should be handled by the compiler as
> prctl_get_mdwe() returns an int already.

Ah yes. Not sure why I added this one... :) thank you!

Re: [PATCH v3 5/5] kselftest: vm: Add tests for no-inherit memory-deny-write-execute

[Florent Revest]

On Sat, Aug 26, 2023 at 12:45 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Jul 04, 2023 at 05:36:29PM +0200, Florent Revest wrote:
> > Add some tests to cover the new PR_MDWE_NO_INHERIT flag of the
> > PR_SET_MDWE prctl.
> >
> > Check that:
> > - it can't be set without PR_SET_MDWE
> > - MDWE flags can't be unset
> > - when set, PR_SET_MDWE doesn't propagate to children
>
> I love more self tests! :)

*Insert here a ridiculously long series of party and dancing emojis* ... :)

> >
> > Signed-off-by: Florent Revest <revest@chromium.org>
> > ---
> >  tools/testing/selftests/mm/mdwe_test.c | 98 ++++++++++++++++++++++++--
> >  1 file changed, 92 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/testing/selftests/mm/mdwe_test.c b/tools/testing/selftests/mm/mdwe_test.c
> > index 91aa9c3099e7..7bfc98bf9baa 100644
> > --- a/tools/testing/selftests/mm/mdwe_test.c
> > +++ b/tools/testing/selftests/mm/mdwe_test.c
> > @@ -22,6 +22,8 @@
> >
> >  TEST(prctl_flags)
> >  {
> > +     EXPECT_LT(prctl(PR_SET_MDWE, PR_MDWE_NO_INHERIT, 0L, 0L, 7L), 0);
> > +
>
> An existing issue, but I think the errno should be checked for each
> of these...

Makes sense! I'll add a bunch of
    EXPECT_EQ(errno, EINVAL);

To every existing line here as part of a previous patch, and modify
this patch to cover errnos in the new test cases too.

> >       EXPECT_LT(prctl(PR_SET_MDWE, 7L, 0L, 0L, 0L), 0);
> >       EXPECT_LT(prctl(PR_SET_MDWE, 0L, 7L, 0L, 0L), 0);
> >       EXPECT_LT(prctl(PR_SET_MDWE, 0L, 0L, 7L, 0L), 0);
> > @@ -33,6 +35,69 @@ TEST(prctl_flags)
> >       EXPECT_LT(prctl(PR_GET_MDWE, 0L, 0L, 0L, 7L), 0);
> >  }
> >
> > +FIXTURE(consecutive_prctl_flags) {};
> > +FIXTURE_SETUP(consecutive_prctl_flags) {}
> > +FIXTURE_TEARDOWN(consecutive_prctl_flags) {}
> > +
> > +FIXTURE_VARIANT(consecutive_prctl_flags)
> > +{
> > +     unsigned long first_flags;
> > +     unsigned long second_flags;
> > +     bool should_work;
> > +};
> > +
> > +FIXTURE_VARIANT_ADD(consecutive_prctl_flags, same)
> > +{
> > +     .first_flags = PR_MDWE_REFUSE_EXEC_GAIN,
> > +     .second_flags = PR_MDWE_REFUSE_EXEC_GAIN,
> > +     .should_work = true,
> > +};
>
> I think two more variants should be added to get all the combinations:
>
> FIXTURE_VARIANT_ADD(consecutive_prctl_no_flags, same)
> {
>         .first_flags = 0,
>         .second_flags = 0,
>         .should_work = true,
> };
>
> FIXTURE_VARIANT_ADD(consecutive_prctl_both_flags, same)
> {
>         .first_flags = PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT,
>         .second_flags = PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT,
>         .should_work = true,
> };

Agreed! :)

> > +
> > +FIXTURE_VARIANT_ADD(consecutive_prctl_flags, cant_disable_mdwe)
> > +{
> > +     .first_flags = PR_MDWE_REFUSE_EXEC_GAIN,
> > +     .second_flags = 0,
> > +     .should_work = false,
> > +};
> > +
> > +FIXTURE_VARIANT_ADD(consecutive_prctl_flags, cant_disable_mdwe_no_inherit)
> > +{
> > +     .first_flags = PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT,
> > +     .second_flags = 0,
> > +     .should_work = false,
> > +};
> > +
> > +FIXTURE_VARIANT_ADD(consecutive_prctl_flags, cant_disable_no_inherit)
> > +{
> > +     .first_flags = PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT,
> > +     .second_flags = PR_MDWE_REFUSE_EXEC_GAIN,
> > +     .should_work = false,
> > +};
> > +
> > +FIXTURE_VARIANT_ADD(consecutive_prctl_flags, cant_enable_no_inherit)
> > +{
> > +     .first_flags = PR_MDWE_REFUSE_EXEC_GAIN,
> > +     .second_flags = PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT,
> > +     .should_work = false,
> > +};
> > +
> > +TEST_F(consecutive_prctl_flags, two_prctls)
> > +{
> > +     int ret;
> > +
> > +     EXPECT_EQ(prctl(PR_SET_MDWE, variant->first_flags, 0L, 0L, 0L), 0);
> > +
> > +     ret = prctl(PR_SET_MDWE, variant->second_flags, 0L, 0L, 0L);
> > +     if (variant->should_work) {
> > +             EXPECT_EQ(ret, 0);
> > +
> > +             ret = prctl(PR_GET_MDWE, 0L, 0L, 0L, 0L);
> > +             ASSERT_EQ(ret, variant->second_flags);
> > +     } else {
> > +             EXPECT_NE(ret, 0);
>
> Please test the expected errno value here.

Alright!

> > +     }
> > +}
> > +
> >  FIXTURE(mdwe)
> >  {
> >       void *p;
> > @@ -45,28 +110,45 @@ FIXTURE_VARIANT(mdwe)
> >  {
> >       bool enabled;
> >       bool forked;
> > +     bool inherit;
> >  };
> >
> >  FIXTURE_VARIANT_ADD(mdwe, stock)
> >  {
> >       .enabled = false,
> >       .forked = false,
> > +     .inherit = false,
> >  };
> >
> >  FIXTURE_VARIANT_ADD(mdwe, enabled)
> >  {
> >       .enabled = true,
> >       .forked = false,
> > +     .inherit = true,
> >  };
> >
> > -FIXTURE_VARIANT_ADD(mdwe, forked)
> > +FIXTURE_VARIANT_ADD(mdwe, inherited)
> >  {
> >       .enabled = true,
> >       .forked = true,
> > +     .inherit = true,
> >  };
> >
> > +FIXTURE_VARIANT_ADD(mdwe, not_inherited)
> > +{
> > +     .enabled = true,
> > +     .forked = true,
> > +     .inherit = false,
> > +};
> > +
> > +static bool executable_map_should_fail(const FIXTURE_VARIANT(mdwe) *variant)
> > +{
> > +     return variant->enabled && (!variant->forked || variant->inherit);
> > +}
> > +
> >  FIXTURE_SETUP(mdwe)
> >  {
> > +     unsigned long mdwe_flags;
> >       int ret, status;
> >
> >       self->p = NULL;
> > @@ -76,13 +158,17 @@ FIXTURE_SETUP(mdwe)
> >       if (!variant->enabled)
> >               return;
> >
> > -     ret = prctl(PR_SET_MDWE, PR_MDWE_REFUSE_EXEC_GAIN, 0L, 0L, 0L);
> > +     mdwe_flags = PR_MDWE_REFUSE_EXEC_GAIN;
> > +     if (!variant->inherit)
> > +             mdwe_flags |= PR_MDWE_NO_INHERIT;
> > +
> > +     ret = prctl(PR_SET_MDWE, mdwe_flags, 0L, 0L, 0L);
> >       ASSERT_EQ(ret, 0) {
> >               TH_LOG("PR_SET_MDWE failed or unsupported");
> >       }
> >
> >       ret = prctl(PR_GET_MDWE, 0L, 0L, 0L, 0L);
> > -     ASSERT_EQ(ret, 1);
> > +     ASSERT_EQ(ret, mdwe_flags);
> >
> >       if (variant->forked) {
> >               self->pid = fork();
> > @@ -113,7 +199,7 @@ TEST_F(mdwe, mmap_READ_EXEC)
> >  TEST_F(mdwe, mmap_WRITE_EXEC)
> >  {
> >       self->p = mmap(NULL, self->size, PROT_WRITE | PROT_EXEC, self->flags, 0, 0);
> > -     if (variant->enabled) {
> > +     if (executable_map_should_fail(variant)) {
> >               EXPECT_EQ(self->p, MAP_FAILED);
> >       } else {
> >               EXPECT_NE(self->p, MAP_FAILED);
> > @@ -139,7 +225,7 @@ TEST_F(mdwe, mprotect_add_EXEC)
> >       ASSERT_NE(self->p, MAP_FAILED);
> >
> >       ret = mprotect(self->p, self->size, PROT_READ | PROT_EXEC);
> > -     if (variant->enabled) {
> > +     if (executable_map_should_fail(variant)) {
> >               EXPECT_LT(ret, 0);
> >       } else {
> >               EXPECT_EQ(ret, 0);
> > @@ -154,7 +240,7 @@ TEST_F(mdwe, mprotect_WRITE_EXEC)
> >       ASSERT_NE(self->p, MAP_FAILED);
> >
> >       ret = mprotect(self->p, self->size, PROT_WRITE | PROT_EXEC);
> > -     if (variant->enabled) {
> > +     if (executable_map_should_fail(variant)) {
> >               EXPECT_LT(ret, 0);
> >       } else {
> >               EXPECT_EQ(ret, 0);
> > --
> > 2.41.0.255.g8b1d071c50-goog
> >
>
> Otherwise looks good to me!

*more happy emojis*




On Sat, Aug 26, 2023 at 12:45 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Jul 04, 2023 at 05:36:29PM +0200, Florent Revest wrote:
> > Add some tests to cover the new PR_MDWE_NO_INHERIT flag of the
> > PR_SET_MDWE prctl.
> >
> > Check that:
> > - it can't be set without PR_SET_MDWE
> > - MDWE flags can't be unset
> > - when set, PR_SET_MDWE doesn't propagate to children
>
> I love more self tests! :)
>
> >
> > Signed-off-by: Florent Revest <revest@chromium.org>
> > ---
> >  tools/testing/selftests/mm/mdwe_test.c | 98 ++++++++++++++++++++++++--
> >  1 file changed, 92 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/testing/selftests/mm/mdwe_test.c b/tools/testing/selftests/mm/mdwe_test.c
> > index 91aa9c3099e7..7bfc98bf9baa 100644
> > --- a/tools/testing/selftests/mm/mdwe_test.c
> > +++ b/tools/testing/selftests/mm/mdwe_test.c
> > @@ -22,6 +22,8 @@
> >
> >  TEST(prctl_flags)
> >  {
> > +     EXPECT_LT(prctl(PR_SET_MDWE, PR_MDWE_NO_INHERIT, 0L, 0L, 7L), 0);
> > +
>
> An existing issue, but I think the errno should be checked for each
> of these...
>
> >       EXPECT_LT(prctl(PR_SET_MDWE, 7L, 0L, 0L, 0L), 0);
> >       EXPECT_LT(prctl(PR_SET_MDWE, 0L, 7L, 0L, 0L), 0);
> >       EXPECT_LT(prctl(PR_SET_MDWE, 0L, 0L, 7L, 0L), 0);
> > @@ -33,6 +35,69 @@ TEST(prctl_flags)
> >       EXPECT_LT(prctl(PR_GET_MDWE, 0L, 0L, 0L, 7L), 0);
> >  }
> >
> > +FIXTURE(consecutive_prctl_flags) {};
> > +FIXTURE_SETUP(consecutive_prctl_flags) {}
> > +FIXTURE_TEARDOWN(consecutive_prctl_flags) {}
> > +
> > +FIXTURE_VARIANT(consecutive_prctl_flags)
> > +{
> > +     unsigned long first_flags;
> > +     unsigned long second_flags;
> > +     bool should_work;
> > +};
> > +
> > +FIXTURE_VARIANT_ADD(consecutive_prctl_flags, same)
> > +{
> > +     .first_flags = PR_MDWE_REFUSE_EXEC_GAIN,
> > +     .second_flags = PR_MDWE_REFUSE_EXEC_GAIN,
> > +     .should_work = true,
> > +};
>
> I think two more variants should be added to get all the combinations:
>
> FIXTURE_VARIANT_ADD(consecutive_prctl_no_flags, same)
> {
>         .first_flags = 0,
>         .second_flags = 0,
>         .should_work = true,
> };
>
> FIXTURE_VARIANT_ADD(consecutive_prctl_both_flags, same)
> {
>         .first_flags = PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT,
>         .second_flags = PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT,
>         .should_work = true,
> };
>
> > +
> > +FIXTURE_VARIANT_ADD(consecutive_prctl_flags, cant_disable_mdwe)
> > +{
> > +     .first_flags = PR_MDWE_REFUSE_EXEC_GAIN,
> > +     .second_flags = 0,
> > +     .should_work = false,
> > +};
> > +
> > +FIXTURE_VARIANT_ADD(consecutive_prctl_flags, cant_disable_mdwe_no_inherit)
> > +{
> > +     .first_flags = PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT,
> > +     .second_flags = 0,
> > +     .should_work = false,
> > +};
> > +
> > +FIXTURE_VARIANT_ADD(consecutive_prctl_flags, cant_disable_no_inherit)
> > +{
> > +     .first_flags = PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT,
> > +     .second_flags = PR_MDWE_REFUSE_EXEC_GAIN,
> > +     .should_work = false,
> > +};
> > +
> > +FIXTURE_VARIANT_ADD(consecutive_prctl_flags, cant_enable_no_inherit)
> > +{
> > +     .first_flags = PR_MDWE_REFUSE_EXEC_GAIN,
> > +     .second_flags = PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT,
> > +     .should_work = false,
> > +};
> > +
> > +TEST_F(consecutive_prctl_flags, two_prctls)
> > +{
> > +     int ret;
> > +
> > +     EXPECT_EQ(prctl(PR_SET_MDWE, variant->first_flags, 0L, 0L, 0L), 0);
> > +
> > +     ret = prctl(PR_SET_MDWE, variant->second_flags, 0L, 0L, 0L);
> > +     if (variant->should_work) {
> > +             EXPECT_EQ(ret, 0);
> > +
> > +             ret = prctl(PR_GET_MDWE, 0L, 0L, 0L, 0L);
> > +             ASSERT_EQ(ret, variant->second_flags);
> > +     } else {
> > +             EXPECT_NE(ret, 0);
>
> Please test the expected errno value here.
>
> > +     }
> > +}
> > +
> >  FIXTURE(mdwe)
> >  {
> >       void *p;
> > @@ -45,28 +110,45 @@ FIXTURE_VARIANT(mdwe)
> >  {
> >       bool enabled;
> >       bool forked;
> > +     bool inherit;
> >  };
> >
> >  FIXTURE_VARIANT_ADD(mdwe, stock)
> >  {
> >       .enabled = false,
> >       .forked = false,
> > +     .inherit = false,
> >  };
> >
> >  FIXTURE_VARIANT_ADD(mdwe, enabled)
> >  {
> >       .enabled = true,
> >       .forked = false,
> > +     .inherit = true,
> >  };
> >
> > -FIXTURE_VARIANT_ADD(mdwe, forked)
> > +FIXTURE_VARIANT_ADD(mdwe, inherited)
> >  {
> >       .enabled = true,
> >       .forked = true,
> > +     .inherit = true,
> >  };
> >
> > +FIXTURE_VARIANT_ADD(mdwe, not_inherited)
> > +{
> > +     .enabled = true,
> > +     .forked = true,
> > +     .inherit = false,
> > +};
> > +
> > +static bool executable_map_should_fail(const FIXTURE_VARIANT(mdwe) *variant)
> > +{
> > +     return variant->enabled && (!variant->forked || variant->inherit);
> > +}
> > +
> >  FIXTURE_SETUP(mdwe)
> >  {
> > +     unsigned long mdwe_flags;
> >       int ret, status;
> >
> >       self->p = NULL;
> > @@ -76,13 +158,17 @@ FIXTURE_SETUP(mdwe)
> >       if (!variant->enabled)
> >               return;
> >
> > -     ret = prctl(PR_SET_MDWE, PR_MDWE_REFUSE_EXEC_GAIN, 0L, 0L, 0L);
> > +     mdwe_flags = PR_MDWE_REFUSE_EXEC_GAIN;
> > +     if (!variant->inherit)
> > +             mdwe_flags |= PR_MDWE_NO_INHERIT;
> > +
> > +     ret = prctl(PR_SET_MDWE, mdwe_flags, 0L, 0L, 0L);
> >       ASSERT_EQ(ret, 0) {
> >               TH_LOG("PR_SET_MDWE failed or unsupported");
> >       }
> >
> >       ret = prctl(PR_GET_MDWE, 0L, 0L, 0L, 0L);
> > -     ASSERT_EQ(ret, 1);
> > +     ASSERT_EQ(ret, mdwe_flags);
> >
> >       if (variant->forked) {
> >               self->pid = fork();
> > @@ -113,7 +199,7 @@ TEST_F(mdwe, mmap_READ_EXEC)
> >  TEST_F(mdwe, mmap_WRITE_EXEC)
> >  {
> >       self->p = mmap(NULL, self->size, PROT_WRITE | PROT_EXEC, self->flags, 0, 0);
> > -     if (variant->enabled) {
> > +     if (executable_map_should_fail(variant)) {
> >               EXPECT_EQ(self->p, MAP_FAILED);
> >       } else {
> >               EXPECT_NE(self->p, MAP_FAILED);
> > @@ -139,7 +225,7 @@ TEST_F(mdwe, mprotect_add_EXEC)
> >       ASSERT_NE(self->p, MAP_FAILED);
> >
> >       ret = mprotect(self->p, self->size, PROT_READ | PROT_EXEC);
> > -     if (variant->enabled) {
> > +     if (executable_map_should_fail(variant)) {
> >               EXPECT_LT(ret, 0);
> >       } else {
> >               EXPECT_EQ(ret, 0);
> > @@ -154,7 +240,7 @@ TEST_F(mdwe, mprotect_WRITE_EXEC)
> >       ASSERT_NE(self->p, MAP_FAILED);
> >
> >       ret = mprotect(self->p, self->size, PROT_WRITE | PROT_EXEC);
> > -     if (variant->enabled) {
> > +     if (executable_map_should_fail(variant)) {
> >               EXPECT_LT(ret, 0);
> >       } else {
> >               EXPECT_EQ(ret, 0);
> > --
> > 2.41.0.255.g8b1d071c50-goog
> >
>
> Otherwise looks good to me!
>
> --
> Kees Cook