Hi Lorenzo,

On Thu, Jan 2, 2025 at 9:30 AM Lorenzo Stoakes lorenzo.stoakes@oracle.com wrote:

Sorry for delay, was super busy leading up to xmas break, then had ~2.5 weeks off.

Thanks for reviewing. There are lots of comments, so it takes some time to go through comments and experiment with some of the suggested changes. Please see below.

And happy new year! :)

Happy new year !

On Wed, Dec 11, 2024 at 05:33:11AM +0000, jeffxu@chromium.org wrote:

...

From: Jeff Xu jeffxu@chromium.org

This change creates the initial version of memorysealing.c.

The introduction of memorysealing.c, which replaces mseal_test.c and

I mean I don't want to be a pain but I would kinda prefer to have 'mseal' everywhere mseal is to avoid confusion vs. memfd seals.

As I tried to explain, the memorysealing.c will eventually replace mseal_test.c, after all the tests are moved to use kselftest_karness. How about mseal_test_new.c ? I'm open to new names if you have one.

Of course we in the kernel absolutely _love_ to overload the meaning of terms but some traditions are worth breaking :)

...

uses the kselftest_harness, aims to initiate a discussion on using the selftest harness for memory sealing tests. Upon approval of this approach, the migration of tests from mseal_test.c to memorysealing.c can be implemented in a step-by-step manner.

Well, I for one like this approach so (unsurprisingly) :) but perhaps sensible to do it iteratively so we can also tweak things as we go.

Yes, that is the idea.

...

This tests addresses following feedback from previous reviews:

1> Use kselftest_harness instead of custom macro, such as EXPECT_XX, ASSERT_XX, etc. (Lorenzo Stoakes, Mark Brown) [1]

2> Use MAP_FAILED to check the return of mmap (Lorenzo Stoakes).

Thanks!

...

3> Adding a check for vma size and prot bits. The discussion for this can be found in [2] [3], here is a brief summary: This is to follow up on Pedro’s in-loop change (from can_modify_mm to can_modify_vma). When mseal_test is initially created, they have a common pattern: setup memory layout, seal the memory, perform a few mm-api steps, verify return code (not zero). Because of the nature of out-of-loop, it is sufficient to just verify the error code in a few cases.

With Pedro's in-loop change, the sealing check happens later in the
stack, thus there are more things and scenarios to verify. And there
was feedbacks to me that mseal_test should be extensive enough to
discover all regressions. Hence I'm adding check for vma size and prot
bits.

In this change: we created two fixtures:

Fixture basic: This creates a single VMA, the VMA has a PROT_NONE page at each end to prevent auto-merging.

Fixture wo_vma: Two VMAs back to end, a PROT_NONE page at each end to prevent auto-merging.

In addition, I add one test (mprotec) in each fixture to demo the tests.

[1] https://lore.kernel.org/all/20240830180237.1220027-5-jeffxu@chromium.org/ [2] https://lore.kernel.org/all/CABi2SkUgDZtJtRJe+J9UNdtZn=EQzZcbMB685P=1rR7DUhg... [3] https://lore.kernel.org/all/2qywbjb5ebtgwkh354w3lj3vhaothvubjokxq5fhyri5jeet...

Signed-off-by: Jeff Xu jeffxu@chromium.org

tools/testing/selftests/mm/.gitignore | 1 + tools/testing/selftests/mm/Makefile | 1 + tools/testing/selftests/mm/memorysealing.c | 182 +++++++++++++++++++++ tools/testing/selftests/mm/memorysealing.h | 116 +++++++++++++ tools/testing/selftests/mm/mseal_test.c | 67 +------- 5 files changed, 301 insertions(+), 66 deletions(-) create mode 100644 tools/testing/selftests/mm/memorysealing.c create mode 100644 tools/testing/selftests/mm/memorysealing.h

diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore index a51a947b2d1d..982234a99f20 100644 --- a/tools/testing/selftests/mm/.gitignore +++ b/tools/testing/selftests/mm/.gitignore @@ -57,3 +57,4 @@ hugetlb_dio pkey_sighandler_tests_32 pkey_sighandler_tests_64 guard-pages +memorysealing diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile index 93a46ac633df..08876624f46d 100644 --- a/tools/testing/selftests/mm/Makefile +++ b/tools/testing/selftests/mm/Makefile @@ -67,6 +67,7 @@ TEST_GEN_FILES += map_populate ifneq (,$(filter $(ARCH),arm64 riscv riscv64 x86 x86_64)) TEST_GEN_FILES += memfd_secret endif +TEST_GEN_FILES += memorysealing TEST_GEN_FILES += migration TEST_GEN_FILES += mkdirty TEST_GEN_FILES += mlock-random-test diff --git a/tools/testing/selftests/mm/memorysealing.c b/tools/testing/selftests/mm/memorysealing.c new file mode 100644 index 000000000000..e10032528b64 --- /dev/null +++ b/tools/testing/selftests/mm/memorysealing.c @@ -0,0 +1,182 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +#define _GNU_SOURCE +#include "../kselftest_harness.h" +#include <asm-generic/unistd.h> +#include <errno.h> +#include <syscall.h> +#include "memorysealing.h"

+/*

• • To avoid auto-merging, create a VMA with PROT_NONE pages at each end.
• • If unsuccessful, return MAP_FAILED.
• */

+static void *setup_single_address(int size, int prot)

Nitty, but size should really be an unsigned long.

sure, thanks

...

+{

• int ret;
  

• void *ptr;
  

• unsigned long page_size = getpagesize();
  

Trivial, but we store page size in self->page_size so why not pass it as a parameter instead of looking up?

sure.

...

• ptr = mmap(NULL, size + 2 * page_size, prot,
  

•         MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
  

• if (ptr == MAP_FAILED)
  

•         return MAP_FAILED;
  

• /* To avoid auto-merging, change to PROT_NONE at each end. */
  

• ret = sys_mprotect(ptr, page_size, PROT_NONE);
  

• if (ret != 0)
  

•         return MAP_FAILED;
  

• ret = sys_mprotect(ptr + size + page_size, page_size, PROT_NONE);
  

• if (ret != 0)
  

•         return MAP_FAILED;
  

• return ptr + page_size;
  

+}

This could be done more easily with a single PROT_NONE, like:

    static void *setup_single_address(unsigned long size, int prot,
                    unsigned long page_size)
    {
            void *ptr;

            /* Ensure we only merge what we intend to. */
            ptr = mmap(NULL, size + 2 * page_size, PROT_NONE,
                       MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
            if (ptr == MAP_FAILED)
                    return MAP_FAILED;

            return mmap(&ptr[page_size], size, prot,
                        MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
    }

sure, create PROT_NONE mapping first, then use mprotect to change the middle part.

It's probably worth having an equivalent freeing function to avoid leaks:

    static int free_single_address(void *ptr, unsigned long size,
                    unsigned long page_size)
    {
            /* Also free the PROT_NONE sentinels. */
            return munmap(ptr - page_size, size + 2 * page_size);

[resp 1] could you clarify? I think we won't need freeing because mapping will be sealed. You might recall, mseal() blocks munmap(), and munmap() is atomic, so the above line that you suggested will result in no-action during FIXTURE_TEARDOWN. There will be separate test cases to test munmap under sealing. In addition, the virtual address space is big enough for this test complete without worry about the out-of-memory situation. Do I miss anything ?

    }

...

+FIXTURE(basic) +{

• unsigned long page_size;
  

• unsigned long size;
  

• void *ptr;
  

+};

+/*

• • Setup for basic:
• • Single VMA with 4 pages, prot = (PROT_READ | PROT_WRITE).
• */

+FIXTURE_SETUP(basic) +{

• int prot;
  

• self->page_size = getpagesize();
  

• if (!mseal_supported())
  

•         SKIP(return, "mseal is not supported");
  

• /* Create a single VMA with 4 pages, prot as PROT_READ | PROT_WRITE. */
  

• self->size = self->page_size * 4;
  

• self->ptr = setup_single_address(self->size, PROT_READ | PROT_WRITE);
  

• EXPECT_NE(self->ptr, MAP_FAILED);
  

• EXPECT_EQ(self->size, get_vma_size(self->ptr, &prot));
  

I mean fine but this check seems a bit overkill here, we don't need to check to ensure that PROT_NONE works.

[resp 2] Those checks are there, for the same reason that test cases usually check the return value of mmap(), mprotect() in the FIXTURE_SETUP. You are right that those calls are unlikely to fail, however, test cases generally do those "housekeeping tasks" to make sure everything is OK before the actual test. If you don't want to keep those, what is the downside of keeping them ?

...

• EXPECT_EQ(PROT_READ | PROT_WRITE, prot);
  

Nitty, but this seems a bit 'Yoda style', that is placing the expectation prior to the value whose expectation we are assessing, that is perhaps it'd be clearer as:

    EXPECT_EQ(prot, PROT_READ | PROT_WRITE);

[resp 3] Both styles are used in self tests and I believe it is equally clear, I can accommodate this request for this patch.

And again, I think this is really overkill here, if the kernel is unable to do an ordinary mapping correctly we have bigger problems than an mseal bug!

please see [resp 2]

...

+};

+FIXTURE_TEARDOWN(basic) +{

Probably worth adding munmap() teardown here.

please see [resp 1]

...

+}

+FIXTURE(two_vma) +{

• unsigned long page_size;
  

• unsigned long size;
  

• void *ptr;
  

+};

+/*

• • Setup for two_vma:
• • Two consecutive VMAs, each with 2 pages.
• • The first VMA: prot = PROT_READ.
• • The second VMA: prot = (PROT_READ | PROT_WRITE).
• */

+FIXTURE_SETUP(two_vma) +{

• int prot;
  

• int ret;
  

• self->page_size = getpagesize();
  

• if (!mseal_supported())
  

•         SKIP(return, "mseal is not supported");
  

• /* Create a single VMA with 4 pages, prot as PROT_READ | PROT_WRITE. */
  

• self->size = getpagesize() * 4;
  

• self->ptr = setup_single_address(self->size, PROT_READ | PROT_WRITE);
  

• EXPECT_NE(self->ptr, MAP_FAILED);
  

• /* Use mprotect to split as two VMA. */
  

• ret = sys_mprotect(self->ptr, self->page_size * 2, PROT_READ);
  

• ASSERT_EQ(ret, 0);
  

...

• /* Verify the first VMA is 2 pages and prot bits */
  

• EXPECT_EQ(self->page_size * 2, get_vma_size(self->ptr, &prot));
  

• EXPECT_EQ(PROT_READ, prot);
  

• /* Verify the second VMA is 2 pages and prot bits */
  

• EXPECT_EQ(self->page_size * 2,
  

•         get_vma_size(self->ptr + self->page_size * 2, &prot));
  

• EXPECT_EQ(PROT_READ | PROT_WRITE, prot);
  

Again, as with the prior fixture, it seems a bit overkill, as this is essentially testing 'does mmap() and mprotect()' work. We should hope so :)

Please see [resp 2]

...

+};

+FIXTURE_TEARDOWN(two_vma) +{

Same comment as for the basic fixture - let's clean up after ourselves.

please see [resp 1]

...

+}

+/*

• • Verify mprotect is blocked.
• */

+TEST_F(basic, mprotect_basic) +{

• int ret;
  

• unsigned long size;
  

• int prot;
  

• /* Seal the mapping. */
  

• ret = sys_mseal(self->ptr, self->size, 0);
  

• ASSERT_EQ(ret, 0);
  

• /* Verify mprotect is blocked. */
  

• ret = sys_mprotect(self->ptr, self->size, PROT_READ);
  

• EXPECT_GT(0, ret);
  

Yeah this one is kinda egregious, this is genuinely hard to read. Wouldn't this be better as:

    EXPECT_NE(ret, 0);

As it explicitly says 'not equal to a success case'. Rather than 'wait 0 is _greater than_ the result which means the result is _less than_ 0 which means an error but let me check the man page can it be positive, oh ok it's -1 on error, 0 on success', which was the Lorenzo thought process... :)

[resp 4] The man page says: "On success, mprotect() and pkey_mprotect() return zero. On error, these system calls return -1, and errno is set to indicate the error."

I wll switch EXPECT_EQ(ret, -1) which is precisely what the man page says.

...

• EXPECT_EQ(EPERM, errno);
  

Let's invert these, Yoda style isn't really useful here and is a little harder to read, e.g. we should do:

    EXPECT_EQ(errno, EPERM);

please see [resp 3]

...

• /* Verify the VMA (sealed) isn't changed */
  

• size = get_vma_size(self->ptr, &prot);
  

• EXPECT_EQ(self->size, size);
  

• EXPECT_EQ(PROT_READ | PROT_WRITE, prot);
  

Again, no to Yoda :)

please see [resp 3]

I also wonder how useful this is - I guess belts + braces, and linux _can_ do partial failures, but is that something we would care about in practice?

[resp 5] Those checks will help to detect regression, such as when Pedro Falcato implemented in-loop change [1]. Liam R. Howlett also commented that mseal_test should be extensive enough to check for size and prot to avoid future regression [2]

For your question about partial failure, there will be additional tests for partial-sealed-address range, and those will check the sealed mapping only.

...

+}

+/*

• • Seal both VMAs in one mseal call.
• • Verify mprotect is blocked on both VMAs in various cases.
• */

+TEST_F(two_vma, mprotect)

I'm guessing you aren't necessarily covering _all_ mprotect cases here (though perhaps you intend to later, iteratively?)

Yes.

...

+{

• int ret;
  

• int prot;
  

• unsigned long size;
  

I think 'size' is a bit confusing as it refers to the size ascertained from the get_vma_size() later on. Perhaps call it actual_size?

To help readability, I will change size to vma_size, which corresponds to the return of get_vma_size().

Maybe worth adding:

    void *ptr = self->ptr;
    unsigned long size = self->size;
    unsigned long page_size = self->page_size;

For convenience?

This is totally optional though, maybe more effort than it's worth.

I don't think it is worth it.

...

• /* Seal both VMAs in one mseal call. */
  

• ret = sys_mseal(self->ptr, self->size, 0);
  

• ASSERT_EQ(ret, 0);
  

• /* Verify mprotect is rejected on the first VMA. */
  

• ret = sys_mprotect(self->ptr, self->page_size * 2,
  

•         PROT_READ | PROT_EXEC);
  

Why PROT_READ | PROT_EXEC? Is this arbitrary? To be thorough shouldn't you loop through all of the prot flags that you think should fail? And perhaps check what happens if you mprotect() the VMA to set it to what it already is?

[resp 6] Not arbitrary. As commented in FIXTURE_SETUP, the first VMA is PROT_READ, and the second VMA is PROT_READ | PROT_WRITE. The test wants to use mprotect to change the proc bit to a different set.

For your question about mprotect() the sealed vma to what it already is (doesn't change any attribute), I don't want to solidify the current behavior (which is to block). It should be successful instead. With in-loop check and partial update behavior, there is no reason to block it, i.e. no security benefit. I already plan to address this.

...

• EXPECT_GT(0, ret);
  

Please convert to EXPECT_NE(ret, 0)

please see [resp 4]

...

• EXPECT_EQ(EPERM, errno);
  

Please invert to EXPECT_EQ(errno, EPERM)

please see [resp 3]

...

• /* Verify mprotect is rejected on the second VMA. */
  

• ret = sys_mprotect(self->ptr, self->page_size * 2,
  

•         PROT_READ | PROT_EXEC);
  

Hang on, isn't this just a repeat of the first test? Shouldn't this be:

    ret = sys_mprotect(&self->ptr[self->page_size * 2],
            self->page_size * 2, PROT_READ | PROT_EXEC);

[resp 7] Not the same, this test has two VMA, and the mseal() covers both VMAs in one call. In the first test, i.e. basic, mprotect_basic, mseal is called on single VMA.

...

• EXPECT_GT(0, ret);
  

• EXPECT_EQ(EPERM, errno);
  

Same comment as above re: ordering.

please see [resp 3]

...

• /* Attempt of mprotect two VMAs at the same call is blocked */
  

• ret = sys_mprotect(self->ptr, self->size,
  

•         PROT_READ | PROT_EXEC);
  

Same comment about iterating through prot flags that should fail as above.

please see [resp 7]

...

• EXPECT_GT(0, ret);
  

• EXPECT_EQ(EPERM, errno);
  

Same comment as above re: ordering.

please see [resp 3]

...

• /* Verify both VMAs aren't changed. */
  

• size = get_vma_size(self->ptr, &prot);
  

• EXPECT_EQ(self->page_size * 2, size);
  

• EXPECT_EQ(PROT_READ, prot);
  

Same comment as above re: ordering.

please see [resp 3]

...

• size = get_vma_size(self->ptr + self->page_size * 2, &prot);
  

• EXPECT_EQ(self->page_size * 2, size);
  

• EXPECT_EQ(PROT_READ | PROT_WRITE, prot);
  

Same comment as above re: ordering.

please see [resp 3]

...

+}

+TEST_HARNESS_MAIN diff --git a/tools/testing/selftests/mm/memorysealing.h b/tools/testing/selftests/mm/memorysealing.h new file mode 100644 index 000000000000..aee6e6700028 --- /dev/null +++ b/tools/testing/selftests/mm/memorysealing.h @@ -0,0 +1,116 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#include <syscall.h>

+/*

• • Define sys_xyx to call syscall directly.
• • This is needed because we want to avoid calling glibc and
• • test syscall directly.

I do wonder if this is worth it, but I mean, I guess it's not too egregious if it's wrapped like this. Same comment for other wrappers.

The self-test should directly test the kernel syscall without any intermediate layer. selftest is not a libc test. It is practical too, glibc's behavior can change across versions which result inconsistent, and Padro Falcato reported one of this and fixed in [3]

...

• • The only exception is mmap, which _NR_mmap2 is not defined for
• • some ARM architecture.
• */

+static inline int sys_mseal(void *start, size_t len, unsigned long flags) +{

• int sret;
  

• errno = 0;
  

Why are we setting this to 0? Same comment for all sys_XXX() wrappers.

The test is only interested in the latest error associated with the failed syscall, and I'm not sure the kernel clears the errno for each system call. Note, there is no glibc layer here.

...

• sret = syscall(__NR_mseal, start, len, flags);
  

• return sret;
  

Nit, but this seems a bit redundant, why not just drop the sret assignment, and simply:

    return syscall(__NR_mseal, start, len, flags);

Same comment for all sys_XXX() wrappers.

[resp 6] I think this is a different taste in coding style. For the test, I prefer to have local variables so it is easy to show them in the debugger, when needed.

...

+}

+static inline int sys_mprotect(void *ptr, size_t size, unsigned long prot) +{

• int sret;
  

• errno = 0;
  

• sret = syscall(__NR_mprotect, ptr, size, prot);
  

• return sret;
  

+}

The wrappers below don't seem to be used yet, could we delay putting them in until they are actually used?

Sure, I will remove sys_mprotect_pkey for this patch.

...

+static inline int sys_mprotect_pkey(void *ptr, size_t size,

• unsigned long orig_prot, unsigned long pkey)
  

+{

• int sret;
  

• errno = 0;
  

• sret = syscall(__NR_pkey_mprotect, ptr, size, orig_prot, pkey);
  

• return sret;
  

+}

+static inline int sys_munmap(void *ptr, size_t size) +{

• int sret;
  

• errno = 0;
  

• sret = syscall(__NR_munmap, ptr, size);
  

• return sret;
  

+}

+static inline int sys_madvise(void *start, size_t len, int types) +{

• int sret;
  

• errno = 0;
  

• sret = syscall(__NR_madvise, start, len, types);
  

• return sret;
  

+}

+static inline void *sys_mremap(void *addr, size_t old_len, size_t new_len,

• unsigned long flags, void *new_addr)
  

+{

• void *sret;
  

• errno = 0;
  

• sret = (void *) syscall(__NR_mremap, addr, old_len, new_len, flags, new_addr);
  

• return sret;
  

+}

+/*

• • Parsing /proc/self/maps to get VMA's size and prot bit.
• */

+static unsigned long get_vma_size(void *addr, int *prot) +{

• FILE *maps;
  

• char line[256];
  

• int size = 0;
  

• uintptr_t  addr_start, addr_end;
  

Why not add a:

    uintptr_t addr_val = (uintptr_t)addr;

To avoid having to constantly cast below?

sure.

...

• char protstr[5];
  

• *prot = 0;
  

Nit but not sure there's much point in setting this to 0 even in failure cases?

With an error code return you can avoid having to do this.

[resp 8] I don't think the error code return is worth the effort, it currently returns 0 size in case of error or not-found. For easy debug parsing errors, I can add logs. Notes, the parsing should always be successful.

...

• maps = fopen("/proc/self/maps", "r");
  

• if (!maps)
  

•         return 0;
  

Might it be better to return an error code or something so we can identify this case? I guess any sensible call will fail on a 0 but you're sort of just implicitly hoping for this.

[resp 9] we could add logging in case of error.

...

• while (fgets(line, sizeof(line), maps)) {
  

•         if (sscanf(line, "%lx-%lx %4s", &addr_start, &addr_end, protstr) == 3) {
  

•                 if (addr_start == (uintptr_t) addr) {
  

This is a Yoda comparison again, let's put what we're comparing first, and the thing we're comparing to afterwards, e.g.

    if ((uintptr_t)addr == addr_start) ...

addr is the expected value from input and addr_start changes after parsing each line. The expected value (addr) is in second position, why is this yoda syntax ? IMO, introducing the yoda syntax concept for "==" is more confusing than useful, A==B is as clear as B==A.

Also I don't think a space after the (type) cast is necesary.

sure

...

•                         size = addr_end - addr_start;
  

•                         if (protstr[0] == 'r')
  

•                                 *prot |= PROT_READ;
  

•                         if (protstr[1] == 'w')
  

•                                 *prot |= PROT_WRITE;
  

•                         if (protstr[2] == 'x')
  

•                                 *prot |= PROT_EXEC;
  

•                         break;
  

•                 }
  

•         }
  

• }
  

On the impl: This would be neater if you used guard clauses:

    while (fgets(line, sizeof(line), maps)) {
            if (sscanf(line, "%lx-%lx %4s", &addr_start, &addr_end, protstr) != 3)
                    continue;

            if (addr_val != addr_start)
                    continue;

            size = addr_end - addr_start;
            if (protstr[0] == 'r')
                    *prot |= PROT_READ;
            if (protstr[1] == 'w')
                    *prot |= PROT_WRITE;
            if (protstr[2] == 'x')
                    *prot |= PROT_EXEC;
            break;
    }

This is a lot more readable?

sure.

On doing this at all - it feels a bit crazy to check this, but I guess it might be the only way to assert things about merge/split.

I'm not sure it's _really useful_ to do that though - merges/splits are internal implementation details in effect (modulo mremap single VMA requirements), and mseal is an external interface, really the check should be around 'can forbidden operations be performed on sealed mappings'.

This is related to resp [5]. This test not only verifies the size, but also verifies the prot bits. The argument that VMA is an internal behavior is questionable, the /proc/pid/maps is user-facing and applications can read it and parse it.

However, if you do really want to do this, I think it would be much better to use the new ioctl interface for this [0] which should avoid having to do unpleasant text wrangling :)

I could try this. This is a new feature though, I do think this selftest should be self-standing and has as minimal dependency as possible. Is there harm of using /proc/pid/maps ? parsing /proc/pid/maps already used by selftest in lots of places (see below), I would prefer testing to use well-established patterns than relying on a newly developed feature.

filesystems/overlayfs/dev_in_maps.c mm/mlock2-tests.c mm/mremap_test.c mm/virtual_address_range.c mm/split_huge_page_test.c filesystems/overlayfs/dev_in_maps.c exec/load_address.c bpf/trace_helpers.c

...

• fclose(maps);
  

• return size;
  

If we don't find the line we don't seem to be indicating this, again worth being able to pass back an error?

It can be normal that we don't find a line, i.e. tests want to verify that a VMA does not exist at address, return 0 size is correct in this case. If a parsing error ever happened, the test would have failed at FIXTURE_SETUP to begin with.

...

+}

+static inline bool mseal_supported(void) +{

• int ret;
  

• void *ptr;
  

• unsigned long page_size = getpagesize();
  

• ptr = mmap(NULL, page_size, PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
  

• if (ptr == MAP_FAILED)
  

•         return false;
  

• ret = sys_mseal(ptr, page_size, 0);
  

• if (ret < 0)
  

•         return false;
  

This is kind of cute, I mean it speaks to the need I think for us to be able to expose 'is X available?' programmatically. But I digress.

I guess this is ok, as this is something that should never fail.

You might recall that mseal is not available on the 32 bits kernel, automation might run the mseal_test on a 32 bit build. mseal_supported() detects this so tests can be skipped.

But we should definitely clean up after ourselves here whether this succeeds or fails.

please see resp [1].

Thanks! -Jeff [1] https://lore.kernel.org/all/CAKbZUD31EK2ah=vWJ46y4nL9OygzAa6ZxPnPHmWYO-sop5t... [2] https://lore.kernel.org/all/ko3pjllsgufbz33hqvwdpdsyxvgrgzqiodxexnpcng3mssff... [3] https://lore.kernel.org/all/20240807153544.2754247-1-jeffxu@chromium.org/

...

• return true;
  

+} diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c index ad17005521a8..8dd20341de3d 100644 --- a/tools/testing/selftests/mm/mseal_test.c +++ b/tools/testing/selftests/mm/mseal_test.c @@ -517,30 +517,6 @@ static void test_seal_twice(void) REPORT_TEST_PASS(); }

-static void test_seal_mprotect(bool seal) -{

• void *ptr;
  

• unsigned long page_size = getpagesize();
  

• unsigned long size = 4 * page_size;
  

• int ret;
  

• setup_single_address(size, &ptr);
  

• FAIL_TEST_IF_FALSE(ptr != (void *)-1);
  

• if (seal) {
  

•         ret = seal_single_address(ptr, size);
  

•         FAIL_TEST_IF_FALSE(!ret);
  

• }
  

• ret = sys_mprotect(ptr, size, PROT_READ | PROT_WRITE);
  

• if (seal)
  

•         FAIL_TEST_IF_FALSE(ret < 0);
  

• else
  

•         FAIL_TEST_IF_FALSE(!ret);
  

• REPORT_TEST_PASS();
  

-}

static void test_seal_start_mprotect(bool seal) { void *ptr; @@ -658,41 +634,6 @@ static void test_seal_mprotect_unalign_len_variant_2(bool seal) REPORT_TEST_PASS(); }

-static void test_seal_mprotect_two_vma(bool seal) -{

• void *ptr;
  

• unsigned long page_size = getpagesize();
  

• unsigned long size = 4 * page_size;
  

• int ret;
  

• setup_single_address(size, &ptr);
  

• FAIL_TEST_IF_FALSE(ptr != (void *)-1);
  

• /* use mprotect to split */
  

• ret = sys_mprotect(ptr, page_size * 2, PROT_READ | PROT_WRITE);
  

• FAIL_TEST_IF_FALSE(!ret);
  

• if (seal) {
  

•         ret = seal_single_address(ptr, page_size * 4);
  

•         FAIL_TEST_IF_FALSE(!ret);
  

• }
  

• ret = sys_mprotect(ptr, page_size * 2, PROT_READ | PROT_WRITE);
  

• if (seal)
  

•         FAIL_TEST_IF_FALSE(ret < 0);
  

• else
  

•         FAIL_TEST_IF_FALSE(!ret);
  

• ret = sys_mprotect(ptr + page_size * 2, page_size * 2,
  

•                 PROT_READ | PROT_WRITE);
  

• if (seal)
  

•         FAIL_TEST_IF_FALSE(ret < 0);
  

• else
  

•         FAIL_TEST_IF_FALSE(!ret);
  

• REPORT_TEST_PASS();
  

-}

static void test_seal_mprotect_two_vma_with_split(bool seal) { void *ptr; @@ -1876,7 +1817,7 @@ int main(void) if (!pkey_supported()) ksft_print_msg("PKEY not supported\n");

• ksft_set_plan(88);
  

• ksft_set_plan(84);
  
  test_seal_addseal();
  test_seal_unmapped_start();
  

@@ -1889,9 +1830,6 @@ int main(void) test_seal_zero_length(); test_seal_twice();

• test_seal_mprotect(false);
  

• test_seal_mprotect(true);
  

• test_seal_start_mprotect(false);
  test_seal_start_mprotect(true);
  
  

@@ -1904,9 +1842,6 @@ int main(void) test_seal_mprotect_unalign_len_variant_2(false); test_seal_mprotect_unalign_len_variant_2(true);

• test_seal_mprotect_two_vma(false);
  

• test_seal_mprotect_two_vma(true);
  

• test_seal_mprotect_two_vma_with_split(false);
  test_seal_mprotect_two_vma_with_split(true);
  
  

-- 2.47.1.613.gc27f4b7a9f-goog

On Mon, Feb 10, 2025 at 01:35:52PM -0800, Jeff Xu wrote:

Hi Lorenzo,

Gentle ping for my clarification questions.

I also tried the new ioctl PROCMAP_QUERY, please see below for details.

Hi Jeff,

Sorry I thought you'd send a new version which is why I didn't reply. I will take a look through and get back to you, it's on my whiteboard now...

Apologies for the delay, this was just an error!

And thanks for taking a look at the ioctl, will take a look and that when I get a sec also.

Cheers, Lorenzo

On Wed, Jan 15, 2025 at 12:47 PM Jeff Xu jeffxu@chromium.org wrote:

...

Hi Lorenzo,

On Thu, Jan 2, 2025 at 9:30 AM Lorenzo Stoakes lorenzo.stoakes@oracle.com wrote:

...

Sorry for delay, was super busy leading up to xmas break, then had ~2.5 weeks off.

Thanks for reviewing. There are lots of comments, so it takes some time to go through comments and experiment with some of the suggested changes. Please see below.

...

And happy new year! :)

Happy new year !

...

On Wed, Dec 11, 2024 at 05:33:11AM +0000, jeffxu@chromium.org wrote:

...

From: Jeff Xu jeffxu@chromium.org

This change creates the initial version of memorysealing.c.

The introduction of memorysealing.c, which replaces mseal_test.c and

I mean I don't want to be a pain but I would kinda prefer to have 'mseal' everywhere mseal is to avoid confusion vs. memfd seals.

As I tried to explain, the memorysealing.c will eventually replace mseal_test.c, after all the tests are moved to use kselftest_karness. How about mseal_test_new.c ? I'm open to new names if you have one.

...

Of course we in the kernel absolutely _love_ to overload the meaning of terms but some traditions are worth breaking :)

...

uses the kselftest_harness, aims to initiate a discussion on using the selftest harness for memory sealing tests. Upon approval of this approach, the migration of tests from mseal_test.c to memorysealing.c can be implemented in a step-by-step manner.

Well, I for one like this approach so (unsurprisingly) :) but perhaps sensible to do it iteratively so we can also tweak things as we go.

Yes, that is the idea.

...

...

This tests addresses following feedback from previous reviews:

1> Use kselftest_harness instead of custom macro, such as EXPECT_XX, ASSERT_XX, etc. (Lorenzo Stoakes, Mark Brown) [1]

2> Use MAP_FAILED to check the return of mmap (Lorenzo Stoakes).

Thanks!

...

3> Adding a check for vma size and prot bits. The discussion for this can be found in [2] [3], here is a brief summary: This is to follow up on Pedro’s in-loop change (from can_modify_mm to can_modify_vma). When mseal_test is initially created, they have a common pattern: setup memory layout, seal the memory, perform a few mm-api steps, verify return code (not zero). Because of the nature of out-of-loop, it is sufficient to just verify the error code in a few cases.

With Pedro's in-loop change, the sealing check happens later in the
stack, thus there are more things and scenarios to verify. And there
was feedbacks to me that mseal_test should be extensive enough to
discover all regressions. Hence I'm adding check for vma size and prot
bits.

In this change: we created two fixtures:

Fixture basic: This creates a single VMA, the VMA has a PROT_NONE page at each end to prevent auto-merging.

Fixture wo_vma: Two VMAs back to end, a PROT_NONE page at each end to prevent auto-merging.

In addition, I add one test (mprotec) in each fixture to demo the tests.

[1] https://lore.kernel.org/all/20240830180237.1220027-5-jeffxu@chromium.org/ [2] https://lore.kernel.org/all/CABi2SkUgDZtJtRJe+J9UNdtZn=EQzZcbMB685P=1rR7DUhg... [3] https://lore.kernel.org/all/2qywbjb5ebtgwkh354w3lj3vhaothvubjokxq5fhyri5jeet...

Signed-off-by: Jeff Xu jeffxu@chromium.org

tools/testing/selftests/mm/.gitignore | 1 + tools/testing/selftests/mm/Makefile | 1 + tools/testing/selftests/mm/memorysealing.c | 182 +++++++++++++++++++++ tools/testing/selftests/mm/memorysealing.h | 116 +++++++++++++ tools/testing/selftests/mm/mseal_test.c | 67 +------- 5 files changed, 301 insertions(+), 66 deletions(-) create mode 100644 tools/testing/selftests/mm/memorysealing.c create mode 100644 tools/testing/selftests/mm/memorysealing.h

diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore index a51a947b2d1d..982234a99f20 100644 --- a/tools/testing/selftests/mm/.gitignore +++ b/tools/testing/selftests/mm/.gitignore @@ -57,3 +57,4 @@ hugetlb_dio pkey_sighandler_tests_32 pkey_sighandler_tests_64 guard-pages +memorysealing diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile index 93a46ac633df..08876624f46d 100644 --- a/tools/testing/selftests/mm/Makefile +++ b/tools/testing/selftests/mm/Makefile @@ -67,6 +67,7 @@ TEST_GEN_FILES += map_populate ifneq (,$(filter $(ARCH),arm64 riscv riscv64 x86 x86_64)) TEST_GEN_FILES += memfd_secret endif +TEST_GEN_FILES += memorysealing TEST_GEN_FILES += migration TEST_GEN_FILES += mkdirty TEST_GEN_FILES += mlock-random-test diff --git a/tools/testing/selftests/mm/memorysealing.c b/tools/testing/selftests/mm/memorysealing.c new file mode 100644 index 000000000000..e10032528b64 --- /dev/null +++ b/tools/testing/selftests/mm/memorysealing.c @@ -0,0 +1,182 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +#define _GNU_SOURCE +#include "../kselftest_harness.h" +#include <asm-generic/unistd.h> +#include <errno.h> +#include <syscall.h> +#include "memorysealing.h"

+/*

• • To avoid auto-merging, create a VMA with PROT_NONE pages at each end.
• • If unsuccessful, return MAP_FAILED.
• */

+static void *setup_single_address(int size, int prot)

Nitty, but size should really be an unsigned long.

sure, thanks

...

...

+{

• int ret;
  

• void *ptr;
  

• unsigned long page_size = getpagesize();
  

Trivial, but we store page size in self->page_size so why not pass it as a parameter instead of looking up?

sure.

...

...

• ptr = mmap(NULL, size + 2 * page_size, prot,
  

•         MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
  

• if (ptr == MAP_FAILED)
  

•         return MAP_FAILED;
  

• /* To avoid auto-merging, change to PROT_NONE at each end. */
  

• ret = sys_mprotect(ptr, page_size, PROT_NONE);
  

• if (ret != 0)
  

•         return MAP_FAILED;
  

• ret = sys_mprotect(ptr + size + page_size, page_size, PROT_NONE);
  

• if (ret != 0)
  

•         return MAP_FAILED;
  

• return ptr + page_size;
  

+}

This could be done more easily with a single PROT_NONE, like:

    static void *setup_single_address(unsigned long size, int prot,
                    unsigned long page_size)
    {
            void *ptr;

            /* Ensure we only merge what we intend to. */
            ptr = mmap(NULL, size + 2 * page_size, PROT_NONE,
                       MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
            if (ptr == MAP_FAILED)
                    return MAP_FAILED;

            return mmap(&ptr[page_size], size, prot,
                        MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
    }

sure, create PROT_NONE mapping first, then use mprotect to change the middle part.

...

It's probably worth having an equivalent freeing function to avoid leaks:

    static int free_single_address(void *ptr, unsigned long size,
                    unsigned long page_size)
    {
            /* Also free the PROT_NONE sentinels. */
            return munmap(ptr - page_size, size + 2 * page_size);

[resp 1] could you clarify? I think we won't need freeing because mapping will be sealed. You might recall, mseal() blocks munmap(), and munmap() is atomic, so the above line that you suggested will result in no-action during FIXTURE_TEARDOWN. There will be separate test cases to test munmap under sealing. In addition, the virtual address space is big enough for this test complete without worry about the out-of-memory situation. Do I miss anything ?

...

    }

...

+FIXTURE(basic) +{

• unsigned long page_size;
  

• unsigned long size;
  

• void *ptr;
  

+};

+/*

• • Setup for basic:
• • Single VMA with 4 pages, prot = (PROT_READ | PROT_WRITE).
• */

+FIXTURE_SETUP(basic) +{

• int prot;
  

• self->page_size = getpagesize();
  

• if (!mseal_supported())
  

•         SKIP(return, "mseal is not supported");
  

• /* Create a single VMA with 4 pages, prot as PROT_READ | PROT_WRITE. */
  

• self->size = self->page_size * 4;
  

• self->ptr = setup_single_address(self->size, PROT_READ | PROT_WRITE);
  

• EXPECT_NE(self->ptr, MAP_FAILED);
  

• EXPECT_EQ(self->size, get_vma_size(self->ptr, &prot));
  

I mean fine but this check seems a bit overkill here, we don't need to check to ensure that PROT_NONE works.

[resp 2] Those checks are there, for the same reason that test cases usually check the return value of mmap(), mprotect() in the FIXTURE_SETUP. You are right that those calls are unlikely to fail, however, test cases generally do those "housekeeping tasks" to make sure everything is OK before the actual test. If you don't want to keep those, what is the downside of keeping them ?

...

...

• EXPECT_EQ(PROT_READ | PROT_WRITE, prot);
  

Nitty, but this seems a bit 'Yoda style', that is placing the expectation prior to the value whose expectation we are assessing, that is perhaps it'd be clearer as:

    EXPECT_EQ(prot, PROT_READ | PROT_WRITE);

[resp 3] Both styles are used in self tests and I believe it is equally clear, I can accommodate this request for this patch.

...

And again, I think this is really overkill here, if the kernel is unable to do an ordinary mapping correctly we have bigger problems than an mseal bug!

please see [resp 2]

...

...

+};

+FIXTURE_TEARDOWN(basic) +{

Probably worth adding munmap() teardown here.

please see [resp 1]

...

...

+}

+FIXTURE(two_vma) +{

• unsigned long page_size;
  

• unsigned long size;
  

• void *ptr;
  

+};

+/*

• • Setup for two_vma:
• • Two consecutive VMAs, each with 2 pages.
• • The first VMA: prot = PROT_READ.
• • The second VMA: prot = (PROT_READ | PROT_WRITE).
• */

+FIXTURE_SETUP(two_vma) +{

• int prot;
  

• int ret;
  

• self->page_size = getpagesize();
  

• if (!mseal_supported())
  

•         SKIP(return, "mseal is not supported");
  

• /* Create a single VMA with 4 pages, prot as PROT_READ | PROT_WRITE. */
  

• self->size = getpagesize() * 4;
  

• self->ptr = setup_single_address(self->size, PROT_READ | PROT_WRITE);
  

• EXPECT_NE(self->ptr, MAP_FAILED);
  

• /* Use mprotect to split as two VMA. */
  

• ret = sys_mprotect(self->ptr, self->page_size * 2, PROT_READ);
  

• ASSERT_EQ(ret, 0);
  

...

• /* Verify the first VMA is 2 pages and prot bits */
  

• EXPECT_EQ(self->page_size * 2, get_vma_size(self->ptr, &prot));
  

• EXPECT_EQ(PROT_READ, prot);
  

• /* Verify the second VMA is 2 pages and prot bits */
  

• EXPECT_EQ(self->page_size * 2,
  

•         get_vma_size(self->ptr + self->page_size * 2, &prot));
  

• EXPECT_EQ(PROT_READ | PROT_WRITE, prot);
  

Again, as with the prior fixture, it seems a bit overkill, as this is essentially testing 'does mmap() and mprotect()' work. We should hope so :)

Please see [resp 2]

...

...

+};

+FIXTURE_TEARDOWN(two_vma) +{

Same comment as for the basic fixture - let's clean up after ourselves.

please see [resp 1]

...

...

+}

+/*

• • Verify mprotect is blocked.
• */

+TEST_F(basic, mprotect_basic) +{

• int ret;
  

• unsigned long size;
  

• int prot;
  

• /* Seal the mapping. */
  

• ret = sys_mseal(self->ptr, self->size, 0);
  

• ASSERT_EQ(ret, 0);
  

• /* Verify mprotect is blocked. */
  

• ret = sys_mprotect(self->ptr, self->size, PROT_READ);
  

• EXPECT_GT(0, ret);
  

Yeah this one is kinda egregious, this is genuinely hard to read. Wouldn't this be better as:

    EXPECT_NE(ret, 0);

As it explicitly says 'not equal to a success case'. Rather than 'wait 0 is _greater than_ the result which means the result is _less than_ 0 which means an error but let me check the man page can it be positive, oh ok it's -1 on error, 0 on success', which was the Lorenzo thought process... :)

[resp 4] The man page says: "On success, mprotect() and pkey_mprotect() return zero. On error, these system calls return -1, and errno is set to indicate the error."

I wll switch EXPECT_EQ(ret, -1) which is precisely what the man page says.

...

...

• EXPECT_EQ(EPERM, errno);
  

Let's invert these, Yoda style isn't really useful here and is a little harder to read, e.g. we should do:

    EXPECT_EQ(errno, EPERM);

please see [resp 3]

...

...

• /* Verify the VMA (sealed) isn't changed */
  

• size = get_vma_size(self->ptr, &prot);
  

• EXPECT_EQ(self->size, size);
  

• EXPECT_EQ(PROT_READ | PROT_WRITE, prot);
  

Again, no to Yoda :)

please see [resp 3]

...

I also wonder how useful this is - I guess belts + braces, and linux _can_ do partial failures, but is that something we would care about in practice?

[resp 5] Those checks will help to detect regression, such as when Pedro Falcato implemented in-loop change [1]. Liam R. Howlett also commented that mseal_test should be extensive enough to check for size and prot to avoid future regression [2]

For your question about partial failure, there will be additional tests for partial-sealed-address range, and those will check the sealed mapping only.

...

...

+}

+/*

• • Seal both VMAs in one mseal call.
• • Verify mprotect is blocked on both VMAs in various cases.
• */

+TEST_F(two_vma, mprotect)

I'm guessing you aren't necessarily covering _all_ mprotect cases here (though perhaps you intend to later, iteratively?)

Yes.

...

...

+{

• int ret;
  

• int prot;
  

• unsigned long size;
  

I think 'size' is a bit confusing as it refers to the size ascertained from the get_vma_size() later on. Perhaps call it actual_size?

To help readability, I will change size to vma_size, which corresponds to the return of get_vma_size().

...

Maybe worth adding:

    void *ptr = self->ptr;
    unsigned long size = self->size;
    unsigned long page_size = self->page_size;

For convenience?

This is totally optional though, maybe more effort than it's worth.

I don't think it is worth it.

...

...

• /* Seal both VMAs in one mseal call. */
  

• ret = sys_mseal(self->ptr, self->size, 0);
  

• ASSERT_EQ(ret, 0);
  

• /* Verify mprotect is rejected on the first VMA. */
  

• ret = sys_mprotect(self->ptr, self->page_size * 2,
  

•         PROT_READ | PROT_EXEC);
  

Why PROT_READ | PROT_EXEC? Is this arbitrary? To be thorough shouldn't you loop through all of the prot flags that you think should fail? And perhaps check what happens if you mprotect() the VMA to set it to what it already is?

[resp 6] Not arbitrary. As commented in FIXTURE_SETUP, the first VMA is PROT_READ, and the second VMA is PROT_READ | PROT_WRITE. The test wants to use mprotect to change the proc bit to a different set.

For your question about mprotect() the sealed vma to what it already is (doesn't change any attribute), I don't want to solidify the current behavior (which is to block). It should be successful instead. With in-loop check and partial update behavior, there is no reason to block it, i.e. no security benefit. I already plan to address this.

...

...

• EXPECT_GT(0, ret);
  

Please convert to EXPECT_NE(ret, 0)

please see [resp 4]

...

...

• EXPECT_EQ(EPERM, errno);
  

Please invert to EXPECT_EQ(errno, EPERM)

please see [resp 3]

...

...

• /* Verify mprotect is rejected on the second VMA. */
  

• ret = sys_mprotect(self->ptr, self->page_size * 2,
  

•         PROT_READ | PROT_EXEC);
  

Hang on, isn't this just a repeat of the first test? Shouldn't this be:

    ret = sys_mprotect(&self->ptr[self->page_size * 2],
            self->page_size * 2, PROT_READ | PROT_EXEC);

[resp 7] Not the same, this test has two VMA, and the mseal() covers both VMAs in one call. In the first test, i.e. basic, mprotect_basic, mseal is called on single VMA.

...

...

• EXPECT_GT(0, ret);
  

• EXPECT_EQ(EPERM, errno);
  

Same comment as above re: ordering.

please see [resp 3]

...

...

• /* Attempt of mprotect two VMAs at the same call is blocked */
  

• ret = sys_mprotect(self->ptr, self->size,
  

•         PROT_READ | PROT_EXEC);
  

Same comment about iterating through prot flags that should fail as above.

please see [resp 7]

...

...

• EXPECT_GT(0, ret);
  

• EXPECT_EQ(EPERM, errno);
  

Same comment as above re: ordering.

please see [resp 3]

...

...

• /* Verify both VMAs aren't changed. */
  

• size = get_vma_size(self->ptr, &prot);
  

• EXPECT_EQ(self->page_size * 2, size);
  

• EXPECT_EQ(PROT_READ, prot);
  

Same comment as above re: ordering.

please see [resp 3]

...

...

• size = get_vma_size(self->ptr + self->page_size * 2, &prot);
  

• EXPECT_EQ(self->page_size * 2, size);
  

• EXPECT_EQ(PROT_READ | PROT_WRITE, prot);
  

Same comment as above re: ordering.

please see [resp 3]

...

...

+}

+TEST_HARNESS_MAIN diff --git a/tools/testing/selftests/mm/memorysealing.h b/tools/testing/selftests/mm/memorysealing.h new file mode 100644 index 000000000000..aee6e6700028 --- /dev/null +++ b/tools/testing/selftests/mm/memorysealing.h @@ -0,0 +1,116 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#include <syscall.h>

+/*

• • Define sys_xyx to call syscall directly.
• • This is needed because we want to avoid calling glibc and
• • test syscall directly.

I do wonder if this is worth it, but I mean, I guess it's not too egregious if it's wrapped like this. Same comment for other wrappers.

The self-test should directly test the kernel syscall without any intermediate layer. selftest is not a libc test. It is practical too, glibc's behavior can change across versions which result inconsistent, and Padro Falcato reported one of this and fixed in [3]

...

...

• • The only exception is mmap, which _NR_mmap2 is not defined for
• • some ARM architecture.
• */

+static inline int sys_mseal(void *start, size_t len, unsigned long flags) +{

• int sret;
  

• errno = 0;
  

Why are we setting this to 0? Same comment for all sys_XXX() wrappers.

The test is only interested in the latest error associated with the failed syscall, and I'm not sure the kernel clears the errno for each system call. Note, there is no glibc layer here.

...

...

• sret = syscall(__NR_mseal, start, len, flags);
  

• return sret;
  

Nit, but this seems a bit redundant, why not just drop the sret assignment, and simply:

    return syscall(__NR_mseal, start, len, flags);

Same comment for all sys_XXX() wrappers.

[resp 6] I think this is a different taste in coding style. For the test, I prefer to have local variables so it is easy to show them in the debugger, when needed.

...

...

+}

+static inline int sys_mprotect(void *ptr, size_t size, unsigned long prot) +{

• int sret;
  

• errno = 0;
  

• sret = syscall(__NR_mprotect, ptr, size, prot);
  

• return sret;
  

+}

The wrappers below don't seem to be used yet, could we delay putting them in until they are actually used?

Sure, I will remove sys_mprotect_pkey for this patch.

...

...

+static inline int sys_mprotect_pkey(void *ptr, size_t size,

• unsigned long orig_prot, unsigned long pkey)
  

+{

• int sret;
  

• errno = 0;
  

• sret = syscall(__NR_pkey_mprotect, ptr, size, orig_prot, pkey);
  

• return sret;
  

+}

+static inline int sys_munmap(void *ptr, size_t size) +{

• int sret;
  

• errno = 0;
  

• sret = syscall(__NR_munmap, ptr, size);
  

• return sret;
  

+}

+static inline int sys_madvise(void *start, size_t len, int types) +{

• int sret;
  

• errno = 0;
  

• sret = syscall(__NR_madvise, start, len, types);
  

• return sret;
  

+}

+static inline void *sys_mremap(void *addr, size_t old_len, size_t new_len,

• unsigned long flags, void *new_addr)
  

+{

• void *sret;
  

• errno = 0;
  

• sret = (void *) syscall(__NR_mremap, addr, old_len, new_len, flags, new_addr);
  

• return sret;
  

+}

+/*

• • Parsing /proc/self/maps to get VMA's size and prot bit.
• */

+static unsigned long get_vma_size(void *addr, int *prot) +{

• FILE *maps;
  

• char line[256];
  

• int size = 0;
  

• uintptr_t  addr_start, addr_end;
  

Why not add a:

    uintptr_t addr_val = (uintptr_t)addr;

To avoid having to constantly cast below?

sure.

...

...

• char protstr[5];
  

• *prot = 0;
  

Nit but not sure there's much point in setting this to 0 even in failure cases?

With an error code return you can avoid having to do this.

[resp 8] I don't think the error code return is worth the effort, it currently returns 0 size in case of error or not-found. For easy debug parsing errors, I can add logs. Notes, the parsing should always be successful.

...

...

• maps = fopen("/proc/self/maps", "r");
  

• if (!maps)
  

•         return 0;
  

Might it be better to return an error code or something so we can identify this case? I guess any sensible call will fail on a 0 but you're sort of just implicitly hoping for this.

[resp 9] we could add logging in case of error.

...

...

• while (fgets(line, sizeof(line), maps)) {
  

•         if (sscanf(line, "%lx-%lx %4s", &addr_start, &addr_end, protstr) == 3) {
  

•                 if (addr_start == (uintptr_t) addr) {
  

This is a Yoda comparison again, let's put what we're comparing first, and the thing we're comparing to afterwards, e.g.

    if ((uintptr_t)addr == addr_start) ...

addr is the expected value from input and addr_start changes after parsing each line. The expected value (addr) is in second position, why is this yoda syntax ? IMO, introducing the yoda syntax concept for "==" is more confusing than useful, A==B is as clear as B==A.

...

Also I don't think a space after the (type) cast is necesary.

sure

...

...

•                         size = addr_end - addr_start;
  

•                         if (protstr[0] == 'r')
  

•                                 *prot |= PROT_READ;
  

•                         if (protstr[1] == 'w')
  

•                                 *prot |= PROT_WRITE;
  

•                         if (protstr[2] == 'x')
  

•                                 *prot |= PROT_EXEC;
  

•                         break;
  

•                 }
  

•         }
  

• }
  

On the impl: This would be neater if you used guard clauses:

    while (fgets(line, sizeof(line), maps)) {
            if (sscanf(line, "%lx-%lx %4s", &addr_start, &addr_end, protstr) != 3)
                    continue;

            if (addr_val != addr_start)
                    continue;

            size = addr_end - addr_start;
            if (protstr[0] == 'r')
                    *prot |= PROT_READ;
            if (protstr[1] == 'w')
                    *prot |= PROT_WRITE;
            if (protstr[2] == 'x')
                    *prot |= PROT_EXEC;
            break;
    }

This is a lot more readable?

sure.

...

On doing this at all - it feels a bit crazy to check this, but I guess it might be the only way to assert things about merge/split.

I'm not sure it's _really useful_ to do that though - merges/splits are internal implementation details in effect (modulo mremap single VMA requirements), and mseal is an external interface, really the check should be around 'can forbidden operations be performed on sealed mappings'.

This is related to resp [5]. This test not only verifies the size, but also verifies the prot bits. The argument that VMA is an internal behavior is questionable, the /proc/pid/maps is user-facing and applications can read it and parse it.

...

However, if you do really want to do this, I think it would be much better to use the new ioctl interface for this [0] which should avoid having to do unpleasant text wrangling :)

I could try this. This is a new feature though, I do think this selftest should be self-standing and has as minimal dependency as possible. Is there harm of using /proc/pid/maps ? parsing /proc/pid/maps already used by selftest in lots of places (see below), I would prefer testing to use well-established patterns than relying on a newly developed feature.

filesystems/overlayfs/dev_in_maps.c mm/mlock2-tests.c mm/mremap_test.c mm/virtual_address_range.c mm/split_huge_page_test.c filesystems/overlayfs/dev_in_maps.c exec/load_address.c bpf/trace_helpers.c

I have tried new ioctl PROCMAP_QUERY to replace get_vma_size, and observed a few facts: 1.The ioctl requires the caller to set the correct PROT bits for the query to succeed. 2. The ioctl returns the VMA containing the query address, which may not start at that address. 3. The ioctl doesn't return the sealing flag.

What get_vma_size wants is: for a given address, return the VMA started from that address, with PROT bits, size, and the sealing flag information.

In the next version, I'll add a check for the VMA's seal flag (by querying /proc/pid/smaps). Since the ioctl doesn't support returning the sealing flag and smaps already contain length/prot bit information, using an additional ioctl doesn't provide any benefit.

Thanks -Jeff

...

...

...

• fclose(maps);
  

• return size;
  

If we don't find the line we don't seem to be indicating this, again worth being able to pass back an error?

It can be normal that we don't find a line, i.e. tests want to verify that a VMA does not exist at address, return 0 size is correct in this case. If a parsing error ever happened, the test would have failed at FIXTURE_SETUP to begin with.

...

...

+}

+static inline bool mseal_supported(void) +{

• int ret;
  

• void *ptr;
  

• unsigned long page_size = getpagesize();
  

• ptr = mmap(NULL, page_size, PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
  

• if (ptr == MAP_FAILED)
  

•         return false;
  

• ret = sys_mseal(ptr, page_size, 0);
  

• if (ret < 0)
  

•         return false;
  

This is kind of cute, I mean it speaks to the need I think for us to be able to expose 'is X available?' programmatically. But I digress.

I guess this is ok, as this is something that should never fail.

You might recall that mseal is not available on the 32 bits kernel, automation might run the mseal_test on a 32 bit build. mseal_supported() detects this so tests can be skipped.

...

But we should definitely clean up after ourselves here whether this succeeds or fails.

please see resp [1].

Thanks! -Jeff [1] https://lore.kernel.org/all/CAKbZUD31EK2ah=vWJ46y4nL9OygzAa6ZxPnPHmWYO-sop5t... [2] https://lore.kernel.org/all/ko3pjllsgufbz33hqvwdpdsyxvgrgzqiodxexnpcng3mssff... [3] https://lore.kernel.org/all/20240807153544.2754247-1-jeffxu@chromium.org/

...

...

• return true;
  

+} diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c index ad17005521a8..8dd20341de3d 100644 --- a/tools/testing/selftests/mm/mseal_test.c +++ b/tools/testing/selftests/mm/mseal_test.c @@ -517,30 +517,6 @@ static void test_seal_twice(void) REPORT_TEST_PASS(); }

-static void test_seal_mprotect(bool seal) -{

• void *ptr;
  

• unsigned long page_size = getpagesize();
  

• unsigned long size = 4 * page_size;
  

• int ret;
  

• setup_single_address(size, &ptr);
  

• FAIL_TEST_IF_FALSE(ptr != (void *)-1);
  

• if (seal) {
  

•         ret = seal_single_address(ptr, size);
  

•         FAIL_TEST_IF_FALSE(!ret);
  

• }
  

• ret = sys_mprotect(ptr, size, PROT_READ | PROT_WRITE);
  

• if (seal)
  

•         FAIL_TEST_IF_FALSE(ret < 0);
  

• else
  

•         FAIL_TEST_IF_FALSE(!ret);
  

• REPORT_TEST_PASS();
  

-}

static void test_seal_start_mprotect(bool seal) { void *ptr; @@ -658,41 +634,6 @@ static void test_seal_mprotect_unalign_len_variant_2(bool seal) REPORT_TEST_PASS(); }

-static void test_seal_mprotect_two_vma(bool seal) -{

• void *ptr;
  

• unsigned long page_size = getpagesize();
  

• unsigned long size = 4 * page_size;
  

• int ret;
  

• setup_single_address(size, &ptr);
  

• FAIL_TEST_IF_FALSE(ptr != (void *)-1);
  

• /* use mprotect to split */
  

• ret = sys_mprotect(ptr, page_size * 2, PROT_READ | PROT_WRITE);
  

• FAIL_TEST_IF_FALSE(!ret);
  

• if (seal) {
  

•         ret = seal_single_address(ptr, page_size * 4);
  

•         FAIL_TEST_IF_FALSE(!ret);
  

• }
  

• ret = sys_mprotect(ptr, page_size * 2, PROT_READ | PROT_WRITE);
  

• if (seal)
  

•         FAIL_TEST_IF_FALSE(ret < 0);
  

• else
  

•         FAIL_TEST_IF_FALSE(!ret);
  

• ret = sys_mprotect(ptr + page_size * 2, page_size * 2,
  

•                 PROT_READ | PROT_WRITE);
  

• if (seal)
  

•         FAIL_TEST_IF_FALSE(ret < 0);
  

• else
  

•         FAIL_TEST_IF_FALSE(!ret);
  

• REPORT_TEST_PASS();
  

-}

static void test_seal_mprotect_two_vma_with_split(bool seal) { void *ptr; @@ -1876,7 +1817,7 @@ int main(void) if (!pkey_supported()) ksft_print_msg("PKEY not supported\n");

• ksft_set_plan(88);
  

• ksft_set_plan(84);
  
  test_seal_addseal();
  test_seal_unmapped_start();
  

@@ -1889,9 +1830,6 @@ int main(void) test_seal_zero_length(); test_seal_twice();

• test_seal_mprotect(false);
  

• test_seal_mprotect(true);
  

• test_seal_start_mprotect(false);
  test_seal_start_mprotect(true);
  
  

@@ -1904,9 +1842,6 @@ int main(void) test_seal_mprotect_unalign_len_variant_2(false); test_seal_mprotect_unalign_len_variant_2(true);

• test_seal_mprotect_two_vma(false);
  

• test_seal_mprotect_two_vma(true);
  

• test_seal_mprotect_two_vma_with_split(false);
  test_seal_mprotect_two_vma_with_split(true);
  
  

-- 2.47.1.613.gc27f4b7a9f-goog