linux-kernel - Re: [PATCH V4] mm: fix use-after free of page

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]

Date:   Tue, 23 Aug 2022 22:35:32 +0530
From:   Charan Teja Kalla <quic_charante@...cinc.com>
To:     David Hildenbrand <david@...hat.com>, <akpm@...ux-foundation.org>,
        <mhocko@...e.com>, <vbabka@...e.cz>, <pasha.tatashin@...een.com>,
        <shakeelb@...gle.com>, <sieberf@...zon.com>, <sjpark@...zon.de>,
        <william.kucharski@...cle.com>, <willy@...radead.org>,
        <quic_pkondeti@...cinc.com>, <minchan@...gle.com>
CC:     <linux-kernel@...r.kernel.org>, <linux-mm@...ck.org>
Subject: Re: [PATCH V4] mm: fix use-after free of page_ext after race with
 memory-offline

Thanks David for the inputs.

On 8/23/2022 6:39 PM, David Hildenbrand wrote:
>>  static ssize_t
>> @@ -508,6 +527,14 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>>  	/* Find an allocated page */
>>  	for (; pfn < max_pfn; pfn++) {
>>  		/*
>> +		 * This temporary page_owner is required so
>> +		 * that we can avoid the context switches while holding
>> +		 * the rcu lock and copying the page owner information to
>> +		 * user through copy_to_user() or GFP_KERNEL allocations.
>> +		 */
>> +		struct page_owner page_owner_tmp;
>> +
>> +		/*
>>  		 * If the new page is in a new MAX_ORDER_NR_PAGES area,
>>  		 * validate the area as existing, skip it if not
>>  		 */
>> @@ -525,7 +552,7 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>>  			continue;
>>  		}
>>  
>> -		page_ext = lookup_page_ext(page);
>> +		page_ext = page_ext_get(page);
>>  		if (unlikely(!page_ext))
>>  			continue;
>>  
>> @@ -534,14 +561,14 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>>  		 * because we don't hold the zone lock.
>>  		 */
>>  		if (!test_bit(PAGE_EXT_OWNER, &page_ext->flags))
>> -			continue;
>> +			goto loop;
>>  
>>  		/*
>>  		 * Although we do have the info about past allocation of free
>>  		 * pages, it's not relevant for current memory usage.
>>  		 */
>>  		if (!test_bit(PAGE_EXT_OWNER_ALLOCATED, &page_ext->flags))
>> -			continue;
>> +			goto loop;
>>  
>>  		page_owner = get_page_owner(page_ext);
>>  
>> @@ -550,7 +577,7 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>>  		 * would inflate the stats.
>>  		 */
>>  		if (!IS_ALIGNED(pfn, 1 << page_owner->order))
>> -			continue;
>> +			goto loop;
>>  
>>  		/*
>>  		 * Access to page_ext->handle isn't synchronous so we should
>> @@ -558,13 +585,17 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>>  		 */
>>  		handle = READ_ONCE(page_owner->handle);
>>  		if (!handle)
>> -			continue;
>> +			goto loop;
>>  
>>  		/* Record the next PFN to read in the file offset */
>>  		*ppos = (pfn - min_low_pfn) + 1;
>>  
>> +		page_owner_tmp = *page_owner;
>> +		page_ext_put(page_ext);
>>  		return print_page_owner(buf, count, pfn, page,
>> -				page_owner, handle);
>> +				&page_owner_tmp, handle);
>> +loop:
>> +		page_ext_put(page_ext);
>>  	}
>>  
>>  	return 0;
>> @@ -617,18 +648,20 @@ static void init_pages_in_zone(pg_data_t *pgdat, struct zone *zone)
>>  			if (PageReserved(page))
>>  				continue;
>>  
>> -			page_ext = lookup_page_ext(page);
>> +			page_ext = page_ext_get(page);
>>  			if (unlikely(!page_ext))
>>  				continue;
>>  
>>  			/* Maybe overlapping zone */
>>  			if (test_bit(PAGE_EXT_OWNER, &page_ext->flags))
>> -				continue;
>> +				goto loop;
>>  
>>  			/* Found early allocated page */
>>  			__set_page_owner_handle(page_ext, early_handle,
>>  						0, 0);
>>  			count++;
>> +loop:
>> +			page_ext_put(page_ext);
>>  		}
> I kind-of dislike the "loop" labels. Can we come up with a more
> expressive name?
> 
> "put_continue"
> 
> or something?
> 
> 
> One alternative would be to add to the beginning of the loop, and after
> the loop sth like
> 
> if (page_ext) {
> 	page_ext_put(page_ext);
> 	page_ext = NULL;
> }

I think, moving this to beginning of the loop looks cleaner than the
goto statement.  Will spin V5.


> 
> One could wrap that in a function, but not sure if that improves the
> situation.