[v3,4/5] selftests/mseal: add more tests for mmap

Commit Message

Jeff Xu Aug. 30, 2024, 6:02 p.m. UTC

From: Jeff Xu <[e-mail]>

Add sealing test to cover mmap for
Expand/shrink across sealed vmas (MAP_FIXED)
Reuse the same address in !MAP_FIXED case.

Signed-off-by: Jeff Xu <[e-mail]>
---
 tools/testing/selftests/mm/mseal_test.c | 126 +++++++++++++++++++++++-
 1 file changed, 125 insertions(+), 1 deletion(-)

Comments

Lorenzo Stoakes Aug. 30, 2024, 6:43 p.m. UTC | #1

On Fri, Aug 30, 2024 at 06:02:36PM GMT, [e-mail] wrote:
> From: Jeff Xu <[e-mail]>
>
> Add sealing test to cover mmap for
> Expand/shrink across sealed vmas (MAP_FIXED)
> Reuse the same address in !MAP_FIXED case.

This commit message is woefully small. I told you on v1 to improve the
commit messages. Linus has told you to do this before.

Please actually respond to feedback. Thanks.

>
> Signed-off-by: Jeff Xu <[e-mail]>
> ---
>  tools/testing/selftests/mm/mseal_test.c | 126 +++++++++++++++++++++++-
>  1 file changed, 125 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
> index e855c8ccefc3..3516389034a7 100644
> --- a/tools/testing/selftests/mm/mseal_test.c
> +++ b/tools/testing/selftests/mm/mseal_test.c
> @@ -2222,6 +2222,123 @@ static void test_munmap_free_multiple_ranges(bool seal)
>  	REPORT_TEST_PASS();
>  }
>
> +static void test_seal_mmap_expand_seal_middle(bool seal)

This test doesn't expand, doesn't do anything in the middle. It does mmap()
though and relates to mseal, so that's something... this is compeltely
misnamed and needs to be rethought.

> +{
> +	void *ptr;
> +	unsigned long page_size = getpagesize();
> +	unsigned long size = 12 * page_size;
> +	int ret;
> +	void *ret2;
> +	int prot;
> +
> +	setup_single_address(size, &ptr);

Please replace every single instance of this with an mmap(). There's
literally no reason to abstract it. And munmap() what you map.

> +	FAIL_TEST_IF_FALSE(ptr != (void *)-1);

Pretty sure Pedro pointed out you should be checking against MAP_FAILED
here. I really don't understand why the rest of your test is full of
mmap()'s but for some reason you choose to abstract this one call? What?

> +	/* ummap last 4 pages. */
> +	ret = sys_munmap(ptr + 8 * page_size, 4 * page_size);

sys_munmap()? What's wrong with munmap()?

> +	FAIL_TEST_IF_FALSE(!ret);

Why do we not have a FAIL_TEST_IF_TRUE()? This is crazy.

Would be nice to have something human-readable like ASSERT_EQ() or
ASSERT_TRUE() or ASSERT_FALSE().

> +
> +	size = get_vma_size(ptr, &prot);
> +	FAIL_TEST_IF_FALSE(size == 8 * page_size);
> +	FAIL_TEST_IF_FALSE(prot == 0x4);
> +
> +	if (seal) {
> +		ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
> +		FAIL_TEST_IF_FALSE(!ret);
> +	}
> +
> +	/* use mmap to expand and overwrite (MAP_FIXED)  */

You don't really need to say MAP_FIXED, it's below.

> +	ret2 = mmap(ptr, 12 * page_size, PROT_READ,
> +			MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);

Why read-only?

You're not expanding you're overwriting. You're not doing anything in the
middle.

I'm again confused about what you think you're testing here. I don't think
we need an arbitrary MAP_FIXED mmap() at a size larger than the overwritten
VMA?

You just need a single instance of a MAP_FIXED mmap() over a sealed mmap()
if that's what you want.

> +	if (seal) {
> +		FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
> +		FAIL_TEST_IF_FALSE(errno == EPERM);
> +
> +		size = get_vma_size(ptr, &prot);
> +		FAIL_TEST_IF_FALSE(size == 4 * page_size);
> +		FAIL_TEST_IF_FALSE(prot == 0x4);
> +
> +		size = get_vma_size(ptr + 4 * page_size, &prot);
> +		FAIL_TEST_IF_FALSE(size == 4 * page_size);
> +		FAIL_TEST_IF_FALSE(prot == 0x4);
> +	} else
> +		FAIL_TEST_IF_FALSE(ret2 == ptr);

Don't do dangling else's after a big block.

> +
> +	REPORT_TEST_PASS();
> +}
> +
> +static void test_seal_mmap_shrink_seal_middle(bool seal)

What's going on in the 'middle'? This test doesn't shrink, it overwrites
the beginning of a sealed VMA?
> +{
> +	void *ptr;
> +	unsigned long page_size = getpagesize();
> +	unsigned long size = 12 * page_size;
> +	int ret;
> +	void *ret2;
> +	int prot;
> +
> +	setup_single_address(size, &ptr);
> +	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> +
> +	if (seal) {
> +		ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
> +		FAIL_TEST_IF_FALSE(!ret);
> +	}
> +
> +	/* use mmap to shrink and overwrite (MAP_FIXED)  */

What exactly are you shrinking? You're overwriting the start of the vma?

What is this testing that is different from the previous test? This seems
useless honestly.

> +	ret2 = mmap(ptr, 7 * page_size, PROT_READ,
> +			MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
> +	if (seal) {
> +		FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
> +		FAIL_TEST_IF_FALSE(errno == EPERM);
> +
> +		size = get_vma_size(ptr, &prot);
> +		FAIL_TEST_IF_FALSE(size == 4 * page_size);
> +		FAIL_TEST_IF_FALSE(prot == 0x4);

What the hell is this comparison to magic numbers? This is
ridiculous. What's wrong with PROT_xxx??

> +
> +		size = get_vma_size(ptr + 4 * page_size, &prot);
> +		FAIL_TEST_IF_FALSE(size == 4 * page_size);
> +		FAIL_TEST_IF_FALSE(prot == 0x4);
> +
> +		size = get_vma_size(ptr + 4 * page_size, &prot);
> +		FAIL_TEST_IF_FALSE(size == 4 * page_size);
> +		FAIL_TEST_IF_FALSE(prot == 0x4);

Err dude, you're doing this twice?

So what are we testing here exactly? That we got a VMA split? This is
err... why are we asserting this?

> +	} else
> +		FAIL_TEST_IF_FALSE(ret2 == ptr);
> +
> +	REPORT_TEST_PASS();
> +}
> +
> +static void test_seal_mmap_reuse_addr(bool seal)

This is wrong, you're not reusing anything. This test is useless.

> +{
> +	void *ptr;
> +	unsigned long page_size = getpagesize();
> +	unsigned long size = page_size;
> +	int ret;
> +	void *ret2;
> +	int prot;
> +
> +	setup_single_address(size, &ptr);
> +	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> +
> +	if (seal) {
> +		ret = sys_mseal(ptr, size);
> +		FAIL_TEST_IF_FALSE(!ret);

We could avoid this horrid ret, ret2 naming if you just did:

	FAIL_TEST_IF_FALSE(sys_mseal(ptr, size));

> +	}
> +
> +	/* use mmap to change protection. */
> +	ret2 = mmap(ptr, size, PROT_NONE,
> +			MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);

How are you using mmap to change the protection when you're providing a
hint to the address to use? You're not changing any protection at all!

You're allocating an entirely new VMA hinting that you want it near
ptr. Please read the man page for mmap():

       If addr is NULL, then the kernel chooses the (page-aligned) address
       at which to create the mapping; this is the most portable method of
       creating a new mapping.  If addr is not NULL, then the kernel takes
       it as a hint about where to place the mapping; on Linux, the kernel
       will pick a nearby page boundary (but always above or equal to the
       value specified by /proc/sys/vm/mmap_min_addr) and attempt to create
       the mapping there.  If another mapping already exists there, the
       kernel picks a new address that may or may not depend on the hint.
       The address of the new mapping is returned as the result of the
       call.

> +
> +	/* MAP_FIXED is not used, expect new addr */
> +	FAIL_TEST_IF_FALSE(!(ret2 == MAP_FAILED));

This is beyond horrible. You really have to add more asserts.

Also you're expecting a new address here, so again, what on earth are you
asserting? That we can mmap()?

> +	FAIL_TEST_IF_FALSE(ret2 != ptr);
> +
> +	size = get_vma_size(ptr, &prot);
> +	FAIL_TEST_IF_FALSE(size == page_size);
> +	FAIL_TEST_IF_FALSE(prot == 0x4);
> +
> +	REPORT_TEST_PASS();
> +}
> +
>  int main(int argc, char **argv)
>  {
>  	bool test_seal = seal_support();
> @@ -2243,7 +2360,7 @@ int main(int argc, char **argv)
>  	if (!get_vma_size_supported())
>  		ksft_exit_skip("get_vma_size not supported\n");
>
> -	ksft_set_plan(91);
> +	ksft_set_plan(97);

I'm guessing this is the number of tests, but I mean this is horrible. Is
there not a better way of doing this?

>
>  	test_seal_addseal();
>  	test_seal_unmapped_start();
> @@ -2357,5 +2474,12 @@ int main(int argc, char **argv)
>  	test_munmap_free_multiple_ranges(false);
>  	test_munmap_free_multiple_ranges(true);
>
> +	test_seal_mmap_expand_seal_middle(false);
> +	test_seal_mmap_expand_seal_middle(true);
> +	test_seal_mmap_shrink_seal_middle(false);
> +	test_seal_mmap_shrink_seal_middle(true);
> +	test_seal_mmap_reuse_addr(false);
> +	test_seal_mmap_reuse_addr(true);
> +
>  	ksft_finished();
>  }
> --
> 2.46.0.469.g59c65b2a67-goog
>

Lorenzo Stoakes Aug. 30, 2024, 7:22 p.m. UTC | #2

On Fri, Aug 30, 2024 at 07:43:12PM GMT, Lorenzo Stoakes wrote:
> On Fri, Aug 30, 2024 at 06:02:36PM GMT, [e-mail] wrote:
> > From: Jeff Xu <[e-mail]>
> >
> > Add sealing test to cover mmap for
> > Expand/shrink across sealed vmas (MAP_FIXED)
> > Reuse the same address in !MAP_FIXED case.
>
> This commit message is woefully small. I told you on v1 to improve the
> commit messages. Linus has told you to do this before.
>
> Please actually respond to feedback. Thanks.
>
> >
> > Signed-off-by: Jeff Xu <[e-mail]>
> > ---
> >  tools/testing/selftests/mm/mseal_test.c | 126 +++++++++++++++++++++++-
> >  1 file changed, 125 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
> > index e855c8ccefc3..3516389034a7 100644
> > --- a/tools/testing/selftests/mm/mseal_test.c
> > +++ b/tools/testing/selftests/mm/mseal_test.c
> > @@ -2222,6 +2222,123 @@ static void test_munmap_free_multiple_ranges(bool seal)
> >  	REPORT_TEST_PASS();
> >  }
> >
> > +static void test_seal_mmap_expand_seal_middle(bool seal)
>
> This test doesn't expand, doesn't do anything in the middle. It does mmap()
> though and relates to mseal, so that's something... this is compeltely
> misnamed and needs to be rethought.
>

OK correction - it _seals_ in the middle. The remained of the criticism remains,
and this is rather confusing... and I continue to wonder what the purpose of
this is?

> > +{
> > +	void *ptr;
> > +	unsigned long page_size = getpagesize();
> > +	unsigned long size = 12 * page_size;
> > +	int ret;
> > +	void *ret2;
> > +	int prot;
> > +
> > +	setup_single_address(size, &ptr);
>
> Please replace every single instance of this with an mmap(). There's
> literally no reason to abstract it. And munmap() what you map.
>
> > +	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
>
> Pretty sure Pedro pointed out you should be checking against MAP_FAILED
> here. I really don't understand why the rest of your test is full of
> mmap()'s but for some reason you choose to abstract this one call? What?
>
> > +	/* ummap last 4 pages. */
> > +	ret = sys_munmap(ptr + 8 * page_size, 4 * page_size);
>
> sys_munmap()? What's wrong with munmap()?
>
> > +	FAIL_TEST_IF_FALSE(!ret);
>
> Why do we not have a FAIL_TEST_IF_TRUE()? This is crazy.
>
> Would be nice to have something human-readable like ASSERT_EQ() or
> ASSERT_TRUE() or ASSERT_FALSE().
>
> > +
> > +	size = get_vma_size(ptr, &prot);
> > +	FAIL_TEST_IF_FALSE(size == 8 * page_size);
> > +	FAIL_TEST_IF_FALSE(prot == 0x4);
> > +
> > +	if (seal) {
> > +		ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
> > +		FAIL_TEST_IF_FALSE(!ret);
> > +	}
> > +
> > +	/* use mmap to expand and overwrite (MAP_FIXED)  */
>
> You don't really need to say MAP_FIXED, it's below.
>
> > +	ret2 = mmap(ptr, 12 * page_size, PROT_READ,
> > +			MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
>
> Why read-only?
>
> You're not expanding you're overwriting. You're not doing anything in the
> middle.
>
> I'm again confused about what you think you're testing here. I don't think
> we need an arbitrary MAP_FIXED mmap() at a size larger than the overwritten
> VMA?
>
> You just need a single instance of a MAP_FIXED mmap() over a sealed mmap()
> if that's what you want.
>
> > +	if (seal) {
> > +		FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
> > +		FAIL_TEST_IF_FALSE(errno == EPERM);
> > +
> > +		size = get_vma_size(ptr, &prot);
> > +		FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > +		FAIL_TEST_IF_FALSE(prot == 0x4);
> > +
> > +		size = get_vma_size(ptr + 4 * page_size, &prot);
> > +		FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > +		FAIL_TEST_IF_FALSE(prot == 0x4);
> > +	} else
> > +		FAIL_TEST_IF_FALSE(ret2 == ptr);
>
> Don't do dangling else's after a big block.
>
> > +
> > +	REPORT_TEST_PASS();
> > +}
> > +
> > +static void test_seal_mmap_shrink_seal_middle(bool seal)
>
> What's going on in the 'middle'? This test doesn't shrink, it overwrites
> the beginning of a sealed VMA?

Correction - the middle is sealed. Other points remain.

> > +{
> > +	void *ptr;
> > +	unsigned long page_size = getpagesize();
> > +	unsigned long size = 12 * page_size;
> > +	int ret;
> > +	void *ret2;
> > +	int prot;
> > +
> > +	setup_single_address(size, &ptr);
> > +	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > +
> > +	if (seal) {
> > +		ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
> > +		FAIL_TEST_IF_FALSE(!ret);
> > +	}
> > +
> > +	/* use mmap to shrink and overwrite (MAP_FIXED)  */
>
> What exactly are you shrinking? You're overwriting the start of the vma?
>
> What is this testing that is different from the previous test? This seems
> useless honestly.
>
> > +	ret2 = mmap(ptr, 7 * page_size, PROT_READ,
> > +			MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
> > +	if (seal) {
> > +		FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
> > +		FAIL_TEST_IF_FALSE(errno == EPERM);
> > +
> > +		size = get_vma_size(ptr, &prot);
> > +		FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > +		FAIL_TEST_IF_FALSE(prot == 0x4);
>
> What the hell is this comparison to magic numbers? This is
> ridiculous. What's wrong with PROT_xxx??
>
> > +
> > +		size = get_vma_size(ptr + 4 * page_size, &prot);
> > +		FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > +		FAIL_TEST_IF_FALSE(prot == 0x4);
> > +
> > +		size = get_vma_size(ptr + 4 * page_size, &prot);
> > +		FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > +		FAIL_TEST_IF_FALSE(prot == 0x4);
>
> Err dude, you're doing this twice?
>
> So what are we testing here exactly? That we got a VMA split? This is
> err... why are we asserting this?

I guess, that we can't overwrite a sealed bit of a VMA at the end. But again
this feels entirely redundant. For this kind of thing to fail would mean the
whole VMA machinery is broken.

>
> > +	} else
> > +		FAIL_TEST_IF_FALSE(ret2 == ptr);
> > +
> > +	REPORT_TEST_PASS();
> > +}
> > +
> > +static void test_seal_mmap_reuse_addr(bool seal)
>
> This is wrong, you're not reusing anything. This test is useless.
>
> > +{
> > +	void *ptr;
> > +	unsigned long page_size = getpagesize();
> > +	unsigned long size = page_size;
> > +	int ret;
> > +	void *ret2;
> > +	int prot;
> > +
> > +	setup_single_address(size, &ptr);
> > +	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > +
> > +	if (seal) {
> > +		ret = sys_mseal(ptr, size);
> > +		FAIL_TEST_IF_FALSE(!ret);
>
> We could avoid this horrid ret, ret2 naming if you just did:
>
> 	FAIL_TEST_IF_FALSE(sys_mseal(ptr, size));
>
> > +	}
> > +
> > +	/* use mmap to change protection. */
> > +	ret2 = mmap(ptr, size, PROT_NONE,
> > +			MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
>
> How are you using mmap to change the protection when you're providing a
> hint to the address to use? You're not changing any protection at all!
>
> You're allocating an entirely new VMA hinting that you want it near
> ptr. Please read the man page for mmap():
>
>        If addr is NULL, then the kernel chooses the (page-aligned) address
>        at which to create the mapping; this is the most portable method of
>        creating a new mapping.  If addr is not NULL, then the kernel takes
>        it as a hint about where to place the mapping; on Linux, the kernel
>        will pick a nearby page boundary (but always above or equal to the
>        value specified by /proc/sys/vm/mmap_min_addr) and attempt to create
>        the mapping there.  If another mapping already exists there, the
>        kernel picks a new address that may or may not depend on the hint.
>        The address of the new mapping is returned as the result of the
>        call.
>
> > +
> > +	/* MAP_FIXED is not used, expect new addr */
> > +	FAIL_TEST_IF_FALSE(!(ret2 == MAP_FAILED));
>
> This is beyond horrible. You really have to add more asserts.
>
> Also you're expecting a new address here, so again, what on earth are you
> asserting? That we can mmap()?
>
> > +	FAIL_TEST_IF_FALSE(ret2 != ptr);
> > +
> > +	size = get_vma_size(ptr, &prot);
> > +	FAIL_TEST_IF_FALSE(size == page_size);
> > +	FAIL_TEST_IF_FALSE(prot == 0x4);
> > +
> > +	REPORT_TEST_PASS();
> > +}
> > +
> >  int main(int argc, char **argv)
> >  {
> >  	bool test_seal = seal_support();
> > @@ -2243,7 +2360,7 @@ int main(int argc, char **argv)
> >  	if (!get_vma_size_supported())
> >  		ksft_exit_skip("get_vma_size not supported\n");
> >
> > -	ksft_set_plan(91);
> > +	ksft_set_plan(97);
>
> I'm guessing this is the number of tests, but I mean this is horrible. Is
> there not a better way of doing this?
>
> >
> >  	test_seal_addseal();
> >  	test_seal_unmapped_start();
> > @@ -2357,5 +2474,12 @@ int main(int argc, char **argv)
> >  	test_munmap_free_multiple_ranges(false);
> >  	test_munmap_free_multiple_ranges(true);
> >
> > +	test_seal_mmap_expand_seal_middle(false);
> > +	test_seal_mmap_expand_seal_middle(true);
> > +	test_seal_mmap_shrink_seal_middle(false);
> > +	test_seal_mmap_shrink_seal_middle(true);
> > +	test_seal_mmap_reuse_addr(false);
> > +	test_seal_mmap_reuse_addr(true);
> > +
> >  	ksft_finished();
> >  }
> > --
> > 2.46.0.469.g59c65b2a67-goog
> >

Jeff Xu Aug. 30, 2024, 11:57 p.m. UTC | #3

On Fri, Aug 30, 2024 at 12:23 PM Lorenzo Stoakes
<[e-mail]> wrote:
>
> On Fri, Aug 30, 2024 at 07:43:12PM GMT, Lorenzo Stoakes wrote:
> > On Fri, Aug 30, 2024 at 06:02:36PM GMT, [e-mail] wrote:
> > > From: Jeff Xu <[e-mail]>
> > >
> > > Add sealing test to cover mmap for
> > > Expand/shrink across sealed vmas (MAP_FIXED)
> > > Reuse the same address in !MAP_FIXED case.
> >
> > This commit message is woefully small. I told you on v1 to improve the
> > commit messages. Linus has told you to do this before.
> >
> > Please actually respond to feedback. Thanks.
> >
> > >
> > > Signed-off-by: Jeff Xu <[e-mail]>
> > > ---
> > >  tools/testing/selftests/mm/mseal_test.c | 126 +++++++++++++++++++++++-
> > >  1 file changed, 125 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
> > > index e855c8ccefc3..3516389034a7 100644
> > > --- a/tools/testing/selftests/mm/mseal_test.c
> > > +++ b/tools/testing/selftests/mm/mseal_test.c
> > > @@ -2222,6 +2222,123 @@ static void test_munmap_free_multiple_ranges(bool seal)
> > >     REPORT_TEST_PASS();
> > >  }
> > >
> > > +static void test_seal_mmap_expand_seal_middle(bool seal)
> >
> > This test doesn't expand, doesn't do anything in the middle. It does mmap()
> > though and relates to mseal, so that's something... this is compeltely
> > misnamed and needs to be rethought.
> >
>
> OK correction - it _seals_ in the middle. The remained of the criticism remains,
> and this is rather confusing... and I continue to wonder what the purpose of
> this is?
>
It expands the size (start from ptr).

> > > +{
> > > +   void *ptr;
> > > +   unsigned long page_size = getpagesize();
> > > +   unsigned long size = 12 * page_size;
> > > +   int ret;
> > > +   void *ret2;
> > > +   int prot;
> > > +
> > > +   setup_single_address(size, &ptr);
> >
> > Please replace every single instance of this with an mmap(). There's
> > literally no reason to abstract it. And munmap() what you map.
> >
No, we need to abstract it.  In addition to the mmap, it also
allocates an additional two blocks before and after the allocated
memory, to avoid auto-merging, so we can use get_vma_size.

> > > +   FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> >
> > Pretty sure Pedro pointed out you should be checking against MAP_FAILED
> > here. I really don't understand why the rest of your test is full of
> > mmap()'s but for some reason you choose to abstract this one call? What?
> >
> > > +   /* ummap last 4 pages. */
> > > +   ret = sys_munmap(ptr + 8 * page_size, 4 * page_size);
> >
> > sys_munmap()? What's wrong with munmap()?
> >
> > > +   FAIL_TEST_IF_FALSE(!ret);
> >
> > Why do we not have a FAIL_TEST_IF_TRUE()? This is crazy.
> >
> > Would be nice to have something human-readable like ASSERT_EQ() or
> > ASSERT_TRUE() or ASSERT_FALSE().
> >
ASSERT_EQ and ASSERT_TURE are not recommended by the self-test. The
FAIL_TEST_IF_FAIL wrap will take care of some of the admin tasks
related to self-test infra, such as count how many tests are failing.

> > > +
> > > +   size = get_vma_size(ptr, &prot);
> > > +   FAIL_TEST_IF_FALSE(size == 8 * page_size);
> > > +   FAIL_TEST_IF_FALSE(prot == 0x4);
> > > +
> > > +   if (seal) {
> > > +           ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
> > > +           FAIL_TEST_IF_FALSE(!ret);
> > > +   }
> > > +
> > > +   /* use mmap to expand and overwrite (MAP_FIXED)  */
> >
> > You don't really need to say MAP_FIXED, it's below.
> >
Adding a comment here to help reviewers.

> > > +   ret2 = mmap(ptr, 12 * page_size, PROT_READ,
> > > +                   MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
> >
> > Why read-only?
> >
> > You're not expanding you're overwriting. You're not doing anything in the
> > middle.
> >
The MAP_FIXED is overwriting.  It also expands the address range
(start from ptr) from 8 to 12 pages.

> > I'm again confused about what you think you're testing here. I don't think
> > we need an arbitrary MAP_FIXED mmap() at a size larger than the overwritten
> > VMA?
> >
> > You just need a single instance of a MAP_FIXED mmap() over a sealed mmap()
> > if that's what you want.
> >
> > > +   if (seal) {
> > > +           FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
> > > +           FAIL_TEST_IF_FALSE(errno == EPERM);
> > > +
> > > +           size = get_vma_size(ptr, &prot);
> > > +           FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > > +           FAIL_TEST_IF_FALSE(prot == 0x4);
> > > +
> > > +           size = get_vma_size(ptr + 4 * page_size, &prot);
> > > +           FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > > +           FAIL_TEST_IF_FALSE(prot == 0x4);
> > > +   } else
> > > +           FAIL_TEST_IF_FALSE(ret2 == ptr);
> >
> > Don't do dangling else's after a big block.
> >
patch passed the checkpatch.pl for style check.

> > > +
> > > +   REPORT_TEST_PASS();
> > > +}
> > > +
> > > +static void test_seal_mmap_shrink_seal_middle(bool seal)
> >
> > What's going on in the 'middle'? This test doesn't shrink, it overwrites
> > the beginning of a sealed VMA?
>
> Correction - the middle is sealed. Other points remain.
>
The mmap attempts to shrink the address range from 12 pages to 8 pages.

> > > +{
> > > +   void *ptr;
> > > +   unsigned long page_size = getpagesize();
> > > +   unsigned long size = 12 * page_size;
> > > +   int ret;
> > > +   void *ret2;
> > > +   int prot;
> > > +
> > > +   setup_single_address(size, &ptr);
> > > +   FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > > +
> > > +   if (seal) {
> > > +           ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
> > > +           FAIL_TEST_IF_FALSE(!ret);
> > > +   }
> > > +
> > > +   /* use mmap to shrink and overwrite (MAP_FIXED)  */
> >
> > What exactly are you shrinking? You're overwriting the start of the vma?
> >
> > What is this testing that is different from the previous test? This seems
> > useless honestly.
> >
Again, as above, one test is expanding, the other test is shrinking.
Please take a look at mmap parameters and steps before mmap call.


> > > +   ret2 = mmap(ptr, 7 * page_size, PROT_READ,
> > > +                   MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
> > > +   if (seal) {
> > > +           FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
> > > +           FAIL_TEST_IF_FALSE(errno == EPERM);
> > > +
> > > +           size = get_vma_size(ptr, &prot);
> > > +           FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > > +           FAIL_TEST_IF_FALSE(prot == 0x4);
> >
> > What the hell is this comparison to magic numbers? This is
> > ridiculous. What's wrong with PROT_xxx??
> >
The PROT_xxx can't be used here.
get_vma_size doesn't return PROT_ type, i.e. the bit sequence is different.

> > > +
> > > +           size = get_vma_size(ptr + 4 * page_size, &prot);
> > > +           FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > > +           FAIL_TEST_IF_FALSE(prot == 0x4);
> > > +
> > > +           size = get_vma_size(ptr + 4 * page_size, &prot);
> > > +           FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > > +           FAIL_TEST_IF_FALSE(prot == 0x4);
> >
> > Err dude, you're doing this twice?
> >
The second get_vma_size should be (ptr + 8 * page_size)
I will update that.

> > So what are we testing here exactly? That we got a VMA split? This is
> > err... why are we asserting this?
>
> I guess, that we can't overwrite a sealed bit of a VMA at the end. But again
> this feels entirely redundant. For this kind of thing to fail would mean the
> whole VMA machinery is broken.
>
The test is testing mmap(MAP_FIXED), since it can be used to overwrite
the sealed memory range (without sealing), then there is a variant of
expand/shrink.


> >
> > > +   } else
> > > +           FAIL_TEST_IF_FALSE(ret2 == ptr);
> > > +
> > > +   REPORT_TEST_PASS();
> > > +}
> > > +
> > > +static void test_seal_mmap_reuse_addr(bool seal)
> >
> > This is wrong, you're not reusing anything. This test is useless.
> >
The ptr is reused as a hint.

> > > +{
> > > +   void *ptr;
> > > +   unsigned long page_size = getpagesize();
> > > +   unsigned long size = page_size;
> > > +   int ret;
> > > +   void *ret2;
> > > +   int prot;
> > > +
> > > +   setup_single_address(size, &ptr);
> > > +   FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > > +
> > > +   if (seal) {
> > > +           ret = sys_mseal(ptr, size);
> > > +           FAIL_TEST_IF_FALSE(!ret);
> >
> > We could avoid this horrid ret, ret2 naming if you just did:
> >
> >       FAIL_TEST_IF_FALSE(sys_mseal(ptr, size));
> >
> > > +   }
> > > +
> > > +   /* use mmap to change protection. */
> > > +   ret2 = mmap(ptr, size, PROT_NONE,
> > > +                   MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> >
> > How are you using mmap to change the protection when you're providing a
> > hint to the address to use? You're not changing any protection at all!
> >
It is necessary to add the this tests to make sure mseal is behave as
it should be, which is !MAP_FIXED case, new address will be allocated,
instead of fail of mmap()


> > You're allocating an entirely new VMA hinting that you want it near
> > ptr. Please read the man page for mmap():
> >
> >        If addr is NULL, then the kernel chooses the (page-aligned) address
> >        at which to create the mapping; this is the most portable method of
> >        creating a new mapping.  If addr is not NULL, then the kernel takes
> >        it as a hint about where to place the mapping; on Linux, the kernel
> >        will pick a nearby page boundary (but always above or equal to the
> >        value specified by /proc/sys/vm/mmap_min_addr) and attempt to create
> >        the mapping there.  If another mapping already exists there, the
> >        kernel picks a new address that may or may not depend on the hint.
> >        The address of the new mapping is returned as the result of the
> >        call.
> >
> > > +
> > > +   /* MAP_FIXED is not used, expect new addr */
> > > +   FAIL_TEST_IF_FALSE(!(ret2 == MAP_FAILED));
> >
> > This is beyond horrible. You really have to add more asserts.
> >
Again assert is not recommended by self_test

> > Also you're expecting a new address here, so again, what on earth are you
> > asserting? That we can mmap()?
> >
> > > +   FAIL_TEST_IF_FALSE(ret2 != ptr);
> > > +
> > > +   size = get_vma_size(ptr, &prot);
> > > +   FAIL_TEST_IF_FALSE(size == page_size);
> > > +   FAIL_TEST_IF_FALSE(prot == 0x4);
> > > +
> > > +   REPORT_TEST_PASS();
> > > +}
> > > +
> > >  int main(int argc, char **argv)
> > >  {
> > >     bool test_seal = seal_support();
> > > @@ -2243,7 +2360,7 @@ int main(int argc, char **argv)
> > >     if (!get_vma_size_supported())
> > >             ksft_exit_skip("get_vma_size not supported\n");
> > >
> > > -   ksft_set_plan(91);
> > > +   ksft_set_plan(97);
> >
> > I'm guessing this is the number of tests, but I mean this is horrible. Is
> > there not a better way of doing this?
> >
Again, this is recommended by self-test.



> > >
> > >     test_seal_addseal();
> > >     test_seal_unmapped_start();
> > > @@ -2357,5 +2474,12 @@ int main(int argc, char **argv)
> > >     test_munmap_free_multiple_ranges(false);
> > >     test_munmap_free_multiple_ranges(true);
> > >
> > > +   test_seal_mmap_expand_seal_middle(false);
> > > +   test_seal_mmap_expand_seal_middle(true);
> > > +   test_seal_mmap_shrink_seal_middle(false);
> > > +   test_seal_mmap_shrink_seal_middle(true);
> > > +   test_seal_mmap_reuse_addr(false);
> > > +   test_seal_mmap_reuse_addr(true);
> > > +
> > >     ksft_finished();
> > >  }
> > > --
> > > 2.46.0.469.g59c65b2a67-goog
> > >

Lorenzo Stoakes Sept. 7, 2024, 7:27 p.m. UTC | #4

On Fri, Aug 30, 2024 at 04:57:26PM GMT, Jeff Xu wrote:
> On Fri, Aug 30, 2024 at 12:23 PM Lorenzo Stoakes
> <[e-mail]> wrote:
> >
> > On Fri, Aug 30, 2024 at 07:43:12PM GMT, Lorenzo Stoakes wrote:
> > > On Fri, Aug 30, 2024 at 06:02:36PM GMT, [e-mail] wrote:
> > > > From: Jeff Xu <[e-mail]>
> > > >
> > > > Add sealing test to cover mmap for
> > > > Expand/shrink across sealed vmas (MAP_FIXED)
> > > > Reuse the same address in !MAP_FIXED case.

Hi Jeff, I really want to find a constructive way forward, but you've
basically ignored more or less everything I've said here.

I could respond again to each of your points here, but - from my point of
view - if your response to 'what is this even testing?' is to just repeat
in effect the name of the test - we will be stuck in a loop, which will be
exited with a NACK. I don't want this.

The majority of these tests, from a VMA/mmap point of view, appear to me to
be essentially testing 'does basic mmap functionality work correctly',
which isn't testing mseal.

Look - I appreciate your commitment to testing (see my work on mm/vma.c - I
care passionately about testing) - but we must make sure we are actually
testing what we mean to.

So I suggest as a constructive way forward - firstly, submit a regression
test for the change Liam wrapped into his series regarding the -EPERM
thing.

This should go in uncontroversially, I will take the time to review it and
I don't see why that shouldn't be relatively straight forward. I will drop
the concerns about things like test macros etc. for that.

Then after that, I suggest we have a discussion about - at a higher level -
what it is you want to test. And then between me, you, Liam and Pedro -
ahead of time, list out the tests that we want, with all of us reaching
consensus.

I also suggest we figure out this FAIL_TEST_IF_FALSE() thing at this point
too - I may be missing something, but I cannot for the life me understand
why we have to assert negations only, and other self tests do not do this.

I have replied to a few sample points below.

All of us simply want to help make sure mseal works as well as it can, this
is the only motivation at play here.

Hope you have a great weekend!

Cheers, Lorenzo

> > >
> > > This commit message is woefully small. I told you on v1 to improve the
> > > commit messages. Linus has told you to do this before.
> > >
> > > Please actually respond to feedback. Thanks.
> > >
> > > >
> > > > Signed-off-by: Jeff Xu <[e-mail]>
> > > > ---
> > > >  tools/testing/selftests/mm/mseal_test.c | 126 +++++++++++++++++++++++-
> > > >  1 file changed, 125 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
> > > > index e855c8ccefc3..3516389034a7 100644
> > > > --- a/tools/testing/selftests/mm/mseal_test.c
> > > > +++ b/tools/testing/selftests/mm/mseal_test.c
> > > > @@ -2222,6 +2222,123 @@ static void test_munmap_free_multiple_ranges(bool seal)
> > > >     REPORT_TEST_PASS();
> > > >  }
> > > >
> > > > +static void test_seal_mmap_expand_seal_middle(bool seal)
> > >
> > > This test doesn't expand, doesn't do anything in the middle. It does mmap()
> > > though and relates to mseal, so that's something... this is compeltely
> > > misnamed and needs to be rethought.
> > >
> >
> > OK correction - it _seals_ in the middle. The remained of the criticism remains,
> > and this is rather confusing... and I continue to wonder what the purpose of
> > this is?
> >
> It expands the size (start from ptr).
>
> > > > +{
> > > > +   void *ptr;
> > > > +   unsigned long page_size = getpagesize();
> > > > +   unsigned long size = 12 * page_size;
> > > > +   int ret;
> > > > +   void *ret2;
> > > > +   int prot;
> > > > +
> > > > +   setup_single_address(size, &ptr);
> > >
> > > Please replace every single instance of this with an mmap(). There's
> > > literally no reason to abstract it. And munmap() what you map.
> > >
> No, we need to abstract it.  In addition to the mmap, it also
> allocates an additional two blocks before and after the allocated
> memory, to avoid auto-merging, so we can use get_vma_size.

It doesn't?

static void setup_single_address(int size, void **ptrOut)
{
	void *ptr;

	ptr = mmap(NULL, size, PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
	*ptrOut = ptr;
}

>
> > > > +   FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > >
> > > Pretty sure Pedro pointed out you should be checking against MAP_FAILED
> > > here. I really don't understand why the rest of your test is full of
> > > mmap()'s but for some reason you choose to abstract this one call? What?
> > >
> > > > +   /* ummap last 4 pages. */
> > > > +   ret = sys_munmap(ptr + 8 * page_size, 4 * page_size);
> > >
> > > sys_munmap()? What's wrong with munmap()?
> > >
> > > > +   FAIL_TEST_IF_FALSE(!ret);
> > >
> > > Why do we not have a FAIL_TEST_IF_TRUE()? This is crazy.
> > >
> > > Would be nice to have something human-readable like ASSERT_EQ() or
> > > ASSERT_TRUE() or ASSERT_FALSE().
> > >
> ASSERT_EQ and ASSERT_TURE are not recommended by the self-test. The
> FAIL_TEST_IF_FAIL wrap will take care of some of the admin tasks
> related to self-test infra, such as count how many tests are failing.

Can you please point me to where it says you should implement your own
macro that only tests the negation of an expression?

I have found other self tests that do.

>
> > > > +
> > > > +   size = get_vma_size(ptr, &prot);
> > > > +   FAIL_TEST_IF_FALSE(size == 8 * page_size);
> > > > +   FAIL_TEST_IF_FALSE(prot == 0x4);
> > > > +
> > > > +   if (seal) {
> > > > +           ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
> > > > +           FAIL_TEST_IF_FALSE(!ret);
> > > > +   }
> > > > +
> > > > +   /* use mmap to expand and overwrite (MAP_FIXED)  */
> > >
> > > You don't really need to say MAP_FIXED, it's below.
> > >
> Adding a comment here to help reviewers.
>
> > > > +   ret2 = mmap(ptr, 12 * page_size, PROT_READ,
> > > > +                   MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
> > >
> > > Why read-only?
> > >
> > > You're not expanding you're overwriting. You're not doing anything in the
> > > middle.
> > >
> The MAP_FIXED is overwriting.  It also expands the address range
> (start from ptr) from 8 to 12 pages.
>
> > > I'm again confused about what you think you're testing here. I don't think
> > > we need an arbitrary MAP_FIXED mmap() at a size larger than the overwritten
> > > VMA?
> > >
> > > You just need a single instance of a MAP_FIXED mmap() over a sealed mmap()
> > > if that's what you want.
> > >
> > > > +   if (seal) {
> > > > +           FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
> > > > +           FAIL_TEST_IF_FALSE(errno == EPERM);
> > > > +
> > > > +           size = get_vma_size(ptr, &prot);
> > > > +           FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > > > +           FAIL_TEST_IF_FALSE(prot == 0x4);
> > > > +
> > > > +           size = get_vma_size(ptr + 4 * page_size, &prot);
> > > > +           FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > > > +           FAIL_TEST_IF_FALSE(prot == 0x4);
> > > > +   } else
> > > > +           FAIL_TEST_IF_FALSE(ret2 == ptr);
> > >
> > > Don't do dangling else's after a big block.
> > >
> patch passed the checkpatch.pl for style check.
>
> > > > +
> > > > +   REPORT_TEST_PASS();
> > > > +}
> > > > +
> > > > +static void test_seal_mmap_shrink_seal_middle(bool seal)
> > >
> > > What's going on in the 'middle'? This test doesn't shrink, it overwrites
> > > the beginning of a sealed VMA?
> >
> > Correction - the middle is sealed. Other points remain.
> >
> The mmap attempts to shrink the address range from 12 pages to 8 pages.
>
> > > > +{
> > > > +   void *ptr;
> > > > +   unsigned long page_size = getpagesize();
> > > > +   unsigned long size = 12 * page_size;
> > > > +   int ret;
> > > > +   void *ret2;
> > > > +   int prot;
> > > > +
> > > > +   setup_single_address(size, &ptr);
> > > > +   FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > > > +
> > > > +   if (seal) {
> > > > +           ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
> > > > +           FAIL_TEST_IF_FALSE(!ret);
> > > > +   }
> > > > +
> > > > +   /* use mmap to shrink and overwrite (MAP_FIXED)  */
> > >
> > > What exactly are you shrinking? You're overwriting the start of the vma?
> > >
> > > What is this testing that is different from the previous test? This seems
> > > useless honestly.
> > >
> Again, as above, one test is expanding, the other test is shrinking.
> Please take a look at mmap parameters and steps before mmap call.
>
>
> > > > +   ret2 = mmap(ptr, 7 * page_size, PROT_READ,
> > > > +                   MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
> > > > +   if (seal) {
> > > > +           FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
> > > > +           FAIL_TEST_IF_FALSE(errno == EPERM);
> > > > +
> > > > +           size = get_vma_size(ptr, &prot);
> > > > +           FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > > > +           FAIL_TEST_IF_FALSE(prot == 0x4);
> > >
> > > What the hell is this comparison to magic numbers? This is
> > > ridiculous. What's wrong with PROT_xxx??
> > >
> The PROT_xxx can't be used here.
> get_vma_size doesn't return PROT_ type, i.e. the bit sequence is different.
>
> > > > +
> > > > +           size = get_vma_size(ptr + 4 * page_size, &prot);
> > > > +           FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > > > +           FAIL_TEST_IF_FALSE(prot == 0x4);
> > > > +
> > > > +           size = get_vma_size(ptr + 4 * page_size, &prot);
> > > > +           FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > > > +           FAIL_TEST_IF_FALSE(prot == 0x4);
> > >
> > > Err dude, you're doing this twice?
> > >
> The second get_vma_size should be (ptr + 8 * page_size)
> I will update that.
>
> > > So what are we testing here exactly? That we got a VMA split? This is
> > > err... why are we asserting this?
> >
> > I guess, that we can't overwrite a sealed bit of a VMA at the end. But again
> > this feels entirely redundant. For this kind of thing to fail would mean the
> > whole VMA machinery is broken.
> >
> The test is testing mmap(MAP_FIXED), since it can be used to overwrite
> the sealed memory range (without sealing), then there is a variant of
> expand/shrink.
>
>
> > >
> > > > +   } else
> > > > +           FAIL_TEST_IF_FALSE(ret2 == ptr);
> > > > +
> > > > +   REPORT_TEST_PASS();
> > > > +}
> > > > +
> > > > +static void test_seal_mmap_reuse_addr(bool seal)
> > >
> > > This is wrong, you're not reusing anything. This test is useless.
> > >
> The ptr is reused as a hint.
>
> > > > +{
> > > > +   void *ptr;
> > > > +   unsigned long page_size = getpagesize();
> > > > +   unsigned long size = page_size;
> > > > +   int ret;
> > > > +   void *ret2;
> > > > +   int prot;
> > > > +
> > > > +   setup_single_address(size, &ptr);
> > > > +   FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > > > +
> > > > +   if (seal) {
> > > > +           ret = sys_mseal(ptr, size);
> > > > +           FAIL_TEST_IF_FALSE(!ret);
> > >
> > > We could avoid this horrid ret, ret2 naming if you just did:
> > >
> > >       FAIL_TEST_IF_FALSE(sys_mseal(ptr, size));
> > >
> > > > +   }
> > > > +
> > > > +   /* use mmap to change protection. */
> > > > +   ret2 = mmap(ptr, size, PROT_NONE,
> > > > +                   MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> > >
> > > How are you using mmap to change the protection when you're providing a
> > > hint to the address to use? You're not changing any protection at all!
> > >
> It is necessary to add the this tests to make sure mseal is behave as
> it should be, which is !MAP_FIXED case, new address will be allocated,
> instead of fail of mmap()
>
>
> > > You're allocating an entirely new VMA hinting that you want it near
> > > ptr. Please read the man page for mmap():
> > >
> > >        If addr is NULL, then the kernel chooses the (page-aligned) address
> > >        at which to create the mapping; this is the most portable method of
> > >        creating a new mapping.  If addr is not NULL, then the kernel takes
> > >        it as a hint about where to place the mapping; on Linux, the kernel
> > >        will pick a nearby page boundary (but always above or equal to the
> > >        value specified by /proc/sys/vm/mmap_min_addr) and attempt to create
> > >        the mapping there.  If another mapping already exists there, the
> > >        kernel picks a new address that may or may not depend on the hint.
> > >        The address of the new mapping is returned as the result of the
> > >        call.
> > >
> > > > +
> > > > +   /* MAP_FIXED is not used, expect new addr */
> > > > +   FAIL_TEST_IF_FALSE(!(ret2 == MAP_FAILED));
> > >
> > > This is beyond horrible. You really have to add more asserts.
> > >
> Again assert is not recommended by self_test
>
> > > Also you're expecting a new address here, so again, what on earth are you
> > > asserting? That we can mmap()?
> > >
> > > > +   FAIL_TEST_IF_FALSE(ret2 != ptr);
> > > > +
> > > > +   size = get_vma_size(ptr, &prot);
> > > > +   FAIL_TEST_IF_FALSE(size == page_size);
> > > > +   FAIL_TEST_IF_FALSE(prot == 0x4);
> > > > +
> > > > +   REPORT_TEST_PASS();
> > > > +}
> > > > +
> > > >  int main(int argc, char **argv)
> > > >  {
> > > >     bool test_seal = seal_support();
> > > > @@ -2243,7 +2360,7 @@ int main(int argc, char **argv)
> > > >     if (!get_vma_size_supported())
> > > >             ksft_exit_skip("get_vma_size not supported\n");
> > > >
> > > > -   ksft_set_plan(91);
> > > > +   ksft_set_plan(97);
> > >
> > > I'm guessing this is the number of tests, but I mean this is horrible. Is
> > > there not a better way of doing this?
> > >
> Again, this is recommended by self-test.
>
>
>
> > > >
> > > >     test_seal_addseal();
> > > >     test_seal_unmapped_start();
> > > > @@ -2357,5 +2474,12 @@ int main(int argc, char **argv)
> > > >     test_munmap_free_multiple_ranges(false);
> > > >     test_munmap_free_multiple_ranges(true);
> > > >
> > > > +   test_seal_mmap_expand_seal_middle(false);
> > > > +   test_seal_mmap_expand_seal_middle(true);
> > > > +   test_seal_mmap_shrink_seal_middle(false);
> > > > +   test_seal_mmap_shrink_seal_middle(true);
> > > > +   test_seal_mmap_reuse_addr(false);
> > > > +   test_seal_mmap_reuse_addr(true);
> > > > +
> > > >     ksft_finished();
> > > >  }
> > > > --
> > > > 2.46.0.469.g59c65b2a67-goog
> > > >

Pedro Falcato Sept. 8, 2024, 9:35 p.m. UTC | #5

On Sat, Sep 07, 2024 at 08:27:52PM GMT, Lorenzo Stoakes wrote:
> On Fri, Aug 30, 2024 at 04:57:26PM GMT, Jeff Xu wrote:
> > On Fri, Aug 30, 2024 at 12:23 PM Lorenzo Stoakes
> > <[e-mail]> wrote:
> > >
> > > On Fri, Aug 30, 2024 at 07:43:12PM GMT, Lorenzo Stoakes wrote:
> > > > On Fri, Aug 30, 2024 at 06:02:36PM GMT, [e-mail] wrote:
> > > > > From: Jeff Xu <[e-mail]>
> > > > >
> > > > > Add sealing test to cover mmap for
> > > > > Expand/shrink across sealed vmas (MAP_FIXED)
> > > > > Reuse the same address in !MAP_FIXED case.
> 
> Hi Jeff, I really want to find a constructive way forward, but you've
> basically ignored more or less everything I've said here.
> 
> I could respond again to each of your points here, but - from my point of
> view - if your response to 'what is this even testing?' is to just repeat
> in effect the name of the test - we will be stuck in a loop, which will be
> exited with a NACK. I don't want this.
> 
> The majority of these tests, from a VMA/mmap point of view, appear to me to
> be essentially testing 'does basic mmap functionality work correctly',
> which isn't testing mseal.
> 
> Look - I appreciate your commitment to testing (see my work on mm/vma.c - I
> care passionately about testing) - but we must make sure we are actually
> testing what we mean to.
> 
> So I suggest as a constructive way forward - firstly, submit a regression
> test for the change Liam wrapped into his series regarding the -EPERM
> thing.
> 
> This should go in uncontroversially, I will take the time to review it and
> I don't see why that shouldn't be relatively straight forward. I will drop
> the concerns about things like test macros etc. for that.
> 
> Then after that, I suggest we have a discussion about - at a higher level -
> what it is you want to test. And then between me, you, Liam and Pedro -
> ahead of time, list out the tests that we want, with all of us reaching
> consensus.

Hi,

I agree with most of the points. Sitting down here to write unofficial
guidelines for mseal behavior.

mseal should seal regions and mark them immutable, which means their protection
and contents (ish?) (not _only_ backing mapping, but also contents in general
(see madvise below)) should not be changed throughout the lifetime of the address space.

For the general syscall interface, this means:
1) mprotect and munmap need to be blocked on mseal regions.
 1a) munmap _cannot_ tolerate partial failure, per POSIX.
 2b) mmap MAP_FIXED counts as an unmap operation and also needs to be blocked and return -EPERM.

2) Most madvise calls are allowed, except for destructive operations on
read-only anonymous _pages_ (MADV_DONTNEED is destructive for anon, but we also don't care
about blocking these ops if we can do it manually with e.g memset)
 2a) The current implementation only blocks discard on anonymous _regions_, which is slightly
     different. We probably do want to block these on MAP_PRIVATE file mappings, as to block
     stuff like madvise MADV_DONTNEED on program rodata.
 2b) We take into account pkeys when doing the permission checks.

3) mremap is not allowed as we'd change the "contents" of the old region.
 3a) Should mremap expansion be allowed? aka only block moving and shrinking, but allow expansion.
     We already informally allow expansion if e.g mmapping after it + mseal.

4) mlock and msync are allowed.

5) mseal is blocked.

6) Other miscellaneous syscalls (mbind, etc) that do not change contents in any way, are allowed.
 6a) This obviously means PTEs can change as long as the contents don't. Swapping is also ok.

7) FOLL_FORCE (kernel-internal speak, more commonly seen as ptrace and /proc/self/mem from userspace)
   should be disallowed (?)
 7a) This currently does not happen, and seems like a large hole? But disallowing this
     would probably severely break ptrace if the ELF sealing plans come to fruition.

When we say "disallowed", we usually (apart from munmap) allow for partial failure. This
means getting an -EPERM while part of the call succeeded, if we e.g mprotect a region consisting
of [NORMAL VMA][SEALED VMA]. We do not want to test for this, because we do not want to paint ourselves
into a corner - this is strictly "undefined behavior". The msealed regions themselves
will never be touched in such cases. (we do however want to test munmap operation atomicity, but this is
also kind of a munmap-related test, and might not really be something we really want in the mseal tests)

Kernel-internal wise: The VMA and PTE modifications resulting from the above operations are blocked.
Sealed VMAs allow splitting and merging; there was contention about the splitting issue, but it truly
does not make sense to block operations unless they affect a VMA entirely, and that would also force
VMA merging to be ABI ("vma_merge isn't merging these two regions and now my madvise works/doesn't work :(").


Do I have everything right? Am I missing anything?

Pedro Falcato Sept. 8, 2024, 9:56 p.m. UTC | #6

On Sun, Sep 8, 2024 at 10:35 PM Pedro Falcato <[e-mail]> wrote:
> Hi,
>
> I agree with most of the points. Sitting down here to write unofficial
> guidelines for mseal behavior.
>
> mseal should seal regions and mark them immutable, which means their protection
> and contents (ish?) (not _only_ backing mapping, but also contents in general
> (see madvise below)) should not be changed throughout the lifetime of the address space.
>
> For the general syscall interface, this means:
> 1) mprotect and munmap need to be blocked on mseal regions.
>  1a) munmap _cannot_ tolerate partial failure, per POSIX.
>  2b) mmap MAP_FIXED counts as an unmap operation and also needs to be blocked and return -EPERM.
>
> 2) Most madvise calls are allowed, except for destructive operations on
> read-only anonymous _pages_ (MADV_DONTNEED is destructive for anon, but we also don't care
> about blocking these ops if we can do it manually with e.g memset)
>  2a) The current implementation only blocks discard on anonymous _regions_, which is slightly
>      different. We probably do want to block these on MAP_PRIVATE file mappings, as to block
>      stuff like madvise MADV_DONTNEED on program rodata.
>  2b) We take into account pkeys when doing the permission checks.
>
> 3) mremap is not allowed as we'd change the "contents" of the old region.
>  3a) Should mremap expansion be allowed? aka only block moving and shrinking, but allow expansion.
>      We already informally allow expansion if e.g mmapping after it + mseal.
>
> 4) mlock and msync are allowed.
>
> 5) mseal is blocked.
>
> 6) Other miscellaneous syscalls (mbind, etc) that do not change contents in any way, are allowed.
>  6a) This obviously means PTEs can change as long as the contents don't. Swapping is also ok.
>
> 7) FOLL_FORCE (kernel-internal speak, more commonly seen as ptrace and /proc/self/mem from userspace)
>    should be disallowed (?)
>  7a) This currently does not happen, and seems like a large hole? But disallowing this
>      would probably severely break ptrace if the ELF sealing plans come to fruition.
>
> When we say "disallowed", we usually (apart from munmap) allow for partial failure. This
> means getting an -EPERM while part of the call succeeded, if we e.g mprotect a region consisting
> of [NORMAL VMA][SEALED VMA]. We do not want to test for this, because we do not want to paint ourselves
> into a corner - this is strictly "undefined behavior". The msealed regions themselves
> will never be touched in such cases. (we do however want to test munmap operation atomicity, but this is
> also kind of a munmap-related test, and might not really be something we really want in the mseal tests)
>
> Kernel-internal wise: The VMA and PTE modifications resulting from the above operations are blocked.
> Sealed VMAs allow splitting and merging; there was contention about the splitting issue, but it truly
> does not make sense to block operations unless they affect a VMA entirely, and that would also force
> VMA merging to be ABI ("vma_merge isn't merging these two regions and now my madvise works/doesn't work :(").
>
>
> Do I have everything right? Am I missing anything?

Small addendum: file write, truncate and hole punching might also
matter for sealed file-backed regions, as these change the region's
contents, but we probably
want to rely on file write permissions to protect against this (as we
already do). Any other solution is probably terrible and probably
endlessly NAK'd by fs folks, but it does
mean sealed regions aren't really immutable if you or the attacker can
write to the file.

Jeff Xu Sept. 13, 2024, 10:50 p.m. UTC | #7

Hi Lorenzo

On Sat, Sep 7, 2024 at 12:28 PM Lorenzo Stoakes
<[e-mail]> wrote:
>
> On Fri, Aug 30, 2024 at 04:57:26PM GMT, Jeff Xu wrote:
> > On Fri, Aug 30, 2024 at 12:23 PM Lorenzo Stoakes
> > <[e-mail]> wrote:
> > >
> > > On Fri, Aug 30, 2024 at 07:43:12PM GMT, Lorenzo Stoakes wrote:
> > > > On Fri, Aug 30, 2024 at 06:02:36PM GMT, [e-mail] wrote:
> > > > > From: Jeff Xu <[e-mail]>
> > > > >
> > > > > Add sealing test to cover mmap for
> > > > > Expand/shrink across sealed vmas (MAP_FIXED)
> > > > > Reuse the same address in !MAP_FIXED case.
>
> Hi Jeff, I really want to find a constructive way forward, but you've
> basically ignored more or less everything I've said here.
>
> I could respond again to each of your points here, but - from my point of
> view - if your response to 'what is this even testing?' is to just repeat
> in effect the name of the test - we will be stuck in a loop, which will be
> exited with a NACK. I don't want this.
>
> The majority of these tests, from a VMA/mmap point of view, appear to me to
> be essentially testing 'does basic mmap functionality work correctly',
> which isn't testing mseal.
>
> Look - I appreciate your commitment to testing (see my work on mm/vma.c - I
> care passionately about testing) - but we must make sure we are actually
> testing what we mean to.
>
I'm also passionate about testing :-)

Maybe there is a mis-understanding ? I explained the purpose of this
patch set in various responses, maybe not directly to your email though.

Even though the number of lines is large in these patches, its main
intention is to test Pedro's in-place change (from can_modify_mm to
can_modify_vma). Before this patch,  the test had 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 were
feedback to me during in-loop change that selftest should be extensive
enough to discover all regressions.  Even though this viewpoint is subject
to debate. Since none would want to do it, I thought I would just do it.

So the Patch V3 1/5 is dedicated entirely to increasing the verification
for existing scenarios, this including checking return code code, vma-size,
etc after mm api return.

Patch V3 3/5 are for unmap(), during review of V2 of Pedro's in-loop
change, we discovered a bug in unmap(), and unmap() is not atomic.
This leads to 4/5(mmap), 5/5(mremap), which calls munmap().
In addition, I add scenarios to cover cross-multiple-vma cases.

The  high-level goal of mseal test are two folds:
1> make sure sealing is working correctly under different scenarios,
i.e. sealed mapping are not modified.
2> For unsealed memory, added mseal code  doesn't regress on regular mm API.

The goal 2 is as important as 1, that is why tests usually are done in
two phases, one with sealing, the other without.

> So I suggest as a constructive way forward - firstly, submit a regression
> test for the change Liam wrapped into his series regarding the -EPERM
> thing.
>
I could work on this (to split the patch further) if this helps
acceptance of the patch series.

However, since the merge window is closer, everyone is busy, and it
is not really urgent to get it merged.  the added tests  already
passed in the linux-next branch,  we could wait till after
merge-window to review/perfect those tests.

> This should go in uncontroversially, I will take the time to review it and
> I don't see why that shouldn't be relatively straight forward. I will drop
> the concerns about things like test macros etc. for that.
>
> Then after that, I suggest we have a discussion about - at a higher level -
> what it is you want to test. And then between me, you, Liam and Pedro -
> ahead of time, list out the tests that we want, with all of us reaching
> consensus.
>
> I also suggest we figure out this FAIL_TEST_IF_FALSE() thing at this point
> too - I may be missing something, but I cannot for the life me understand
> why we have to assert negations only, and other self tests do not do this.
>
My most test-infra related comments comes from Muhammad Usama Anjum
(added into this email), e.g. assert is not recommended.[1] ,

[1] https://lore.kernel.org/all/[e-mail]/

> I have replied to a few sample points below.
>
> All of us simply want to help make sure mseal works as well as it can, this
> is the only motivation at play here.
>
> Hope you have a great weekend!
>

Thanks
Hope a great weekend too !
-Jeff


-Jeff

> Cheers, Lorenzo
>
> > > >
> > > > This commit message is woefully small. I told you on v1 to improve the
> > > > commit messages. Linus has told you to do this before.
> > > >
> > > > Please actually respond to feedback. Thanks.
> > > >
> > > > >
> > > > > Signed-off-by: Jeff Xu <[e-mail]>
> > > > > ---
> > > > >  tools/testing/selftests/mm/mseal_test.c | 126 +++++++++++++++++++++++-
> > > > >  1 file changed, 125 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
> > > > > index e855c8ccefc3..3516389034a7 100644
> > > > > --- a/tools/testing/selftests/mm/mseal_test.c
> > > > > +++ b/tools/testing/selftests/mm/mseal_test.c
> > > > > @@ -2222,6 +2222,123 @@ static void test_munmap_free_multiple_ranges(bool seal)
> > > > >     REPORT_TEST_PASS();
> > > > >  }
> > > > >
> > > > > +static void test_seal_mmap_expand_seal_middle(bool seal)
> > > >
> > > > This test doesn't expand, doesn't do anything in the middle. It does mmap()
> > > > though and relates to mseal, so that's something... this is compeltely
> > > > misnamed and needs to be rethought.
> > > >
> > >
> > > OK correction - it _seals_ in the middle. The remained of the criticism remains,
> > > and this is rather confusing... and I continue to wonder what the purpose of
> > > this is?
> > >
> > It expands the size (start from ptr).
> >
> > > > > +{
> > > > > +   void *ptr;
> > > > > +   unsigned long page_size = getpagesize();
> > > > > +   unsigned long size = 12 * page_size;
> > > > > +   int ret;
> > > > > +   void *ret2;
> > > > > +   int prot;
> > > > > +
> > > > > +   setup_single_address(size, &ptr);
> > > >
> > > > Please replace every single instance of this with an mmap(). There's
> > > > literally no reason to abstract it. And munmap() what you map.
> > > >
> > No, we need to abstract it.  In addition to the mmap, it also
> > allocates an additional two blocks before and after the allocated
> > memory, to avoid auto-merging, so we can use get_vma_size.
>
> It doesn't?
>
> static void setup_single_address(int size, void **ptrOut)
> {
>         void *ptr;
>
>         ptr = mmap(NULL, size, PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
>         *ptrOut = ptr;
> }
>
> >
> > > > > +   FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > > >
> > > > Pretty sure Pedro pointed out you should be checking against MAP_FAILED
> > > > here. I really don't understand why the rest of your test is full of
> > > > mmap()'s but for some reason you choose to abstract this one call? What?
> > > >
> > > > > +   /* ummap last 4 pages. */
> > > > > +   ret = sys_munmap(ptr + 8 * page_size, 4 * page_size);
> > > >
> > > > sys_munmap()? What's wrong with munmap()?
> > > >
> > > > > +   FAIL_TEST_IF_FALSE(!ret);
> > > >
> > > > Why do we not have a FAIL_TEST_IF_TRUE()? This is crazy.
> > > >
> > > > Would be nice to have something human-readable like ASSERT_EQ() or
> > > > ASSERT_TRUE() or ASSERT_FALSE().
> > > >
> > ASSERT_EQ and ASSERT_TURE are not recommended by the self-test. The
> > FAIL_TEST_IF_FAIL wrap will take care of some of the admin tasks
> > related to self-test infra, such as count how many tests are failing.
>
> Can you please point me to where it says you should implement your own
> macro that only tests the negation of an expression?
>
> I have found other self tests that do.
>
> >
> > > > > +
> > > > > +   size = get_vma_size(ptr, &prot);
> > > > > +   FAIL_TEST_IF_FALSE(size == 8 * page_size);
> > > > > +   FAIL_TEST_IF_FALSE(prot == 0x4);
> > > > > +
> > > > > +   if (seal) {
> > > > > +           ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
> > > > > +           FAIL_TEST_IF_FALSE(!ret);
> > > > > +   }
> > > > > +
> > > > > +   /* use mmap to expand and overwrite (MAP_FIXED)  */
> > > >
> > > > You don't really need to say MAP_FIXED, it's below.
> > > >
> > Adding a comment here to help reviewers.
> >
> > > > > +   ret2 = mmap(ptr, 12 * page_size, PROT_READ,
> > > > > +                   MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
> > > >
> > > > Why read-only?
> > > >
> > > > You're not expanding you're overwriting. You're not doing anything in the
> > > > middle.
> > > >
> > The MAP_FIXED is overwriting.  It also expands the address range
> > (start from ptr) from 8 to 12 pages.
> >
> > > > I'm again confused about what you think you're testing here. I don't think
> > > > we need an arbitrary MAP_FIXED mmap() at a size larger than the overwritten
> > > > VMA?
> > > >
> > > > You just need a single instance of a MAP_FIXED mmap() over a sealed mmap()
> > > > if that's what you want.
> > > >
> > > > > +   if (seal) {
> > > > > +           FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
> > > > > +           FAIL_TEST_IF_FALSE(errno == EPERM);
> > > > > +
> > > > > +           size = get_vma_size(ptr, &prot);
> > > > > +           FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > > > > +           FAIL_TEST_IF_FALSE(prot == 0x4);
> > > > > +
> > > > > +           size = get_vma_size(ptr + 4 * page_size, &prot);
> > > > > +           FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > > > > +           FAIL_TEST_IF_FALSE(prot == 0x4);
> > > > > +   } else
> > > > > +           FAIL_TEST_IF_FALSE(ret2 == ptr);
> > > >
> > > > Don't do dangling else's after a big block.
> > > >
> > patch passed the checkpatch.pl for style check.
> >
> > > > > +
> > > > > +   REPORT_TEST_PASS();
> > > > > +}
> > > > > +
> > > > > +static void test_seal_mmap_shrink_seal_middle(bool seal)
> > > >
> > > > What's going on in the 'middle'? This test doesn't shrink, it overwrites
> > > > the beginning of a sealed VMA?
> > >
> > > Correction - the middle is sealed. Other points remain.
> > >
> > The mmap attempts to shrink the address range from 12 pages to 8 pages.
> >
> > > > > +{
> > > > > +   void *ptr;
> > > > > +   unsigned long page_size = getpagesize();
> > > > > +   unsigned long size = 12 * page_size;
> > > > > +   int ret;
> > > > > +   void *ret2;
> > > > > +   int prot;
> > > > > +
> > > > > +   setup_single_address(size, &ptr);
> > > > > +   FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > > > > +
> > > > > +   if (seal) {
> > > > > +           ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
> > > > > +           FAIL_TEST_IF_FALSE(!ret);
> > > > > +   }
> > > > > +
> > > > > +   /* use mmap to shrink and overwrite (MAP_FIXED)  */
> > > >
> > > > What exactly are you shrinking? You're overwriting the start of the vma?
> > > >
> > > > What is this testing that is different from the previous test? This seems
> > > > useless honestly.
> > > >
> > Again, as above, one test is expanding, the other test is shrinking.
> > Please take a look at mmap parameters and steps before mmap call.
> >
> >
> > > > > +   ret2 = mmap(ptr, 7 * page_size, PROT_READ,
> > > > > +                   MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
> > > > > +   if (seal) {
> > > > > +           FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
> > > > > +           FAIL_TEST_IF_FALSE(errno == EPERM);
> > > > > +
> > > > > +           size = get_vma_size(ptr, &prot);
> > > > > +           FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > > > > +           FAIL_TEST_IF_FALSE(prot == 0x4);
> > > >
> > > > What the hell is this comparison to magic numbers? This is
> > > > ridiculous. What's wrong with PROT_xxx??
> > > >
> > The PROT_xxx can't be used here.
> > get_vma_size doesn't return PROT_ type, i.e. the bit sequence is different.
> >
> > > > > +
> > > > > +           size = get_vma_size(ptr + 4 * page_size, &prot);
> > > > > +           FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > > > > +           FAIL_TEST_IF_FALSE(prot == 0x4);
> > > > > +
> > > > > +           size = get_vma_size(ptr + 4 * page_size, &prot);
> > > > > +           FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > > > > +           FAIL_TEST_IF_FALSE(prot == 0x4);
> > > >
> > > > Err dude, you're doing this twice?
> > > >
> > The second get_vma_size should be (ptr + 8 * page_size)
> > I will update that.
> >
> > > > So what are we testing here exactly? That we got a VMA split? This is
> > > > err... why are we asserting this?
> > >
> > > I guess, that we can't overwrite a sealed bit of a VMA at the end. But again
> > > this feels entirely redundant. For this kind of thing to fail would mean the
> > > whole VMA machinery is broken.
> > >
> > The test is testing mmap(MAP_FIXED), since it can be used to overwrite
> > the sealed memory range (without sealing), then there is a variant of
> > expand/shrink.
> >
> >
> > > >
> > > > > +   } else
> > > > > +           FAIL_TEST_IF_FALSE(ret2 == ptr);
> > > > > +
> > > > > +   REPORT_TEST_PASS();
> > > > > +}
> > > > > +
> > > > > +static void test_seal_mmap_reuse_addr(bool seal)
> > > >
> > > > This is wrong, you're not reusing anything. This test is useless.
> > > >
> > The ptr is reused as a hint.
> >
> > > > > +{
> > > > > +   void *ptr;
> > > > > +   unsigned long page_size = getpagesize();
> > > > > +   unsigned long size = page_size;
> > > > > +   int ret;
> > > > > +   void *ret2;
> > > > > +   int prot;
> > > > > +
> > > > > +   setup_single_address(size, &ptr);
> > > > > +   FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > > > > +
> > > > > +   if (seal) {
> > > > > +           ret = sys_mseal(ptr, size);
> > > > > +           FAIL_TEST_IF_FALSE(!ret);
> > > >
> > > > We could avoid this horrid ret, ret2 naming if you just did:
> > > >
> > > >       FAIL_TEST_IF_FALSE(sys_mseal(ptr, size));
> > > >
> > > > > +   }
> > > > > +
> > > > > +   /* use mmap to change protection. */
> > > > > +   ret2 = mmap(ptr, size, PROT_NONE,
> > > > > +                   MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> > > >
> > > > How are you using mmap to change the protection when you're providing a
> > > > hint to the address to use? You're not changing any protection at all!
> > > >
> > It is necessary to add the this tests to make sure mseal is behave as
> > it should be, which is !MAP_FIXED case, new address will be allocated,
> > instead of fail of mmap()
> >
> >
> > > > You're allocating an entirely new VMA hinting that you want it near
> > > > ptr. Please read the man page for mmap():
> > > >
> > > >        If addr is NULL, then the kernel chooses the (page-aligned) address
> > > >        at which to create the mapping; this is the most portable method of
> > > >        creating a new mapping.  If addr is not NULL, then the kernel takes
> > > >        it as a hint about where to place the mapping; on Linux, the kernel
> > > >        will pick a nearby page boundary (but always above or equal to the
> > > >        value specified by /proc/sys/vm/mmap_min_addr) and attempt to create
> > > >        the mapping there.  If another mapping already exists there, the
> > > >        kernel picks a new address that may or may not depend on the hint.
> > > >        The address of the new mapping is returned as the result of the
> > > >        call.
> > > >
> > > > > +
> > > > > +   /* MAP_FIXED is not used, expect new addr */
> > > > > +   FAIL_TEST_IF_FALSE(!(ret2 == MAP_FAILED));
> > > >
> > > > This is beyond horrible. You really have to add more asserts.
> > > >
> > Again assert is not recommended by self_test
> >
> > > > Also you're expecting a new address here, so again, what on earth are you
> > > > asserting? That we can mmap()?
> > > >
> > > > > +   FAIL_TEST_IF_FALSE(ret2 != ptr);
> > > > > +
> > > > > +   size = get_vma_size(ptr, &prot);
> > > > > +   FAIL_TEST_IF_FALSE(size == page_size);
> > > > > +   FAIL_TEST_IF_FALSE(prot == 0x4);
> > > > > +
> > > > > +   REPORT_TEST_PASS();
> > > > > +}
> > > > > +
> > > > >  int main(int argc, char **argv)
> > > > >  {
> > > > >     bool test_seal = seal_support();
> > > > > @@ -2243,7 +2360,7 @@ int main(int argc, char **argv)
> > > > >     if (!get_vma_size_supported())
> > > > >             ksft_exit_skip("get_vma_size not supported\n");
> > > > >
> > > > > -   ksft_set_plan(91);
> > > > > +   ksft_set_plan(97);
> > > >
> > > > I'm guessing this is the number of tests, but I mean this is horrible. Is
> > > > there not a better way of doing this?
> > > >
> > Again, this is recommended by self-test.
> >
> >
> >
> > > > >
> > > > >     test_seal_addseal();
> > > > >     test_seal_unmapped_start();
> > > > > @@ -2357,5 +2474,12 @@ int main(int argc, char **argv)
> > > > >     test_munmap_free_multiple_ranges(false);
> > > > >     test_munmap_free_multiple_ranges(true);
> > > > >
> > > > > +   test_seal_mmap_expand_seal_middle(false);
> > > > > +   test_seal_mmap_expand_seal_middle(true);
> > > > > +   test_seal_mmap_shrink_seal_middle(false);
> > > > > +   test_seal_mmap_shrink_seal_middle(true);
> > > > > +   test_seal_mmap_reuse_addr(false);
> > > > > +   test_seal_mmap_reuse_addr(true);
> > > > > +
> > > > >     ksft_finished();
> > > > >  }
> > > > > --
> > > > > 2.46.0.469.g59c65b2a67-goog
> > > > >

Jeff Xu Sept. 13, 2024, 11 p.m. UTC | #8

Hi Pedro

On Sun, Sep 8, 2024 at 2:56 PM Pedro Falcato <[e-mail]> wrote:
>
> On Sun, Sep 8, 2024 at 10:35 PM Pedro Falcato <[e-mail]> wrote:
> > Hi,
> >
> > I agree with most of the points. Sitting down here to write unofficial
> > guidelines for mseal behavior.
> >
> > mseal should seal regions and mark them immutable, which means their protection
> > and contents (ish?) (not _only_ backing mapping, but also contents in general
> > (see madvise below)) should not be changed throughout the lifetime of the address space.
> >
> > For the general syscall interface, this means:
> > 1) mprotect and munmap need to be blocked on mseal regions.
> >  1a) munmap _cannot_ tolerate partial failure, per POSIX.
> >  2b) mmap MAP_FIXED counts as an unmap operation and also needs to be blocked and return -EPERM.
> >
> > 2) Most madvise calls are allowed, except for destructive operations on
> > read-only anonymous _pages_ (MADV_DONTNEED is destructive for anon, but we also don't care
> > about blocking these ops if we can do it manually with e.g memset)
> >  2a) The current implementation only blocks discard on anonymous _regions_, which is slightly
> >      different. We probably do want to block these on MAP_PRIVATE file mappings, as to block
> >      stuff like madvise MADV_DONTNEED on program rodata.
> >  2b) We take into account pkeys when doing the permission checks.
> >
> > 3) mremap is not allowed as we'd change the "contents" of the old region.
> >  3a) Should mremap expansion be allowed? aka only block moving and shrinking, but allow expansion.
> >      We already informally allow expansion if e.g mmapping after it + mseal.
> >
> > 4) mlock and msync are allowed.
> >
> > 5) mseal is blocked.
> >
> > 6) Other miscellaneous syscalls (mbind, etc) that do not change contents in any way, are allowed.
> >  6a) This obviously means PTEs can change as long as the contents don't. Swapping is also ok.
> >
> > 7) FOLL_FORCE (kernel-internal speak, more commonly seen as ptrace and /proc/self/mem from userspace)
> >    should be disallowed (?)
> >  7a) This currently does not happen, and seems like a large hole? But disallowing this
> >      would probably severely break ptrace if the ELF sealing plans come to fruition.
> >
> > When we say "disallowed", we usually (apart from munmap) allow for partial failure. This
> > means getting an -EPERM while part of the call succeeded, if we e.g mprotect a region consisting
> > of [NORMAL VMA][SEALED VMA]. We do not want to test for this, because we do not want to paint ourselves
> > into a corner - this is strictly "undefined behavior". The msealed regions themselves
> > will never be touched in such cases. (we do however want to test munmap operation atomicity, but this is
> > also kind of a munmap-related test, and might not really be something we really want in the mseal tests)
> >
> > Kernel-internal wise: The VMA and PTE modifications resulting from the above operations are blocked.
> > Sealed VMAs allow splitting and merging; there was contention about the splitting issue, but it truly
> > does not make sense to block operations unless they affect a VMA entirely, and that would also force
> > VMA merging to be ABI ("vma_merge isn't merging these two regions and now my madvise works/doesn't work :(").
> >
> >
> > Do I have everything right? Am I missing anything?
>
> Small addendum: file write, truncate and hole punching might also
> matter for sealed file-backed regions, as these change the region's
> contents, but we probably
> want to rely on file write permissions to protect against this (as we
> already do). Any other solution is probably terrible and probably
> endlessly NAK'd by fs folks, but it does
> mean sealed regions aren't really immutable if you or the attacker can
> write to the file.
>
Right. The mseal protects the control-data of VMA (e.g. prot),
it doesn't do anything more than that. The file permission relies on
dac/mac control.


-Jeff


> --
> Pedro

Jeff Xu Sept. 13, 2024, 11 p.m. UTC | #9

Hi Pedro

On Sun, Sep 8, 2024 at 2:35 PM Pedro Falcato <[e-mail]> wrote:
>
> I agree with most of the points. Sitting down here to write unofficial
> guidelines for mseal behavior.
>
> mseal should seal regions and mark them immutable, which means their protection
> and contents (ish?) (not _only_ backing mapping, but also contents in general
> (see madvise below)) should not be changed throughout the lifetime of the address space.
>
> For the general syscall interface, this means:
> 1) mprotect and munmap need to be blocked on mseal regions.
>  1a) munmap _cannot_ tolerate partial failure, per POSIX.
>  2b) mmap MAP_FIXED counts as an unmap operation and also needs to be blocked and return -EPERM.
>
> 2) Most madvise calls are allowed, except for destructive operations on
> read-only anonymous _pages_ (MADV_DONTNEED is destructive for anon, but we also don't care
> about blocking these ops if we can do it manually with e.g memset)
>  2a) The current implementation only blocks discard on anonymous _regions_, which is slightly
>      different. We probably do want to block these on MAP_PRIVATE file mappings, as to block
>      stuff like madvise MADV_DONTNEED on program rodata.
>  2b) We take into account pkeys when doing the permission checks.
>
> 3) mremap is not allowed as we'd change the "contents" of the old region.
>  3a) Should mremap expansion be allowed? aka only block moving and shrinking, but allow expansion.
>      We already informally allow expansion if e.g mmapping after it + mseal.
>
> 4) mlock and msync are allowed.
>
> 5) mseal is blocked.
mseal is not blocked, i.e. seal on an already sealed memory is
no-op. This is described in mseal.rst [1]

[1] https://github.com/torvalds/linux/blob/master/Documentation/userspace-api/mseal.rst

>
> 6) Other miscellaneous syscalls (mbind, etc) that do not change contents in any way, are allowed.
>  6a) This obviously means PTEs can change as long as the contents don't. Swapping is also ok.
>
> 7) FOLL_FORCE (kernel-internal speak, more commonly seen as ptrace and /proc/self/mem from userspace)
>    should be disallowed (?)
>  7a) This currently does not happen, and seems like a large hole? But disallowing this
>      would probably severely break ptrace if the ELF sealing plans come to fruition.
>
Jann Horn  pointed out FOLL_FORCE during RFC [2], and this is in mseal.rst too.

In short, FOLL_FORCE is not covered by mseal. On ChromeOS, FOLL_FORCE
is disabled. Recently, Adrian Ratiu upstreamed that [3]

[2] https://lore.kernel.org/lkml/[e-mail]/
[3]   https://lore.kernel.org/lkml/[e-mail]/

-Jeff

> When we say "disallowed", we usually (apart from munmap) allow for partial failure. This
> means getting an -EPERM while part of the call succeeded, if we e.g mprotect a region consisting
> of [NORMAL VMA][SEALED VMA]. We do not want to test for this, because we do not want to paint ourselves
> into a corner - this is strictly "undefined behavior". The msealed regions themselves
> will never be touched in such cases. (we do however want to test munmap operation atomicity, but this is
> also kind of a munmap-related test, and might not really be something we really want in the mseal tests)
>
> Kernel-internal wise: The VMA and PTE modifications resulting from the above operations are blocked.
> Sealed VMAs allow splitting and merging; there was contention about the splitting issue, but it truly
> does not make sense to block operations unless they affect a VMA entirely, and that would also force
> VMA merging to be ABI ("vma_merge isn't merging these two regions and now my madvise works/doesn't work :(").
>
>
> Do I have everything right? Am I missing anything?
>
> --
> Pedro

Patch

diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
index e855c8ccefc3..3516389034a7 100644
--- a/tools/testing/selftests/mm/mseal_test.c
+++ b/tools/testing/selftests/mm/mseal_test.c
@@ -2222,6 +2222,123 @@  static void test_munmap_free_multiple_ranges(bool seal)
 	REPORT_TEST_PASS();
 }
 
+static void test_seal_mmap_expand_seal_middle(bool seal)
+{
+	void *ptr;
+	unsigned long page_size = getpagesize();
+	unsigned long size = 12 * page_size;
+	int ret;
+	void *ret2;
+	int prot;
+
+	setup_single_address(size, &ptr);
+	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
+	/* ummap last 4 pages. */
+	ret = sys_munmap(ptr + 8 * page_size, 4 * page_size);
+	FAIL_TEST_IF_FALSE(!ret);
+
+	size = get_vma_size(ptr, &prot);
+	FAIL_TEST_IF_FALSE(size == 8 * page_size);
+	FAIL_TEST_IF_FALSE(prot == 0x4);
+
+	if (seal) {
+		ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
+		FAIL_TEST_IF_FALSE(!ret);
+	}
+
+	/* use mmap to expand and overwrite (MAP_FIXED)  */
+	ret2 = mmap(ptr, 12 * page_size, PROT_READ,
+			MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
+	if (seal) {
+		FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
+		FAIL_TEST_IF_FALSE(errno == EPERM);
+
+		size = get_vma_size(ptr, &prot);
+		FAIL_TEST_IF_FALSE(size == 4 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+
+		size = get_vma_size(ptr + 4 * page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 4 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	} else
+		FAIL_TEST_IF_FALSE(ret2 == ptr);
+
+	REPORT_TEST_PASS();
+}
+
+static void test_seal_mmap_shrink_seal_middle(bool seal)
+{
+	void *ptr;
+	unsigned long page_size = getpagesize();
+	unsigned long size = 12 * page_size;
+	int ret;
+	void *ret2;
+	int prot;
+
+	setup_single_address(size, &ptr);
+	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
+
+	if (seal) {
+		ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
+		FAIL_TEST_IF_FALSE(!ret);
+	}
+
+	/* use mmap to shrink and overwrite (MAP_FIXED)  */
+	ret2 = mmap(ptr, 7 * page_size, PROT_READ,
+			MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
+	if (seal) {
+		FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
+		FAIL_TEST_IF_FALSE(errno == EPERM);
+
+		size = get_vma_size(ptr, &prot);
+		FAIL_TEST_IF_FALSE(size == 4 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+
+		size = get_vma_size(ptr + 4 * page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 4 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+
+		size = get_vma_size(ptr + 4 * page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 4 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	} else
+		FAIL_TEST_IF_FALSE(ret2 == ptr);
+
+	REPORT_TEST_PASS();
+}
+
+static void test_seal_mmap_reuse_addr(bool seal)
+{
+	void *ptr;
+	unsigned long page_size = getpagesize();
+	unsigned long size = page_size;
+	int ret;
+	void *ret2;
+	int prot;
+
+	setup_single_address(size, &ptr);
+	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
+
+	if (seal) {
+		ret = sys_mseal(ptr, size);
+		FAIL_TEST_IF_FALSE(!ret);
+	}
+
+	/* use mmap to change protection. */
+	ret2 = mmap(ptr, size, PROT_NONE,
+			MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
+
+	/* MAP_FIXED is not used, expect new addr */
+	FAIL_TEST_IF_FALSE(!(ret2 == MAP_FAILED));
+	FAIL_TEST_IF_FALSE(ret2 != ptr);
+
+	size = get_vma_size(ptr, &prot);
+	FAIL_TEST_IF_FALSE(size == page_size);
+	FAIL_TEST_IF_FALSE(prot == 0x4);
+
+	REPORT_TEST_PASS();
+}
+
 int main(int argc, char **argv)
 {
 	bool test_seal = seal_support();
@@ -2243,7 +2360,7 @@  int main(int argc, char **argv)
 	if (!get_vma_size_supported())
 		ksft_exit_skip("get_vma_size not supported\n");
 
-	ksft_set_plan(91);
+	ksft_set_plan(97);
 
 	test_seal_addseal();
 	test_seal_unmapped_start();
@@ -2357,5 +2474,12 @@  int main(int argc, char **argv)
 	test_munmap_free_multiple_ranges(false);
 	test_munmap_free_multiple_ranges(true);
 
+	test_seal_mmap_expand_seal_middle(false);
+	test_seal_mmap_expand_seal_middle(true);
+	test_seal_mmap_shrink_seal_middle(false);
+	test_seal_mmap_shrink_seal_middle(true);
+	test_seal_mmap_reuse_addr(false);
+	test_seal_mmap_reuse_addr(true);
+
 	ksft_finished();
 }