linux-kernel - Re: [PATCH] lib/test_vmalloc.c: avoid garbage in page array

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: <b87ff2af-c89d-4ddd-8992-2ffb337fbe0c@lucifer.local>
Date:   Fri, 26 May 2023 08:13:07 +0100
From:   Lorenzo Stoakes <lstoakes@...il.com>
To:     Baoquan He <bhe@...hat.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Christoph Hellwig <hch@...radead.org>,
        Uladzislau Rezki <urezki@...il.com>
Subject: Re: [PATCH] lib/test_vmalloc.c: avoid garbage in page array

On Fri, May 26, 2023 at 08:08:33AM +0800, Baoquan He wrote:
> On 05/24/23 at 09:24am, Lorenzo Stoakes wrote:
> > It turns out that alloc_pages_bulk_array() does not treat the page_array
> > parameter as an output parameter, but rather reads the array and skips any
> > entries that have already been allocated.
>
> I read __alloc_pages_bulk() carefully, it does store the allocated page
> pointers into page_array[] and pass out, not just reads the array and
> skips entry alreay allocated.

Umm, the function literally opens with:-

	/*
	 * Skip populated array elements to determine if any pages need
	 * to be allocated before disabling IRQs.
	 */
	while (page_array && nr_populated < nr_pages && page_array[nr_populated])
		nr_populated++;

And then later:-

		/* Skip existing pages */
		if (page_array && page_array[nr_populated]) {
			nr_populated++;
			continue;
		}

This explicitly skips populated array entries and reads page_array to see
if entries already exist, and literally documents this in the comments
above each line, exactly as I describe.

>
> For the issue this patch is trying to fix, you mean __alloc_pages_bulk()
> doesn't initialize page_array intentionally if it doesn't successfully
> allocate desired number of pages. we may need add one sentence to notice
> user that page_array need be initialized explicitly.

It isn't 'trying' to fix it, it fixes it. I have this reproing locally.

What you're stating about 'successfully allocate desired number of pages'
is irrelevant, we literally check the number of allocated pages in the
caller.

No sentences need to be added, I explicitly state that the issue is due to
the array being uninitialised, the summary lines talks about reading
garbage.

>
> By the way, could you please tell in which line the test was referencing
> uninitialized data and causing the PFN to not be valid and trigger the
> WANR_ON? Please forgive my dumb head.

Well, I showed you the lines above where __alloc_bulk_array() is accessing
uninitialised data by reading page_array[].

But ultimately this is called from vm_map_ram_test() in lib/test_vmalloc.c:-

	for (i = 0; i < test_loop_count; i++) {
		v_ptr = vm_map_ram(pages, map_nr_pages, NUMA_NO_NODE);
^--- triggers warning because we can't map the invalid PFN
		*v_ptr = 'a';
^--- NULL pointer deref
		vm_unmap_ram(v_ptr, map_nr_pages);
	}

The warning is triggered in:-

vm_map_ram()
vmap_pages_range()
vmap_pages_range_noflush()
__vmap_pages_range_noflush()
vmap_pages_p4d_range()
vmap_pages_pud_range()
vmap_pages_pmd_range()
vmap_pages_pte_range()

In:-

		if (WARN_ON(!pfn_valid(page_to_pfn(page))))
			return -EINVAL;

The PFN is invalid because I happen to have garbage in an array entry such
that page_to_pfn(garbage) >= max_pfn.

> >
> > This is somewhat unexpected and breaks this test, as we allocate the pages
> > array uninitialised on the assumption it will be overwritten.
> >
> > As a result, the test was referencing uninitialised data and causing the
> > PFN to not be valid and thus a WARN_ON() followed by a null pointer deref
> > and panic.
> >
> > In addition, this is an array of pointers not of struct page objects, so we
> > need only allocate an array with elements of pointer size.
> >
> > We solve both problems by simply using kcalloc() and referencing
> > sizeof(struct page *) rather than sizeof(struct page).
> >
> > Signed-off-by: Lorenzo Stoakes <lstoakes@...il.com>
> > ---
> >  lib/test_vmalloc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/test_vmalloc.c b/lib/test_vmalloc.c
> > index 9dd9745d365f..3718d9886407 100644
> > --- a/lib/test_vmalloc.c
> > +++ b/lib/test_vmalloc.c
> > @@ -369,7 +369,7 @@ vm_map_ram_test(void)
> >  	int i;
> >
> >  	map_nr_pages = nr_pages > 0 ? nr_pages:1;
> > -	pages = kmalloc(map_nr_pages * sizeof(struct page), GFP_KERNEL);
> > +	pages = kcalloc(map_nr_pages, sizeof(struct page *), GFP_KERNEL);
> >  	if (!pages)
> >  		return -1;
> >
> > --
> > 2.40.1
> >
>

A broader problem we might want to think about is how little anybody is
running this test in order that it wasn't picked up before now... obviously
there's an element of luck as to whether the page_array happens to be
zeroed or not, but you'd think it'd be garbage filled at least a reasonable
amount of the time.