mseal sysmap: Choose system mapping sealing at boot

CONFIG_MSEAL_SYSTEM_MAPPINGS seals each process' system mappings when they are created. As it is today this can only be toggled at build time, so hosts that need sealing and hosts running software which moves or unmaps these mappings end up with separate kernels. This is not ideal since it adds more complexity of deployment, it would be great to be able to have one kernel which can configurably do both.

So, to improve this, let's add mseal_system_mappings= so that this can be chosen at boot.

Signed-off-by Chris Down <[email protected]>

---
 .../admin-guide/kernel-parameters.txt         |   7 ++
 Documentation/userspace-api/mseal.rst         |   3 +-
 include/linux/mm.h                            |   3 +-
 mm/mseal.c                                    |  12 ++
 security/Kconfig                              |   2 +
 .../mseal_system_mappings/sysmap_is_sealed.c  | 104 +++++++++++++++---
 6 files changed, 115 insertions(+), 16 deletions(-)

Documentation/admin-guide/kernel-parameters.txt +0 -0

index e75344f4e0cd..b1ec9a8a979e 100644

Documentation/admin-guide/kernel-parameters.txt +1 -0

+
 	mtdparts=	[MTD]
 			See drivers/mtd/parsers/cmdlinepart.c
 

Documentation/userspace-api/mseal.rst +0 -0

index c039d782f9f5..4e50215e4762 100644

Documentation/userspace-api/mseal.rst +0 -0

   WARNING: This feature breaks programs which rely on relocating
   or unmapping system mappings. Known broken software at the time

include/linux/mm.h +0 -0

index 5f82be0fcde5..2329d81b7d46 100644

include/linux/mm.h +0 -0

 #else
 #define VM_SEALED_SYSMAP	VM_NONE
 #endif

mm/mseal.c +2 -0

index 7a8ac66dc215..e0563796cfa3 100644
@@ -7,6 +7,8 @@
  *  Author: Jeff Xu <[email protected]>
  */
 
+#include <linux/init.h>
+#include <linux/kstrtox.h>
 #include <linux/mempolicy.h>
 #include <linux/minmax.h>
 #include <linux/mman.h>

mm/mseal.c +8 -0

+
+static int __init early_mseal_sysmap(char *buf)
+{
+	return kstrtobool(buf, &mseal_sysmap_enabled);
+}
+early_param("mseal_system_mappings", early_mseal_sysmap);
+#endif
+
 static bool range_contains_unmapped(unsigned long start, unsigned long end)
 {
 	VMA_ITERATOR(vmi, current->mm, start);

security/Kconfig +0 -0

index f7bf6cdc6229..d5e83af1bfc6 100644

tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c +58 -0

 #define _GNU_SOURCE
 #include <stdio.h>
 #include <errno.h>
+#include <ctype.h>
 #include <unistd.h>
 #include <string.h>
 #include <stdbool.h>

@@ -17,6 +17,73 @@
 #define VMFLAGS "VmFlags:"
 #define MSEAL_FLAGS "sl"
 #define MAX_LINE_LEN 512
+#define SYSMAP_PARAM "mseal_system_mappings="
+
+static int parse_bool(const char *s, bool *res)
+{
+	if (!s)
+		return -EINVAL;
+
+	switch (tolower((unsigned char)s[0])) {
+	case 'e':
+	case 'y':
+	case 't':
+	case '1':
+		*res = true;
+		return 0;
+	case 'd':
+	case 'n':
+	case 'f':
+	case '0':
+		*res = false;
+		return 0;
+	case 'o':
+		switch (tolower((unsigned char)s[1])) {
+		case 'n':
+			*res = true;
+			return 0;
+		case 'f':
+			*res = false;
+			return 0;
+		}
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static int sysmap_sealed_expected(void)
+{
+	char cmdline[4096], *tok, *save;
+	int sealed = 1;
+	bool value;
+	size_t len;
+	FILE *f;
+
+	f = fopen("/proc/cmdline", "r");
+	if (!f)
+		return -1;
+	len = fread(cmdline, 1, sizeof(cmdline) - 1, f);
+	if (ferror(f) || !feof(f)) {
+		fclose(f);
+		return -1;
+	}
+	fclose(f);
+	cmdline[len] = '\0';
+
+	for (tok = strtok_r(cmdline, " \t\n", &save); tok;
+	     tok = strtok_r(NULL, " \t\n", &save)) {
+		if (!strcmp(tok, "--"))

tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c +21 -13

 bool has_mapping(char *name, FILE *maps)
 {

@@ -30,7 +97,7 @@ bool has_mapping(char *name, FILE *maps)
 	return false;
 }
 
-bool mapping_is_sealed(char *name, FILE *maps)
+int mapping_is_sealed(FILE *maps)
 {
 	char line[MAX_LINE_LEN];
 

@@ -43,7 +110,7 @@ bool mapping_is_sealed(char *name, FILE *maps)
 		}
 	}
 
-	return false;
+	return -1;
 }
 
 FIXTURE(basic) {

@@ -67,53 +134,62 @@ FIXTURE_TEARDOWN(basic)
 FIXTURE_VARIANT(basic)
 {
 	char *name;
-	bool sealed;
+	bool sysmap;
 };
 
 FIXTURE_VARIANT_ADD(basic, vdso) {
 	.name = "[vdso]",
-	.sealed = true,
+	.sysmap = true,
 };
 
 FIXTURE_VARIANT_ADD(basic, vvar) {
 	.name = "[vvar]",
-	.sealed = true,
+	.sysmap = true,
 };
 
 FIXTURE_VARIANT_ADD(basic, vvar_vclock) {
 	.name = "[vvar_vclock]",
-	.sealed = true,
+	.sysmap = true,
 };
 
 FIXTURE_VARIANT_ADD(basic, sigpage) {
 	.name = "[sigpage]",
-	.sealed = true,
+	.sysmap = true,
 };
 
 FIXTURE_VARIANT_ADD(basic, vectors) {
 	.name = "[vectors]",
-	.sealed = true,
+	.sysmap = true,
 };
 
 FIXTURE_VARIANT_ADD(basic, uprobes) {
 	.name = "[uprobes]",
-	.sealed = true,
+	.sysmap = true,
 };
 
 FIXTURE_VARIANT_ADD(basic, stack) {
 	.name = "[stack]",
-	.sealed = false,
+	.sysmap = false,
 };
 
 TEST_F(basic, check_sealed)
 {
+	int expected = 0, sealed;
+
 	if (!has_mapping(variant->name, self->maps)) {
 		SKIP(return, "could not find the mapping, %s",
 			variant->name);
 	}
 
-	EXPECT_EQ(variant->sealed,
-		mapping_is_sealed(variant->name, self->maps));
+	sealed = mapping_is_sealed(self->maps);
+	if (sealed < 0)
+		SKIP(return, "could not read the mapping's sealing state");
+	if (variant->sysmap) {
+		expected = sysmap_sealed_expected();
+		if (expected < 0)
+			SKIP(return, "could not determine the expected sealing state");
+	}
+	EXPECT_EQ(expected, sealed);
 };
 
 TEST_HARNESS_MAIN
--

2.54.0

The existing implementation is a bit of a mess, so any work that extends it has to put in the work to improve things first.

I _hate_ VM_SEALED_SYSMAP. It's a hack. A tasteful way of doing this is really required for a series touching this to be taken, honestly.

See below for implementation ideas.

(Also it's the merge window, so no more revisions until -rc1, when you need to resend, thanks.)

Could we have some actual justification for the use of the feature please? So far I've seen arm-waving.

Quoted 8 lines

Documentation/admin-guide/kernel-parameters.txt +0 -0

index e75344f4e0cd..b1ec9a8a979e 100644
@@ -4270,6 +4270,13 @@ Kernel parameters
 	MTD_Region=	[MTD] Format:
 			<name>,<region-number>[,<base>,<size>,<buswidth>,<altbuswidth>]
 

Documentation/admin-guide/kernel-parameters.txt Kernel parameters · line 4278

Quoted 6 lines

Documentation/admin-guide/kernel-parameters.txt +6 -0

+	mseal_system_mappings=
+			[KNL,EARLY] Under CONFIG_MSEAL_SYSTEM_MAPPINGS,
+			whether system mappings are sealed for this boot.
+			See Documentation/userspace-api/mseal.rst.
+			Format: <bool>
+			Default: 1 (enabled).

You _have_ to duplicate the list of things this breaks here please.

Let's negate things first. Make the boot flag 'no_mseal_sys_mappings' rather than confusingly implying mseal_system_mappings=1 could enable it even with a config option not provided.

Then add something like:

static inline bool mseal_system_mappings(void)

{
       return IS_ENABLED(CONFIG_MSEAL_SYSTEM_MAPPINGS) &&
              !mseal_system_mappings_disabled;
}

And call that in _install_special_mapping().

The LLM overlords tell me that only uprobe trampolines would see a delta from this given the ARCH_SUPPORTS_MSEAL_SYSTEM_MAPPINGS restriction.

This would improve things a lot.

Documentation/admin-guide/kernel-parameters.txt Kernel parameters · line 4279

Quoted 1 line

Documentation/admin-guide/kernel-parameters.txt +1 -0

+#define VM_SEALED_SYSMAP	(mseal_sysmap_enabled ? VM_SEALED : VM_NONE)

No, you don't use the legacy VMA flags API for new work. Sorry.

See above as to a pointer as to how this should look.

security/Kconfig config MSEAL_SYSTEM_MAPPINGS · line 71

Quoted 13 lines

Documentation/admin-guide/kernel-parameters.txt +0 -0

 static bool range_contains_unmapped(unsigned long start, unsigned long end)
 {
 	VMA_ITERATOR(vmi, current->mm, start);

security/Kconfig +2 -0

index f7bf6cdc6229..d5e83af1bfc6 100644
@@ -67,6 +67,8 @@ config MSEAL_SYSTEM_MAPPINGS
 	  of writing includes CHECKPOINT_RESTORE, UML, gVisor, rr. Therefore
 	  this config can't be enabled universally.
 
+	  Sealing can be turned off at boot with "mseal_system_mappings=0".
+

'With this configuration option set, sealing can...' right now this reads a little like it's defaulted on.

Also can we move this Kconfig option out of security and into mm please? This is explicitly an mm feature, and it should never have been added there (mm needs to know if somebody sets this default=y or something for instance).

Tests as separate commits please.

This seems total overkill, you're checking whether a boot parameter works?

Hi Lorenzo,

Thanks for the quick and thorough review!

Quoted 7 lines · Lorenzo Stoakes (ARM)

The existing implementation is a bit of a mess, so any work that extends it has to put in the work to improve things first.

I _hate_ VM_SEALED_SYSMAP. It's a hack. A tasteful way of doing this is really required for a series touching this to be taken, honestly.

See below for implementation ideas.

That's fair, I'll do the cleanup first in v2.

Quoted 3 lines

(Also it's the merge window, so no more revisions until -rc1, when you need to resend, thanks.)

Sure, I'll wait until after -rc1.

Quoted 3 lines

Could we have some actual justification for the use of the feature please? So far I've seen arm-waving.

Sure. At Meta, we build sealing into the kernel for our VM hosts, and we'd like it on as much of the rest of the fleet as possible. The problem is that the rest of the fleet also runs things which it breaks. Just as one example among many, Reverie (which Hermit our deterministic execution tool is built on) patches the vDSO of the processes it traces by injecting an mprotect()[0].

So right now the hosts which want sealing need their own kernel image, and this option is one of the things which forces that. We're working towards hopefully running one kernel across the fleet, and with a boot parameter we can build it with CONFIG_MSEAL_SYSTEM_MAPPINGS=y, keep sealing on by default, and turn it off only on the hosts which run those tools.

Let me know if you'd like more information. I can also put that in the commit message for v2.

0: https://docs.rs/reverie-core/latest/reverie/vdso/index.html

Quoted 8 lines

Quoted 8 lines

Documentation/admin-guide/kernel-parameters.txt +0 -0

index e75344f4e0cd..b1ec9a8a979e 100644
@@ -4270,6 +4270,13 @@ Kernel parameters
 	MTD_Region=	[MTD] Format:
 			<name>,<region-number>[,<base>,<size>,<buswidth>,<altbuswidth>]
 

Quoted 8 lines

Documentation/admin-guide/kernel-parameters.txt Kernel parameters · line 4278

Quoted 6 lines

Documentation/admin-guide/kernel-parameters.txt +6 -0

+	mseal_system_mappings=
+			[KNL,EARLY] Under CONFIG_MSEAL_SYSTEM_MAPPINGS,
+			whether system mappings are sealed for this boot.
+			See Documentation/userspace-api/mseal.rst.
+			Format: <bool>
+			Default: 1 (enabled).

You _have_ to duplicate the list of things this breaks here please.

Will do.

Quoted 13 lines

Let's negate things first. Make the boot flag 'no_mseal_sys_mappings' rather than confusingly implying mseal_system_mappings=1 could enable it even with a config option not provided.

Then add something like:

static inline bool mseal_system_mappings(void)

{
       return IS_ENABLED(CONFIG_MSEAL_SYSTEM_MAPPINGS) &&
              !mseal_system_mappings_disabled;
}

Quoted 1 line

And call that in _install_special_mapping().

Makes sense. I'll call it in __install_special_mapping(), since that's where we already have a vma_flags_t, so it can just set VMA_SEALED_BIT without going near the legacy flags at all, if that sounds good? Then VM_SEALED_SYSMAP and all of its users can hopefully go away.

Quoted 5 lines

The LLM overlords tell me that only uprobe trampolines would see a delta from this given the ARCH_SUPPORTS_MSEAL_SYSTEM_MAPPINGS restriction.

This would improve things a lot.

Yes, but there is a wrinkle. Since 07c308eb2bcf ("uprobes/x86: Do not leak trampoline vma mapping on optimization failure"), __arch_uprobe_optimize() unmaps a trampoline it has just created if swbp_optimize() fails, and with the trampoline sealed that would now fail with -EPERM. I can see two ways to deal with that:

One is to seal trampolines too, and just leave a sealed trampoline in place on that path. It's already a WARN_ON_ONCE(), and get_uprobe_trampoline() reuses any trampoline which is in reach, so nothing is really leaked.

Another way would be to keep trampolines unsealed, but that means bringing back some kind of per-mapping opt-in, which is exactly what we're trying to get rid of.

What do you think? Either way I'll Cc Jiri and Oleg on it.

Quoted 6 lines

Documentation/admin-guide/kernel-parameters.txt Kernel parameters · line 4279

Quoted 2 lines

Documentation/admin-guide/kernel-parameters.txt +1 -0

+#define VM_SEALED_SYSMAP	(mseal_sysmap_enabled ? VM_SEALED : VM_NONE)

No, you don't use the legacy VMA flags API for new work. Sorry.

See above as to a pointer as to how this should look.

Sure thing.

Quoted 18 lines

Quoted 9 lines

Documentation/admin-guide/kernel-parameters.txt +0 -0

 static bool range_contains_unmapped(unsigned long start, unsigned long end)
 {
 	VMA_ITERATOR(vmi, current->mm, start);

security/Kconfig +0 -0

index f7bf6cdc6229..d5e83af1bfc6 100644

security/Kconfig config MSEAL_SYSTEM_MAPPINGS · line 71

Quoted 5 lines

security/Kconfig +2 -0

@@ -67,6 +67,8 @@ config MSEAL_SYSTEM_MAPPINGS
 	  of writing includes CHECKPOINT_RESTORE, UML, gVisor, rr. Therefore
 	  this config can't be enabled universally.
 
+	  Sealing can be turned off at boot with "mseal_system_mappings=0".
+

'With this configuration option set, sealing can...' right now this reads a little like it's defaulted on.

Quoted 4 lines

Also can we move this Kconfig option out of security and into mm please? This is explicitly an mm feature, and it should never have been added there (mm needs to know if somebody sets this default=y or something for instance).

Sure thing.

Quoted 2 lines

Tests as separate commits please.

Happy to do so, but aren't you worried about bisect safety? Or would you suggest another way?

Quoted 1 line

This seems total overkill, you're checking whether a boot parameter works?

It was mostly there so that the existing test wouldn't fail when booted with mseal_system_mappings=0, which meant parsing a bool. With a negated flag it can be a lot simpler, but I'm not sure which of these you'd prefer:

One option is to skip if [vdso] isn't sealed, and otherwise expect all system mappings to be sealed. Then there's parsing at all, and it also stops failing on kernels built without CONFIG_MSEAL_SYSTEM_MAPPINGS, which matters more once it's in run_vmtests.sh. The downside is that it can't notice sealing being broken entirely.

Another is to check /proc/cmdline for no_mseal_sys_mappings and expect the mappings to be unsealed if it's there. That's only a few lines now, but the test still fails on kernels without the option, as it does today.

Do you have a preference between the two?

Thanks a lot for the review!

Quoted 12 lines

One option is to skip if [vdso] isn't sealed, and otherwise expect all system mappings to be sealed. Then there's parsing at all, and it also stops failing on kernels built without CONFIG_MSEAL_SYSTEM_MAPPINGS, which matters more once it's in run_vmtests.sh. The downside is that it can't notice sealing being broken entirely.

Another is to check /proc/cmdline for no_mseal_sys_mappings and expect the mappings to be unsealed if it's there. That's only a few lines now, but the test still fails on kernels without the option, as it does today.

Quoted 1 line

Do you have a preference between the two?

Yeah it's awkward, really moving the tests to mm we'd have to skip them if the vdso isn't sealed, then assume a sealed vdso for a new process = feature enabled.

I guess at that point it's redundant to check if disabled?

Could we please keep the test coverage for the unsealed [vdso] case and avoid unconditionally skipping whenever [vdso] is unsealed?

There are two regressions I'd love for us to still be able to catch:

1. Kernel or config regressions when sealing is expected to be on:

2. Regressions in the new no_mseal_sys_mappings boot parameter itself:

When booted with no_mseal_sys_mappings (or when unsealed), running the assertions in Patch 3 (verifying system mappings have no sl flag and that munmap() / mremap() -- and ideally mprotect(PROT_READ | PROT_WRITE) -- on [vdso] succeed) directly verifies that the boot parameter works and doesn't regress.

or keep strict failure as an option (e.g. ./sysmap_is_sealed fails if unsealed without the boot param, while run_vmtests.sh passes a flag like --allow-disabled when CONFIG_MSEAL_SYSTEM_MAPPINGS may not be enabled), so CI environments that expect sealing still catch a broken seal.

- run_vmtests runs with -T expect_sealed when CONFIG_MSEAL_SYSTEM_MAPPINGS=y

Quoted 5 lines · Chris Down

- Have check_sealed skip as usual when [vdso] isn't sealed and param is not set
- A new test, say expect_sealed, fails in the same case
- run_vmtests runs with -T expect_sealed when CONFIG_MSEAL_SYSTEM_MAPPINGS=y

Er, the other way around. I meant vm_runtests passes -T expect_sealed _unless_ CONFIG_MSEAL_SYSTEM_MAPPINGS=y.

Hi Jeff,

Thanks for the review!

Quoted 6 lines · Jeff Xu

Could we please keep the test coverage for the unsealed [vdso] case and avoid unconditionally skipping whenever [vdso] is unsealed?

There are two regressions I'd love for us to still be able to catch:

1. Kernel or config regressions when sealing is expected to be on:

Yeah agreed. With the cleanup it matters even more I think because when sealing is set in one place the most likely regression is probably one that unseals everything at once.

Quoted 1 line

2. Regressions in the new no_mseal_sys_mappings boot parameter itself:

In v2 I can check /proc/cmdline for no_mseal_sys_mappings to do that.

Lorenzo, I think that also may inform your question about whether checking for the disabled case is redundant, right?

Quoted 5 lines

When booted with no_mseal_sys_mappings (or when unsealed), running the assertions in Patch 3 (verifying system mappings have no sl flag and that munmap() / mremap() -- and ideally mprotect(PROT_READ | PROT_WRITE) -- on [vdso] succeed) directly verifies that the boot parameter works and doesn't regress.

Sounds good.

Quoted 5 lines

or keep strict failure as an option (e.g. ./sysmap_is_sealed fails if unsealed without the boot param, while run_vmtests.sh passes a flag like --allow-disabled when CONFIG_MSEAL_SYSTEM_MAPPINGS may not be enabled), so CI environments that expect sealing still catch a broken seal.

I like this and I think we can do it without adding some of the complexity that Lorenzo was not so happy about:

- Have check_sealed skip as usual when [vdso] isn't sealed and param is not set
- A new test, say expect_sealed, fails in the same case

- run_vmtests runs with -T expect_sealed when CONFIG_MSEAL_SYSTEM_MAPPINGS=y

That should be pretty minimal. Lorenzo, Jeff, how does that sound?

CONFIG_MSEAL_SYSTEM_MAPPINGS seals each process' system mappings when they are created. As it is today this can only be toggled at build time, so hosts that need sealing and hosts running software which moves or unmaps these mappings end up with separate kernels. This is not ideal since it adds more complexity of deployment, it would be great to be able to have one kernel which can configurably do both.

So, to improve this, let's add mseal_system_mappings= so that this can be chosen at boot.

MSEAL_SYSTEM_MAPPINGS depends on !CHECKPOINT_RESTORE because CRIU moves system mappings when restoring a process. Now that sealing can be chosen at boot we should instead use this to determine defaults.

Let's add MSEAL_SYSTEM_MAPPINGS_DEFAULT_ON and move the dependency there. Existing configurations retain their behaviour. Kernels built with CHECKPOINT_RESTORE can build sealing in, but will only seal mappings when explicitly requested with mseal_system_mappings=1.

Hosts can also share a kernel built with MSEAL_SYSTEM_MAPPINGS=y and MSEAL_SYSTEM_MAPPINGS_DEFAULT_ON=n, enabling sealing only where needed. This lets hosts which need sealing share the kernel image with hosts which need their system mappings to remain movable, which simplifies deployment.

Signed-off-by Chris Down <[email protected]>

Ok, sounds good. I'll drop MSEAL_SYSTEM_MAPPINGS_DEFAULT_ON, and fold dropping the !CHECKPOINT_RESTORE dependency into the boot parameter patch. Thanks!

tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c +12 -0

@@ -10,6 +10,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <stdbool.h>
+#include <sys/mman.h>
 #include <sys/utsname.h>
 #include <zlib.h>
 

@@ -232,4 +233,95 @@ TEST_F(basic, check_sealed)
 	EXPECT_EQ(expected, sealed);
 };
 
+static bool find_vdso(unsigned long *start, unsigned long *end)
+{
+	char line[MAX_LINE_LEN];
+	bool found = false;
+	FILE *maps;
+
+	maps = fopen("/proc/self/maps", "r");
+	if (!maps)
+		return false;
+
+	while (fgets(line, sizeof(line), maps)) {

tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c +50 -0

+
+static int vdso_is_sealed(void)
+{
+	int sealed = -1;
+	FILE *maps;
+
+	maps = fopen("/proc/self/smaps", "r");
+	if (!maps)
+		return -1;
+	if (has_mapping("[vdso]", maps))
+		sealed = mapping_is_sealed(maps);
+	fclose(maps);
+	return sealed;
+}
+
+/*
+ * Unmapping or moving [vdso] must fail with EPERM when it is sealed. When it
+ * is not sealed, it succeeds and leaves glibc with a stale vdso pointer, which
+ * is fine because each test runs in its own forked child.
+ */
+TEST(vdso_munmap)
+{
+	int sealed;
+	unsigned long start, end;
+	int ret;
+
+	if (!find_vdso(&start, &end))
+		SKIP(return, "could not find the mapping, [vdso]");
+	sealed = vdso_is_sealed();
+	if (sealed < 0)
+		SKIP(return, "could not read the [vdso] sealing state");
+
+	ret = munmap((void *)start, end - start);
+	if (sealed) {
+		EXPECT_EQ(-1, ret);
+		EXPECT_EQ(EPERM, errno);
+	} else {
+		EXPECT_EQ(0, ret);
+	}
+	EXPECT_EQ(sealed, find_vdso(&start, &end));
+}
+
+TEST(vdso_mremap)
+{
+	int sealed;
+	unsigned long start, end, size, new_start;
+	void *dst, *ret;
+
+	if (!find_vdso(&start, &end))
+		SKIP(return, "could not find the mapping, [vdso]");

tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c +10 -1

+	if (sealed) {
+		EXPECT_EQ(MAP_FAILED, ret);
+		EXPECT_EQ(EPERM, errno);
+	} else {
+		EXPECT_EQ(dst, ret);
+	}
+	ASSERT_TRUE(find_vdso(&new_start, &end));
+	EXPECT_EQ(sealed ? start : (unsigned long)dst, new_start);
+}
+
 TEST_HARNESS_MAIN
--

2.54.0

Sorry, in mm we require commit messages. Even for tests :)

tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c

Quoted 9 lines

Signed-off-by Chris Down <[email protected]>

---
 .../mseal_system_mappings/sysmap_is_sealed.c  | 92 +++++++++++++++++++
 1 file changed, 92 insertions(+)

tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c +0 -0

index ea75ad537620..83b4fadfbfe7 100644

Why on earth does 'mseal_system_mappings' have its own directory...??

Ugh I don't know how I let this stuff go on the original series :/

Please, for the love of all that's sacred, move these tests over to tools/selftests/mm/. This is an mm feature, for our sins...

You can do the move as a prerequisite patch in the series.

(And, again, I really do insist on your change doing some cleanup here as well rather than extending this stuff.)

Quoted 23 lines

tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c +12 -0

@@ -10,6 +10,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <stdbool.h>
+#include <sys/mman.h>
 #include <sys/utsname.h>
 #include <zlib.h>
 

@@ -232,4 +233,95 @@ TEST_F(basic, check_sealed)
 	EXPECT_EQ(expected, sealed);
 };
 
+static bool find_vdso(unsigned long *start, unsigned long *end)
+{
+	char line[MAX_LINE_LEN];
+	bool found = false;
+	FILE *maps;
+
+	maps = fopen("/proc/self/maps", "r");
+	if (!maps)
+		return false;
+
+	while (fgets(line, sizeof(line), maps)) {

tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c TEST_F(basic, check_sealed) · line 256

Quoted 10 lines

tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c +10 -0

+		if (strstr(line, "[vdso]") &&
+		    sscanf(line, "%lx-%lx", start, end) == 2) {
+			found = true;
+			break;
+		}
+	}
+
+	fclose(maps);
+	return found;
+}

There's shared code for this kind of thing, please don't duplicate a /proc/self/maps thing if unnecessary. Also there's a nicer ioctl interface for all this.

See vm_utils.[ch].

I mean arguably this is an mremap test that belongs with mremap? I guess debatable...

This all just seems like work that should have been done in the past when this feature was proposed, and doesn't depend on your changes at all AFAICT? Maybe I missed it though.

Valuable in itself, but probably worth sending separately as a result? Otherwise it's a little disconnected from the actual change you're making.

Quoted 1 line · Lorenzo Stoakes (ARM)

Sorry, in mm we require commit messages. Even for tests :)

Sure thing, I'll add one.

Quoted 13 lines · Chris Down

tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c

Quoted 9 lines

Signed-off-by Chris Down <[email protected]>

---
 .../mseal_system_mappings/sysmap_is_sealed.c  | 92 +++++++++++++++++++
 1 file changed, 92 insertions(+)

tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c +0 -0

index ea75ad537620..83b4fadfbfe7 100644

Why on earth does 'mseal_system_mappings' have its own directory...??

Quoted 6 lines

Ugh I don't know how I let this stuff go on the original series :/

Please, for the love of all that's sacred, move these tests over to tools/selftests/mm/. This is an mm feature, for our sins...

You can do the move as a prerequisite patch in the series.

Will do, I'll make the move the first patch.

Quoted 2 lines

(And, again, I really do insist on your change doing some cleanup here as well rather than extending this stuff.)

Yep, see my reply to 1/3.

Quoted 38 lines

Quoted 23 lines

tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c +12 -0

@@ -10,6 +10,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <stdbool.h>
+#include <sys/mman.h>
 #include <sys/utsname.h>
 #include <zlib.h>
 

@@ -232,4 +233,95 @@ TEST_F(basic, check_sealed)
 	EXPECT_EQ(expected, sealed);
 };
 
+static bool find_vdso(unsigned long *start, unsigned long *end)
+{
+	char line[MAX_LINE_LEN];
+	bool found = false;
+	FILE *maps;
+
+	maps = fopen("/proc/self/maps", "r");
+	if (!maps)
+		return false;
+
+	while (fgets(line, sizeof(line), maps)) {

tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c TEST_F(basic, check_sealed) · line 256

Quoted 10 lines

tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c +10 -0

+		if (strstr(line, "[vdso]") &&
+		    sscanf(line, "%lx-%lx", start, end) == 2) {
+			found = true;
+			break;
+		}
+	}
+
+	fclose(maps);
+	return found;
+}

There's shared code for this kind of thing, please don't duplicate a /proc/self/maps thing if unnecessary. Also there's a nicer ioctl interface for all this.

Quoted 1 line

See vm_utils.[ch].

Oh thank you! I will use that.

Quoted 2 lines

I mean arguably this is an mremap test that belongs with mremap? I guess debatable...

Hmm, I'd lean towards keeping it with the sysmap test, since it's really testing that the seal is enforced on these mappings rather than anything about mremap() itself, but I don't feel strongly if you insist, just let me know.

Quoted 7 lines

This all just seems like work that should have been done in the past when this feature was proposed, and doesn't depend on your changes at all AFAICT? Maybe I missed it though.

Valuable in itself, but probably worth sending separately as a result? Otherwise it's a little disconnected from the actual change you're making.

Yeah, it doesn't depend on it. I can send it separately on top of the move if you like, I just don't like touching things without tests where possible.

Thanks for the review!

System mapping sealing is currently a build time choice. Hosts which want it and hosts which need to move or unmap these mappings therefore need separate kernels, which is not ideal. Let's allow the administrator to choose at boot time.

Chris Down (3):
mseal sysmap: Add mseal_system_mappings= boot parameter mseal sysmap: Allow with CHECKPOINT_RESTORE, default off