linux-kernel - Re: [PATCH] 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: <fca23df7-37b0-f32d-ece3-58317dfad210@quicinc.com>
Date:   Mon, 18 Jul 2022 19:28:13 +0530
From:   Charan Teja Kalla <quic_charante@...cinc.com>
To:     Michal Hocko <mhocko@...e.com>
CC:     <akpm@...ux-foundation.org>, <pasha.tatashin@...een.com>,
        <sjpark@...zon.de>, <sieberf@...zon.com>, <shakeelb@...gle.com>,
        <dhowells@...hat.com>, <willy@...radead.org>, <vbabka@...e.cz>,
        <david@...hat.com>, <minchan@...nel.org>,
        <linux-kernel@...r.kernel.org>, <linux-mm@...ck.org>,
        "iamjoonsoo.kim@....com" <iamjoonsoo.kim@....com>
Subject: Re: [PATCH] mm: fix use-after free of page_ext after race with
 memory-offline

Thanks Michal for the comments!!

On 7/18/2022 5:20 PM, Michal Hocko wrote:
>> The above mentioned race is just one example __but the problem persists
>> in the other paths too involving page_ext->flags access(eg:
>> page_is_idle())__. Since offline waits till the last reference on the
>> page goes down i.e. any path that took the refcount on the page can make
>> the memory offline operation to wait. Eg: In the migrate_pages()
>> operation, we do take the extra refcount on the pages that are under
>> migration and then we do copy page_owner by accessing page_ext. For
>>
>> Fix those paths where offline races with page_ext access by maintaining
>> synchronization with rcu lock.
> Please be much more specific about the synchronization. How does RCU
> actually synchronize the offlining and access? Higher level description
> of all the actors would be very helpful not only for the review but also
> for future readers.

I will improve the commit message about this synchronization change
using RCU's.

> 
> Also, more specifically
> [...]
>> diff --git a/mm/page_ext.c b/mm/page_ext.c
>> index 3dc715d..5ccd3ee 100644
>> --- a/mm/page_ext.c
>> +++ b/mm/page_ext.c
>> @@ -299,8 +299,9 @@ static void __free_page_ext(unsigned long pfn)
>>  	if (!ms || !ms->page_ext)
>>  		return;
>>  	base = get_entry(ms->page_ext, pfn);
>> -	free_page_ext(base);
>>  	ms->page_ext = NULL;
>> +	synchronize_rcu();
>> +	free_page_ext(base);
>>  }
> So you are imposing the RCU grace period for each page_ext! This can get
> really expensive. Have you tried to measure the effect?
> 

I didn't really measure the effect. Let me measure it and post these in V2.

> Is there any reason why page_ext is freed during offlining rather when
> it is hotremoved?

This is something I am struggling to get the answer. IMO, this is even
wrong design where I don't have page_ext but page. Moving the freeing of
page_ext to hotremove path actually solves the problem but somehow this
idea didn't liked[1].  copying the excerpt here:

">
> 3) Change the design where the page_ext is valid as long as the struct
> page is alive.

:/ Doesn't spark joy."


@Joonsoo : We see that you did commit eefa864b701d ("mm/page_ext:
resurrect struct page extending code for debugging").  Any reason why
the page_ext is chosen to free at offline operation rather than the
remove operation of a memory block?


[1]
https://lore.kernel.org/linux-mm/8fefe59d-c893-39f4-3225-65343086c867@redhat.com/
> 
> Thanks!