Re: [PATCH v3 1/2] ARM: mm: fix use-after-free in __do_user_fault() under CONFIG_DEBUG_USER

[Lorenzo Stoakes]

On Fri, Jul 10, 2026 at 10:32:29AM +0800, Qi Xi wrote:
>
> On 07/07/2026 23:34, Russell King wrote:
> > On Tue, Jul 07, 2026 at 02:20:19PM +0100, Lorenzo Stoakes wrote:
> > > On Tue, Jul 07, 2026 at 09:14:09PM +0800, Xie Yuanbin wrote:
> > > > On Tue, 7 Jul 2026 12:57:45 +0100, Russell King wrote:
> > > > > No. This information is useful debug for kernel oops.
> > > > For kernel oops, I think it should be `!user_mode(regs)`, Qi Xi's reply:
> > > >
> > > > On Tue, 7 Jul 2026 19:48:12 +0800, Qi Xi wrote:
> > > > > For do_DataAbort() fallback:
> > > > >
> > > > > 	if (user_mode(regs)) {
> > > > > 		if (addr < TASK_SIZE) {
> > > > > 			mmap_read_lock(current->mm);
> > > > > 			show_pte(KERN_ALERT, current->mm, addr);
> > > > > 			mmap_read_unlock(current->mm);
> > > > > 		}
> > > > > 	} else {
> > > > > 		show_pte(KERN_ALERT, current->mm, addr);
> > > > > 	}
> > > > changes nothing to kernel oops. It only skip show_pte() for user-mode
> > > > faults, and the fault addr is a kernel address, which means a user
> > > > program is trying to access a kernel address.
> > > > I think it is reasonable to skip show_pte() in this case?
> > > Well the whole reason you're faulting here might be because a userland process
> > > did that right? The page tables should tell you (presumably on ARM32 :)
> > >
> > > And I hate to repeat myself, maybe you didn't read the whole thread but... just
> > > use mmap_write_lock(), this isn't necessary?
> > >
> > > What is this trying to achieve?
> > >
> > > You're not in a hotpath, why are you bothering to conditionally take/not take
> > > the lock?
> > Unconditionally taking the lock could lead to a deadlock. Consider
> > the case where the mmap lock is held, and we get an unrecognised
> > abort from the kernel.
> >
> > If we try to take the mmap lock again, we'll deadlock, which will
> > result in very little debug information being output - and the
> > system locks up. The only thing that would save such a case would
> > be if the user had decided to use a hardware watchdog, or is
> > physically present to press the reset button.

Yeah right, ugh.

We don't currently have an mmap_write_trylock(), that was removed in commit
cf95e337cb63 ("mm: delete mmap_write_trylock() and vma_try_start_write()")
but I think it's legit to bring _only_ the mmap_write_trylock() back (not
vma_try_start_write()), AND updated to account for how VMA locking works
currently (we have to update a seqcount on mmap write lock acquisition).

To make life easy - I've attached a patch that you can add as part of a
series (please keep attribution etc. to me so I can be blamed/villified if
this is insane :)

That way we avoid the deadlock and also avoid concurrent downgraded
munmap() oopsing in show_pte().

>
> We are preparing a v4 and would like to confirm the approach for
> the do_DataAbort() fallback path (__do_user_fault() is similar).
>
> As Lorenzo noted, an mmap write lock is required here because
> munmap() downgrades the write lock to a read lock before tearing
> down page tables.
>
> -	show_pte(KERN_ALERT, current->mm, addr);
> +	if (user_mode(regs)) {
> +		if (addr < TASK_SIZE) {
> +			mmap_write_lock(current->mm);
> +			show_pte(KERN_ALERT, current->mm, addr);
> +			mmap_write_unlock(current->mm);
> +		}
> +	} else {
> +		show_pte(KERN_ALERT, current->mm, addr);
> +	}
>
> The lock is taken only for user_mode(regs) + addr < TASK_SIZE, so
> kernel aborts that may already hold the mmap lock are left unchanged.
> For user-mode faults on kernel addresses (addr >= TASK_SIZE), as
> Yuanbin noted, it is reasonable to skip show_pte().
>
> Please let us know if you see any issues with this approach, or if you
> would suggest a different way to handle it.
>

In general yeah but with mmap_write_trylock() after you add the attached
patch to a series (making sure to cc- the right people etc.)... but also,
wouldn't you just generally want this in show_pte()?

Seems to me best way is to put the existing show_pte() into a __show_pte()
and then do show_pte() like (untested top of my head thing):

void show_pte(const char *lvl, struct mm_struct *mm, unsigned long addr,
	      bool is_user)
{
	/* Write lock needed to account for concurrent downgraded munmap(). */
	if (is_user && !mmap_write_trylock(mm)) {
		printk("%s[%08lx] unable to acquire lock for PTE output\n",
			lvl, addr);
		return;
	}
	__show_pte(lvl, mm, addr);
	if (is_user)
		mmap_write_unlock(mm);
}

Cheers, Lorenzo

----8<----