Re: [PATCH mm-hotfixes v5 3/5] x86/mm/pat: acquire init_mm read lock on attribute change to avoid UAF

[Lorenzo Stoakes (ARM)]

On Sat, Jul 18, 2026 at 01:54:50PM +0300, Mike Rapoport wrote:
> On Fri, Jul 17, 2026 at 06:30:09PM +0100, Lorenzo Stoakes (ARM) wrote:
> > A previous commit protected us against races between ptdump and CPA
> > collapse, however one still exists between attribute changes and collapse
> > as reported by Denis V. Lunev (linked).
> >
> > When an attribute change arises, a lockless page table walker obtains a PTE
> > entry, which is later written to via set_pte_atomic():
> >
> > ...
> > -> change_page_attr_set_clr()
> > -> __change_page_attr_set_clr()
> > -> __change_page_attr()
> > 	-> _lookup_address_cpa()
> > 	-> lookup_address_in_pgd_attr()
> > 	-> [ lockless page table walker ]
> > -> set_pte_atomic()
> >
> > There is nothing preventing a concurrent CPA collapse which can free the
> > PTE that was retrieved here, resulting in a use-after-free.
> >
> > With the mmap write lock taken on init_mm over CPA collapse, we can now
> > resolve this race by acquiring an mmap read lock on init_mm over
> > __change_page_attr_set_clr().
> >
> > This locks across the whole operation over which the walk and the PTE entry
> > write occurs, solving the race.
> >
> > It is safe to do this here, as no spinlocks are held upon entry to
> > __change_page_attr_set_clr().
> >
> > The CPA_COLLAPSE flag is only set by set_memory_rox(), which exclusively
> > operates upon vmalloc ranges, and on x86 only within the module mapping
> > space.
> >
> > This is important, because some callers directly invoke
> > __change_page_attr_set_clr(), bypassing this lock. However, none of these
> > operate within the module mapping space.
> >
> > * cpa_process_alias() - a recursive helper called by
> >   __change_page_attr_set_clr().
> > * __set_memory_enc_pgtable() - operates on the direct mapping and (via
> >   __vmbus_establish_gpadl()) the vmalloc mapping space.
> > * __set_pages_[n]p() - called by set_direct_map_[invalid, default,
> >   valid]_noflush(), __kernel_map_pages() - operates on the direct map.
> > * kernel_[un]map_pages_in_pgd() - operates on EFI ranges.
> >
> > This work is based upon Denis V. Lunev's excellent analysis of the bug with
> > gratitude.
> >
> > Link: https://lore.kernel.org/all/20260626163213.2284080-1-den@xxxxxxxxxx/
> > Fixes: 41d88484c71c ("x86/mm/pat: restore large ROX pages after fragmentation")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>
> > ---
> >  arch/x86/mm/pat/set_memory.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> > index d1e63f7d267f..301fb9e77d91 100644
> > --- a/arch/x86/mm/pat/set_memory.c
> > +++ b/arch/x86/mm/pat/set_memory.c
> > @@ -2122,7 +2122,9 @@ static int change_page_attr_set_clr(unsigned long *addr, int numpages,
> >  	cpa.curpage = 0;
> >  	cpa.force_split = force_split;
> >
> > -	ret = __change_page_attr_set_clr(&cpa, 1);
> > +	/* Avoid race with concurrent CPA collapse. */
> > +	scoped_guard(mmap_read_lock, &init_mm)
> > +		ret = __change_page_attr_set_clr(&cpa, 1);
>
> There's a small issue with this. debug_pagealloc_unmap_pages() ends up here
> and since it's called from __free_pages() it can be in an atomic context.

Not the case - the callstack is:

debug_pagealloc_unmap_pages()
-> __kernel_map_pages()
-> __set_pages_[n]p()
-> __change_page_attr_set_clr()

So it calls __change_page_attr_set_clr() not change_page_addr_set_clr()
and doesn't hit this code.

As per the commit msg, it (and all the other bare
__change_page_attr_set_clr() callers) do not hit ranges that can be
collapsed so the race doesn't exist there.

Also the CPA_NO_CHECK_ALIAS flag is set, meaning there's no weird aliasing
recursion via cpa_process_alias() (which in any case, also calls
__change_page_attr_set_clr() directly anyway).

>
> >
> >  	/*
> >  	 * Check whether we really changed something:
> >
> > --
> > 2.55.0
> >
>
> --
> Sincerely yours,
> Mike.

Thanks, Lorenzo