Re: [PATCH mm-hotfixes v4 2/4] x86/mm/pat: acquire init_mm write lock to avoid UAF

[David CARLIER]

Yes I used AI to agglomerate my otherwise disparate thoughts :)

On Fri, 17 Jul 2026 at 08:47, Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx> wrote:
>
> On Fri, Jul 17, 2026 at 04:46:42AM +0100, David CARLIER wrote:
> > On Thu, 16 Jul 2026 at 22:31, Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx> wrote:
> > >
> > > x86 implements page attribute modification using its Change Page
> > > Attributes (CPA) mechanism.
> > >
> > > This tracks properties of ranges such as cache mode through x86 page
> > > attributes, and as part of that logic manipulates kernel page tables.
> > >
> > > Since commit 41d88484c71c ("x86/mm/pat: restore large ROX pages after
> > > fragmentation") ranges of kernel page table entries can be collapsed into
> > > huge page table entries as part of this logic.
> > >
> > > As part of this collapse, it frees the page tables which the collapsed
> > > entries previously pointed to, and it does so without any relevant locks
> > > being held to preclude concurrent kernel page table walkers.
> > >
> > > The only way this code can be reached is if CPA_COLLAPSE is specified, and
> > > this is only set in set_memory_rox() via:
> > >
> > > set_memory_rox()
> > > -> change_page_attr_set_clr()
> > > -> cpa_flush()
> > > -> cpa_collapse_large_pages()
> > >
> > > Notable users of this are execmem and bpf when manipulating executable
> > > mappings.
> > >
> > > However, this is problematic for ptdump as it walks ranges it does not own
> > > and thus runs the risk of a use-after-free on page tables freed underneath
> > > it.
> > >
> > > In addition, concurrent CPA collapse operations are possible which can also
> > > cause races.
> > >
> > > Resolve the issue by acquiring the mmap write lock on init_mm across the
> > > whole operation.
> > >
> > > It is safe to acquire a sleeping lock as all the callers invoke
> > > set_memory_rox() from process context and in any case,
> > > change_page_attr_set_clr() calls vm_unmap_alias() which ultimately takes a
> > > mutex, disallowing atomic context here.
> > >
> > > Fixes: 41d88484c71c ("x86/mm/pat: restore large ROX pages after fragmentation")
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Reviewed-by: Mike Rapoport (Microsoft) <rppt@xxxxxxxxxx>
> > > Reviewed-by: Kiryl Shutsemau (Meta) <kas@xxxxxxxxxx>
> > > Reviewed-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> > > Reviewed-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>
> > > ---
> > >  arch/x86/mm/pat/set_memory.c | 15 ++++++++++++++-
> > >  include/linux/mmap_lock.h    |  2 ++
> > >  2 files changed, 16 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> > > index d023a40a1e03..d1e63f7d267f 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>
> > > @@ -410,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;
> > > @@ -442,6 +443,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);
> > > +}
> > > +
> > >  static void cpa_flush(struct cpa_data *cpa, int cache)
> > >  {
> > >         unsigned int i;
> > > diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
> > > index 6b5c2390cc30..047f5f5e2c34 100644
> > > --- a/include/linux/mmap_lock.h
> > > +++ b/include/linux/mmap_lock.h
> > > @@ -621,6 +621,8 @@ static inline void mmap_read_unlock(struct mm_struct *mm)
> > >
> > >  DEFINE_GUARD(mmap_read_lock, struct mm_struct *,
> > >              mmap_read_lock(_T), mmap_read_unlock(_T))
> > > +DEFINE_GUARD(mmap_write_lock, struct mm_struct *,
> > > +            mmap_write_lock(_T), mmap_write_unlock(_T))
> > >  DEFINE_GUARD_COND(mmap_read_lock, _try, mmap_read_trylock(_T))
> > >
> > >  static inline void mmap_read_unlock_non_owner(struct mm_struct *mm)
> > >
> > > --
> > > 2.55.0
> > >
> >
> > Hi,
> >
> > This bullet confuses me. __change_page_attr() and friends are serialised by
>
> Which bullet? You're quoting the whole mail?
>
> Presumably "- Serialise concurrent CPA walkers".
>
> > cpa_lock/pgd_lock and never take init_mm's mmap lock, so this doesn't
> > exclude them.
> >
> > Collapse vs collapse looks covered already: collapse_large_pages() holds
>
> cpa_collapse_large_pages() has 3 stages:
>
> - walk + gather
> - flush tlb
> - free page tables
>
> Holding a lock then releasing it across stages doesn't serialise the whole
> operation.
>
> Thus concurrent collapse can occur.
>
> > pgd_lock across the walk and both collapse_pmd_page()/collapse_pud_page()
> > calls, and the pmd_leaf()/pud_leaf() early-outs stop a second CPU
> > re-queueing a collapsed table. I couldn't build the interleaving behind the
> > cover letter's "could result in memory corruption". Can you spell it out?
>
> "Build the interleaving" is an odd turn of phrase :)
>
> >
> > If the reason is really Will's v3 point - collapse rounds up to PMD/PUD and
> > frees tables outside its caller's range, so it has to exclude
> > walk_kernel_page_table_range() walkers holding the read lock - then say
> > that. It's a UAF independent of ptdump, and it's why v3's read lock wasn't
> > enough.
>
> "Then say that" could be rephrased into something a little more polite :)
>

Sorry I did not mean that tone originally

> >
> > Also: patch 1 justifies the trylock fallback on ptdump being rare, but this
> > takes the write lock on every set_memory_rox() (BPF JIT, module load,
> > execmem), even when nothing collapsed. vmap_try_huge_*() then loses the
>
> Locks work by excluding the critical section.
>
> > trylock and quietly falls back to PTEs on every BPF load. No counter, no
>
> Entirely false. You don't have concurrent writers on every load.
>
> > trace. Not a blocker and I've no better idea, but fa93b45fd397 was about
> > getting vmalloc-huge on by default, so it's worth calling out.
>
> I have no idea why you're referencing Dev's arm64 fix?
>
> Anyway as to the 'costly' stuff - you're doing costly operations on this code
> path anyway.
>
> BPF JIT, module load + execmem allocation are not hot path operations nor do
> they seem likely to be problematic in terms of lock contention.
>
> >
> > Rest looks right. 4/4 restores pud_free_pmd_page() exactly as it was before
> > fa93b45fd397, and 3/4 builds the same walk init_mm got via
> > walk_kernel_page_table_range_lockless().
>
> ??? I didn't change any of those in this revision? Let's keep review of a
> specific patch to review of a specific patch, please :)
>
> >
> > Reviewed-by: David Carlier <devnexen@xxxxxxxxx>
>
> Thanks.
>
> >
> > Cheers !
>
> This reply reads like it was... assisted shall we say. Let me gently push back
> on doing that please ;)
>
> I don't think there are actually any other readers that could be problematic
> here, so this a bit of a half-fix, really we need to do the same thing or take a
> read lock on the split path too.
>
> Let me think about it some more.
>
> Thanks, Lorenzo
>
> Assisted-by: Caffeine (Vimto energy with real fruit juice)