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: <ecada69b-03e4-4214-ed8e-f25ac74c19e1@redhat.com>
Date:   Tue, 19 Jul 2022 17:50:56 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Michal Hocko <mhocko@...e.com>
Cc:     Charan Teja Kalla <quic_charante@...cinc.com>,
        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,
        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

> 
>> 2) I really dislike having to scatter section online checks all over the
>> place in page ext code. Once there is a difference between active vs.
>> stale page ext data things get a bit messy and error prone. This is
>> already ugly enough in our generic memmap handling code IMHO.
> 
> They should represent a free page in any case so even they are stall
> they shouldn't be really dangerous, right?
Good question. The use-after-free tells me that there could at least be
something accessing page_ext data after offlining right now. As long as
it's only unsynchronized read access, we should be fine.

> 
>> 3) Having on-demand allocations, such as KASAN or page ext from the
>> memory online notifier is at least currently cleaner, because we don't
>> have to handle each and every subsystem that hooks into that during the
>> core memory hotadd/remove phase, which primarily only setups the
>> vmemmap, direct map and memory block devices.
> 
> Cannot this hook into __add_pages which is the real implementation of
> the arch independent way to allocate vmemmap. Or at the sparsemem level
> because we do not (and very likely won't) support memory hotplug on
> any other memory model.

As __add_pages() is also called from mm/memremap.c where we don't want
that metadata, we'd have to special-case (would need a new parameter I
guess).

> 
>> Personally, I think what we have in this patch is quite nice and clean.
>> But I won't object if it can be similarly done in a clean way from
>> hot(un)plug code.
> 
> Well, if the scheme can be done without synchronize_rcu for each section
> which can backfire and if the scheme doesn't add too much complexity to
> achieve that then sure I won't object. I just do not get why page_ext
> should have a different allocation lifetime expectancy than a real page.
> Quite confusing if you ask me.

In contrast to memmap, people actually test for zero pointers here.

If you ask me the memmap access is ugly enough and I don't really enjoy
other metadata following that pattern of "stale and suddenly removed".
Here seems to be an easy way to do it in a clean way.

But yes, if the synchronize_rcu turns out problematic, we'd either have
to optimize or move the allcoation/free phase.

-- 
Thanks,

David / dhildenb