linux-kernel - Re: [PATCH] munmap sealed memory cause memory to split (bug)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3a1bbbdf-1281-4f44-857d-58cba583e3da@lucifer.local>
Date: Thu, 17 Oct 2024 09:18:20 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: jeffxu@...omium.org
Cc: akpm@...ux-foundation.org, keescook@...omium.org,
        torvalds@...ux-foundation.org, usama.anjum@...labora.com,
        corbet@....net, Liam.Howlett@...cle.com, jeffxu@...gle.com,
        jorgelo@...omium.org, groeck@...omium.org,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-mm@...ck.org, jannh@...gle.com, sroettger@...gle.com,
        pedro.falcato@...il.com, linux-hardening@...r.kernel.org,
        willy@...radead.org, gregkh@...uxfoundation.org, deraadt@...nbsd.org,
        surenb@...gle.com, merimus@...gle.com, rdunlap@...radead.org
Subject: Re: [PATCH] munmap sealed memory cause memory to split (bug)

NACK. Greg's bot got to it but...

As per Greg's bot, no signed-off-by line.

The subject should be something about adding a test.

You later say you are somehow dependning on things (what?) to make this work but
it's broken.

Jeff - you're doing things that were raised on previous reviews as if we
never said them. It's starting to get annoying now. Please try to listen to
upstream.

On Thu, Oct 17, 2024 at 02:26:27AM +0000, jeffxu@...omium.org wrote:
> From: Jeff Xu <jeffxu@...gle.com>
>
> It appears there is a regression on the latest mm,
> when munmap sealed memory, it can cause unexpected VMA split.
> E.g. repro use this test.

This is an unacceptably short commit message. You've been told about this
before.

> ---
>  tools/testing/selftests/mm/mseal_test.c | 76 +++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
>
> diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
> index fa74dbe4a684..0af33e13b606 100644
> --- a/tools/testing/selftests/mm/mseal_test.c
> +++ b/tools/testing/selftests/mm/mseal_test.c
> @@ -1969,6 +1969,79 @@ static void test_madvise_filebacked_was_writable(bool seal)
>  	REPORT_TEST_PASS();
>  }
>
> +static void test_munmap_free_multiple_ranges_with_split(bool seal)
> +{
> +	void *ptr;
> +	unsigned long page_size = getpagesize();
> +	unsigned long size = 12 * page_size;
> +	int ret;
> +	int prot;
> +
> +	setup_single_address(size, &ptr);
> +	FAIL_TEST_IF_FALSE(ptr != (void *)-1);

I'm not going to accept any test where you do:

	FAIL_TEST_IF_FALSE(<negation>)

As that's totally unreadable. I asked you before for justification and you
didn't provide it, no other tests appear to do this, I wrote thousands of
lines of tests recently without doing this - stop it.

Also referene MAP_FAILED here please. You've been told before.

> +
> +	/* seal the middle 4 page */
> +	if (seal) {
> +		ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
> +		FAIL_TEST_IF_FALSE(!ret);
> +
> +		size = get_vma_size(ptr, &prot);
> +		FAIL_TEST_IF_FALSE(size == 4 * page_size);
> +		FAIL_TEST_IF_FALSE(prot == 4);

Again, you've been told before, stop referencing numbers instead of
PROT_... flags.

OK I'm stopping at this point, you _must listen to review_ Jeff.

> +
> +		size = get_vma_size(ptr +  4 * page_size, &prot);
> +		FAIL_TEST_IF_FALSE(size == 4 * page_size);
> +		FAIL_TEST_IF_FALSE(prot == 4);
> +
> +		size = get_vma_size(ptr +  8 * page_size, &prot);
> +		FAIL_TEST_IF_FALSE(size == 4 * page_size);
> +		FAIL_TEST_IF_FALSE(prot == 4);
> +	}
> +
> +	/* munmap 4  pages from the third page */
> +	ret = sys_munmap(ptr + 2 * page_size, 4 * page_size);
> +	if (seal) {
> +		FAIL_TEST_IF_FALSE(ret);
> +		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 == 4);
> +
> +		size = get_vma_size(ptr +  4 * page_size, &prot);
> +		FAIL_TEST_IF_FALSE(size == 4 * page_size);
> +		FAIL_TEST_IF_FALSE(prot == 4);
> +
> +		size = get_vma_size(ptr +  8 * page_size, &prot);
> +		FAIL_TEST_IF_FALSE(size == 4 * page_size);
> +		FAIL_TEST_IF_FALSE(prot == 4);
> +	} else
> +		FAIL_TEST_IF_FALSE(!ret);
> +
> +	/* munmap 4 pages from the sealed page */
> +	ret = sys_munmap(ptr + 6 * page_size, 4 * page_size);
> +	if (seal) {
> +		FAIL_TEST_IF_FALSE(ret);
> +		FAIL_TEST_IF_FALSE(errno == EPERM);
> +
> +		size = get_vma_size(ptr + 4 * page_size, &prot);
> +		FAIL_TEST_IF_FALSE(size == 4 * page_size);
> +		FAIL_TEST_IF_FALSE(prot == 4);
> +
> +		size = get_vma_size(ptr +  4 * page_size, &prot);
> +		FAIL_TEST_IF_FALSE(size == 4 * page_size);
> +		FAIL_TEST_IF_FALSE(prot == 4);
> +
> +		size = get_vma_size(ptr +  8 * page_size, &prot);
> +		FAIL_TEST_IF_FALSE(size == 4 * page_size);
> +		FAIL_TEST_IF_FALSE(prot == 4);
> +	} else
> +		FAIL_TEST_IF_FALSE(!ret);
> +
> +	REPORT_TEST_PASS();
> +}
> +
> +
>  int main(int argc, char **argv)
>  {
>  	bool test_seal = seal_support();
> @@ -2099,5 +2172,8 @@ int main(int argc, char **argv)
>  	test_madvise_filebacked_was_writable(false);
>  	test_madvise_filebacked_was_writable(true);
>
> +	test_munmap_free_multiple_ranges_with_split(false);
> +	test_munmap_free_multiple_ranges_with_split(true);
> +
>  	ksft_finished();
>  }
> --
> 2.47.0.rc1.288.g06298d1525-goog
>