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: <f7e1efa4-43da-22e0-b748-d0855ecc7456@quicinc.com>
Date:   Wed, 20 Jul 2022 16:13:19 +0530
From:   Charan Teja Kalla <quic_charante@...cinc.com>
To:     Michal Hocko <mhocko@...e.com>,
        Pavan Kondeti <quic_pkondeti@...cinc.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 & Pavan,

On 7/20/2022 2:40 PM, Michal Hocko wrote:
>>>> Thanks! The most imporant part is how the exclusion is actual achieved
>>>> because that is not really clear at first sight
>>>>
>>>> CPU1					CPU2
>>>> lookup_page_ext(PageA)			offlining
>>>> 					  offline_page_ext
>>>> 					    __free_page_ext(addrA)
>>>> 					      get_entry(addrA)
>>>> 					      ms->page_ext = NULL
>>>> 					      synchronize_rcu()
>>>> 					      free_page_ext
>>>> 					        free_pages_exact (now addrA is unusable)
>>>> 					
>>>>   rcu_read_lock()
>>>>   entryA = get_entry(addrA)
>>>>     base + page_ext_size * index # an address not invalidated by the freeing path
>>>>   do_something(entryA)
>>>>   rcu_read_unlock()
>>>>
>>>> CPU1 never checks ms->page_ext so it cannot bail out early when the
>>>> thing is torn down. Or maybe I am missing something. I am not familiar
>>>> with page_ext much.
>>>
>>> Thanks a lot for catching this Michal. You are correct that the proposed
>>> code from me is still racy. I Will correct this along with the proper
>>> commit message in the next version of this patch.
>>>
>> Trying to understand your discussion with Michal. What part is still racy? We
>> do check for mem_section::page_ext and bail out early from lookup_page_ext(),
>> no?
>>
>> Also to make this scheme explicit, we can annotate page_ext member with __rcu
>> and use rcu_assign_pointer() on the writer side.

Annotating with __rcu requires all the read and writes to ms->page_ext
to be under rcu_[access|assign]_pointer which is a big patch. I think
READ_ONCE and WRITE_ONCE, mentioned by Michal, below should does the job.

>>
>> 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);
>>         /*
>>          * The sanity checks the page allocator does upon freeing a
>>          * page can reach here before the page_ext arrays are
>>          * allocated when feeding a range of pages to the allocator
>>          * for the first time during bootup or memory hotplug.
>>          */
>>         if (!section->page_ext)
>>                 return NULL;
>>         return get_entry(section->page_ext, pfn);
>> }
> You are right. I was looking at the wrong implementation and misread
> ifdef vs. ifndef CONFIG_SPARSEMEM. My bad.
> 

There is still a small race window b/n ms->page_ext setting NULL and its
access even under CONFIG_SPARSEMEM. In the above mentioned example:

 CPU1					CPU2
 rcu_read_lock()
 lookup_page_ext(PageA):		offlining
 					  offline_page_ext
 					    __free_page_ext(addrA)
 					      get_entry(addrA)
    if (!section->page_ext)
       turns to be false.
 					      ms->page_ext = NULL
						
   addrA = get_entry(base=section->page_ext):
     base + page_ext_size * index;
     **Since base is NULL here, caller
     can still do the dereference on
     the invalid pointer address.**
						
 					      synchronize_rcu()
 					      free_page_ext
 					        free_pages_exact (now )


> Memory hotplug is not supported outside of CONFIG_SPARSEMEM so the
> scheme should really work. I would use READ_ONCE for ms->page_ext and
> WRITE_ONCE on the initialization side.

Yes, I should be using the READ_ONCE() and WRITE_ONCE() here.

Thanks,
Charan