Re: [PATCH mm-hotfixes v3 2/4] x86/mm/pat: acquire mmap lock on page table free to avoid ptdump UAF

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Thu, Jul 16, 2026 at 12:31:35PM +0100, Will Deacon wrote:
> On Thu, Jul 16, 2026 at 11:06:51AM +0100, Lorenzo Stoakes (ARM) wrote:
> > On 2026-07-15 18:49 +0300, Mike Rapoport wrote:
> > > On Wed, Jul 15, 2026 at 04:24:59PM +0100, Will Deacon wrote:
> > > > > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> > > > > index d023a40a1e03..4c4b8244502f 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>
> > > > > @@ -436,9 +437,16 @@ static void cpa_collapse_large_pages(struct cpa_data *cpa)
> > > > >
> > > > >  	flush_tlb_all();
> > > > >
> > > > > -	list_for_each_entry_safe(ptdesc, tmp, &pgtables, pt_list) {
> > > > > -		list_del(&ptdesc->pt_list);
> > > > > -		pagetable_free(ptdesc);
> > > > > +	/*
> > > > > +	 * ptdump might read these page tables, so avoid a use-after-free by
> > > > > +	 * acquiring the mmap read lock on init_mm (ptdump acquires the mmap
> > > > > +	 * write lock).
> > > > > +	 */
> > > > > +	scoped_guard(mmap_read_lock, &init_mm) {
> > > > > +		list_for_each_entry_safe(ptdesc, tmp, &pgtables, pt_list) {
> > > > > +			list_del(&ptdesc->pt_list);
> > > > > +			pagetable_free(ptdesc);
> > > > > +		}
> > > >
> > > > As I understand it, the argument for taking the read lock is that we're
> > > > operating on a region that we "wholly own" and therefore we can happily
> > > > run concurrently with CPUs walking distinct parts of the page-table.
> > > > However, from what I can tell, the CPA collapse logic will operate on
> > > > regions outside of the address range being manipulated by its caller
> > > > because it rounds up to the PMD size.
> > > >
> > > > As a made-up example, imagine I have a 2MiB aligned region where the
> > > > first 1MiB is read-only and the second 1MiB is in the default r/w state.
> > > > If one CPU calls set_memory_rw() on the first 1MiB while another CPU is
> > > > walking the second 1MiB (via some other API that doesn't take cpa_lock),
> > > > it looks to me like the first CPU can collapse the page-table and free
> > > > the unused pages under the feet of the other CPU. What prevents that
> > > > from happening?
> > >
> > > Nothing, and there's a patch to fix that that synchronizes
> > > cpa_collapse_large_pages() using cpa_lock:
> > >
> > > https://lore.kernel.org/all/20260626163213.2284080-1-den@openvz.org
>
> Hmm, but that relies on all concurrent walkers taking cpa_lock, no? Surely
> that's not generally the case.
>
> > Thanks Mike!
> >
> > >
> > > This still won't be enough to sync with ptdump though.
> >
> > Yeah, so this patch is still necessary.
> >
> > But that patch conflicts with this one as it holds a spin lock over the page
> > table freeing, which prevents taking an rwsem... :/
> >
> > Anyway I think this one is still fine as it seems there's not a consensus over
> > there as there was discussion about just removing the locking anyway ([0])?
> >
> > So can kick that can down the road and just get these ptdump bugs fixed :)
> >
> > >
> > > > If all concurrent walkers have interrupts disabled, I guess the TLB
> > > > invalidation logic would do it, but it would be good to call this out in
> > > > the commit message because it's not clear to me why the read_lock is
> > > > sufficient for the collapsing case.
> >
> > Yeah, so this patch is _only_ fixing the ptdump case. Any existing bug must be
> > addressed separately.
>
> Up to you folks, but if you took the write lock during collapse wouldn't
> that fix it for all concurrent walkers and we wouldn't need another patch?
>
> I would argue that using the read lock here directly contradicts the
> rationale for why this is safe, because this is an occurence of code
> that operates on a range that it doesn't wholly own. It seems bizarre
> to me to go to the effort of adding locking, but then not adding
> sufficient locking for all concurrent walkers.

Yeah good point, we could avoid the cpa spin lock altogether and guarantee that
we're the only writer by using an mmap write lock instead.

We have to take a write lock in the ptdump reader because of non-VMA ranges for
some architectures + munmap downgrades to a read lock on teardown (having
already removed the VMA ranges hence non-VMA the only ones we are concerned
about).

We take read locks for stabilisation here + in vmap code because that suffices,
but so does a write lock.

I'll post on that thread and do a respin with that then.

>
> Will

Cheers, Lorenzo