linux-kernel - Re: [PATCH V2] 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]

Message-ID: <6b646ff2-b6f6-052e-f3f4-3bf05243f049@quicinc.com>
Date:   Fri, 29 Jul 2022 21:17:44 +0530
From:   Charan Teja Kalla <quic_charante@...cinc.com>
To:     Michal Hocko <mhocko@...e.com>
CC:     <akpm@...ux-foundation.org>, <david@...hat.com>,
        <quic_pkondeti@...cinc.com>, <pasha.tatashin@...een.com>,
        <sjpark@...zon.de>, <sieberf@...zon.com>, <shakeelb@...gle.com>,
        <dhowells@...hat.com>, <willy@...radead.org>, <minchan@...nel.org>,
        <linux-kernel@...r.kernel.org>, <linux-mm@...ck.org>
Subject: Re: [PATCH V2] mm: fix use-after free of page_ext after race with
 memory-offline

Thanks Michal for the reviews!!

On 7/28/2022 8:07 PM, Michal Hocko wrote:
>> FAQ's:
>> Q) Should page_ext_[get|put]() needs to be used for every page_ext
>> access?
>> A) NO, the synchronization is really not needed in all the paths of
>> accessing page_ext. One case is where extra refcount is taken on a
>> page for which memory block, this pages falls into, offline operation is
>> being performed. This extra refcount makes the offline operation not to
>> succeed hence the freeing of page_ext.  Another case is where the page
>> is already being freed and we do reset its page_owner.
> This is just subtlety and something that can get misunderstood over
> time. Moreover there is no documentation explaining the difference.
> What is the reason to have these two different APIs in the first place.
> RCU read side is almost zero cost. So what is the point?
Currently not all the places where page_ext is being used is put under
the rcu_lock. I just used rcu lock in the places where it is possible to
have the use-after-free of page_ext. You recommend to use rcu lock while
using with page_ext in all the places?

My only point here is since there may be a non-atomic context exist
across page_ext_get/put() and If users are sure that this page's
page_ext will not be freed by parallel offline operation, they need not
get the rcu lock.

I agree that this can be misunderstood over time, let me check if I can
use page_ext_get/put in all the places.

>> @@ -57,6 +60,11 @@ static inline void page_ext_init(void)
>>  
>>  struct page_ext *lookup_page_ext(const struct page *page);
>>  
>> +static inline bool page_ext_invalid(struct page_ext *page_ext)
>> +{
>> +	return !page_ext || (((unsigned long)page_ext & PAGE_EXT_INVALID) == 1);
>> +}
>> +
> No real reason to expose this into a header file. Nothing but page_ext.c
> should know and care about this.
Agree. Will move it accordingly.

> 
>> +static inline struct page_ext *page_ext_get(struct page *page)
>> +{
>> +	struct page_ext *page_ext;
>> +
>> +	rcu_read_lock();
>> +	page_ext = lookup_page_ext(page);
>> +	if (!page_ext) {
>> +		rcu_read_unlock();
>> +		return NULL;
>> +	}
>> +
>> +	return page_ext;
> If you make this an extern you can actually hide lookup_page_ext and
> prevent from future bugs where people are using non serialized API
> without realizing that.

This design looks good. Let me check the feasibility in its implementation.

>> diff --git a/mm/page_ext.c b/mm/page_ext.c
>> index 3dc715d..404a2eb 100644
>> --- a/mm/page_ext.c
>> +++ b/mm/page_ext.c
>> @@ -211,15 +211,17 @@ struct page_ext *lookup_page_ext(const struct page *page)
>>  {
>>  	unsigned long pfn = page_to_pfn(page);
>>  	struct mem_section *section = __pfn_to_section(pfn);
>> +	struct page_ext *page_ext = READ_ONCE(section->page_ext);
>> +
> 	WARN_ON_ONCE(!rcu_read_lock_held());

Again this requires page_ext usage should be under the rcu lock always
by the user.

> 
>>  static void *__meminit alloc_page_ext(size_t size, int nid)
>> @@ -298,9 +300,26 @@ static void __free_page_ext(unsigned long pfn)
>>  	ms = __pfn_to_section(pfn);
>>  	if (!ms || !ms->page_ext)
>>  		return;
>> -	base = get_entry(ms->page_ext, pfn);
>> +
>> +	base = READ_ONCE(ms->page_ext);
>> +	if (page_ext_invalid(base))
>> +		base = (void *)base - PAGE_EXT_INVALID;
> All page_ext accesses should use the same fetched pointer including the
> ms->page_ext check. Also page_ext_invalid _must_ be true here otherwise
> something bad is going on so I would go with
> 	if (WARN_ON_ONCE(!page_ext_invalid(base)))
> 		return;
> 	base = (void *)base - PAGE_EXT_INVALID;

The roll back operation in the online_page_ext(), where we free the
allocated page_ext's, will not have the PAGE_EXT_INVALID flag thus
WARN() may not work here. no?
> 

Thanks,
Charan