Re: [tip: x86/urgent] x86/mm/pat: Acquire init_mm read lock on attribute change to avoid UAF

[Lorenzo Stoakes (ARM)]

Dave - Again this patch has been modified vs. what was submitted and the commit
message, tags, etc. are now completely incorrect.

I'm not happy with this going to Linus as-is, is there a way for you to correct
these please?

On Tue, Sep 01, 2026 at 08:05:07AM +0200, Jiri Slaby wrote:
> On 01. 09. 26, 0:27, tip-bot2 for Lorenzo Stoakes (ARM) wrote:
> > The following commit has been merged into the x86/urgent branch of tip:
> >
> > Commit-ID:     477cf5cd1f698053df7426b7b8d9339e85e00946
> > Gitweb:        https://git.kernel.org/tip/477cf5cd1f698053df7426b7b8d9339e85e00946
> > Author:        Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>
> > AuthorDate:    Thu, 13 Aug 2026 12:01:25 +03:00
> > Committer:     Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> > CommitterDate: Mon, 31 Aug 2026 15:15:42 -07:00
> >
> > x86/mm/pat: Acquire init_mm read lock on attribute change to avoid UAF
> >
> > A previous commit protected 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, 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().
> >
> > However, the lock must not be held over an allocation, as allocation can
> > trigger reclaim and shrinkers may call into CPA recursively, making
> > deadlocks possible (init_mm -> ... -> fs_reclaim -> init_mm).
> >
> > A page table is allocated when a huge page needs to be split:
> >
> > -> change_page_attr_set_clr()
> > -> __change_page_attr_set_clr()
> > -> __change_page_attr()
> > -> split_large_page()
> > [ pagetable_alloc() ]
> > -> __split_large_page()
> >
> > Avoid deadlocks by dropping the mmap lock across pagetable_alloc() in
> > split_large_page() and track whether this is needed by adding a new
> > 'init_mm_read_locked' flag to struct cpa_data.
> >
> > This is safe as __split_large_page() (called with locks re-established)
> > revalidates that the page table entry is the same as it was prior to the
> > locks being dropped and __change_page_attr() repeats the entire page table
> > walk whenever a split occurs, so concurrent split and collapse are
> > accounted for.
> >
> > Concurrent ptdump is also safe as the lock is only dropped over page table
> > allocation during which time the page table has not yet been modified.
> >
> > 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.
> >
> > Fixes: 41d88484c71c ("x86/mm/pat: restore large ROX pages after fragmentation")
> > Signed-off-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>
> > Signed-off-by: Mike Rapoport (Microsoft) <rppt@xxxxxxxxxx>
> > Signed-off-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> > Tested-by: Atish Patra <atishp@xxxxxxxx>
> > Tested-by: Nikunj A Dadhania <nikunj@xxxxxxx>
> > Link: https://lore.kernel.org/all/20260626163213.2284080-1-den@xxxxxxxxxx/
> > Cc:stable@xxxxxxxxxxxxxxx
> > Link: https://patch.msgid.link/20260813-cpa-fixes-v2-2-39b4ff90f91d@xxxxxxxxxx
> > ---
> >   arch/x86/mm/pat/set_memory.c | 28 +++++++++++++++++++++++++---
> >   1 file changed, 25 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> > index c38faf3..cb5d6d6 100644
> > --- a/arch/x86/mm/pat/set_memory.c
> > +++ b/arch/x86/mm/pat/set_memory.c
> > @@ -22,6 +22,7 @@
> >   #include <linux/cc_platform.h>
> >   #include <linux/set_memory.h>
> >   #include <linux/memregion.h>
> > +#include <linux/cleanup.h>
> >   #include <asm/e820/api.h>
> >   #include <asm/processor.h>
> > @@ -49,7 +50,8 @@ struct cpa_data {
> >   	unsigned int	flags;
> >   	unsigned int	force_split		: 1,
> >   			force_static_prot	: 1,
> > -			force_flush_all		: 1;
> > +			force_flush_all		: 1,
> > +			init_mm_read_locked	: 1;
> >   	struct page	**pages;
> >   };
> > @@ -409,7 +411,7 @@ static void __cpa_flush_tlb(void *data)
> >   static int collapse_large_pages(unsigned long addr, struct list_head *pgtables);
> > -static void cpa_collapse_large_pages(struct cpa_data *cpa)
> > +static void __cpa_collapse_large_pages(struct cpa_data *cpa)
> >   {
> >   	unsigned long start, addr, end;
> >   	struct ptdesc *ptdesc, *tmp;
> > @@ -443,6 +445,18 @@ static void cpa_collapse_large_pages(struct cpa_data *cpa)
> >   	}
> >   }
> > +static void cpa_collapse_large_pages(struct cpa_data *cpa)
> > +{
> > +	/*
> > +	 * Take the mmap write lock on init_mm to:
> > +	 * - Avoid a use-after-free if raced by ptdump (which takes its own
> > +	 *   write lock on init_mm).
> > +	 * - Serialise concurrent CPA walkers.
> > +	 */
> > +	scoped_guard(mmap_write_lock, &init_mm)
> > +		__cpa_collapse_large_pages(cpa);
> > +}
> > +
>
> Ah, this belongs to the "Acquire init_mm *write* lock" counterpart. Not sure
> how you managed to split/combine these two 8-).

Yeah this is completely incorrect, now the commit message, tags, etc. are all
wrong.

>
> thanks,
> --
> js
> suse labs
>

--
Cheers, Lorenzo