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

[Lorenzo Stoakes <ljs@kernel.org>]

+cc Brendan for fun's sake ;)

On Fri, Jul 10, 2026 at 12:50:26PM -0700, Dave Hansen wrote:
> On 7/10/26 11:53, Lorenzo Stoakes wrote:
> > On Fri, Jul 10, 2026 at 09:26:48AM -0700, Dave Hansen wrote:
> >> 1. We could just bite the bullet and have separate ptdump files for the
> >>    top and bottom of the address space:
> >> 	current_kernel_top
> >> 	current_kernel_bottom
> >> 	current_user_top
> >> 	current_user_bottom
> >> 	etc..
> >>    Then the lock you take is dictated by the file.
> >
> > I mean that'd break userspace though wouldn't it?
>
> It's debugfs. So, yeah, I can see it breaking things, but it's also way
> less of a concern. Nobody ever complained about the new PTI file getting
> added in there.

I wonder how many people use this :P but I expect some do and would moan about a
_removal_.

I guess we could have a warning if somebody used it.

>
> >> 2. We could always take both init_mm and current->mm locks. That seems
> >>    icky.
> >
> > It's actually the least awful of all of these I think :) and the one I
> > implemented ([0]).
>
> Oh, cool, I missed that. That's a good pairing with this one!

Yeah, sent it separately as they stand separately and this already addresses the
init_mm side of it anyway.

>
> >> 3. We could have ptdump_walk_pgd() take a different lock for each
> >>    'range'. Logically:
> >>
> >> 	if (range->start < PAGE_OFFSET)
> >> 		mmap_write_lock(mm);
> >> 	else
> >> 		mmap_write_lock(&init_mm);
> >
> > I don't love this. It feels a hack for x86 that's put in the wrong place,
> > i.e. core code.
> >
> > And can you can make this assumption for efi_mm for all arches? Could other
> > arches might be weird about this?
>
> Yeah, it's possible they're weird. But I thought the whole idea of
> efi_mm was to reuse the non-kernel part of the address space. So oddly
> enough it kinda makes sense.
>
> But, yeah, I totally get the reluctance to do this.

Yeah it's all a bit sucky this :(

>
> >> I'm kinda leaning toward #3.
> >
> > Another way forwards might be simply have the caller _call
> > ptdump_walk_pgd() twice_ once with the range set to [0, PAGE_OFFSET) passing whatever mm
> > != init_mm, and again for [PAGE_OFFSET, ~0) passing init_mm?
>
> Ahh, yeah, that's a good point. It could be done a layer up too.
>
> > Are there cases where you expect to see a delta in the kernel range in x86
> > for an arbitrary mm?
>
> Are you asking if current->mm->pgd[255->511] is always the same as
> init_mm->pgd[255->511]?

Yes :)

>
> I think so, except for the LDT PGD when PTI is on. That can be different
> between mms, and it's a single pgd_t entry. I think Brendan had some
> grand plans to use this PGD for other things for ASI as well.

Yeah I wondered if PTI or something else funky (Brendan's mermap?) might have
changed this.

But whether that'd actually be accessible from the debugfs "this mm's "?

>
> So, yeah, the upper half of the address space is *normally* identical.
> But PTI plus set_ldt() is abnormal and we have to deal with it. The only
> times that code frees page tables is at exit time and in an error path.
>
> So, how does this interact with mmap_lock? Surely, someone looked at
> this recently because the comment says:
>
>  * Lock order:
>  *      context.ldt_usr_sem
>  *        mmap_lock
>  *          context.lock
>
> and not mmap_sem. But, alas, I don't see any mmap_lock anywhere. Someone
> changed the comment and didn't look at the code.

Was a simple mmap_sem -> mmap_lock change since we renamed the lock right?

>
> Is there some mmap_lock interaction that I'm missing? I don't see it
> _anywhere_ in the ldt code.

Presumably dup_mmap() -> arch_dup_mmap() -> ldt_dup_context()?

But I don't think that interacts with the init_mm nested lock stuff we're
looking at here, as it's not touching init_mm and in any case ptdump is
unconditionally taking the mm's mmap write lock as-is without issue.

Cheers, Lorenzo