Linux-mm Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH v3 1/2] ARM: mm: fix use-after-free in __do_user_fault() under CONFIG_DEBUG_USER
       [not found] <20260626073048.3595106-2-xiqi2@huawei.com>
@ 2026-07-06 13:32 ` Xie Yuanbin
  2026-07-07 11:48   ` Qi Xi
  2026-07-07 12:46   ` Lorenzo Stoakes
  0 siblings, 2 replies; 12+ messages in thread
From: Xie Yuanbin @ 2026-07-06 13:32 UTC (permalink / raw)
  To: xiqi2, akpm, linux, david, ljs, liam, vbabka, rppt, surenb,
	mhocko, linusw
  Cc: linux-arm-kernel, linux-kernel, sunnanyong, xieyuanbin1, linux-mm,
	lilinjie8, liaohua4

On Fri, 26 Jun 2026 15:30:47 +0800, Qi Xi wrote:
> @@ -181,7 +181,9 @@ __do_user_fault(unsigned long addr, unsigned int fsr, unsigned int sig,
>  		pr_err("8<--- cut here ---\n");
>  		pr_err("%s: unhandled page fault (%d) at 0x%08lx, code 0x%03x\n",
>  		       tsk->comm, sig, addr, fsr);
> +		mmap_read_lock(tsk->mm);
>  		show_pte(KERN_ERR, tsk->mm, addr);
> +		mmap_read_unlock(tsk->mm);
>  		show_regs(regs);
>  	}
>  #endif

I found that this fix does not completely solve the problem. For a user
fault, the addr could also be a kernel address. For arm32/x86, the kernel
address space and user address space share the same pgd page table,
but the kernel address space's page table is not protected by
current->mm->mmap_lock.

I have written a use case to construct and verify this point. When A user
program accesses a kernel address and triggers __do_user_fault(),
show_pte() will directly print the kernel page table.

So, I suggest that:
```c
	if (user_mode(regs)) {
		struct mm_struct *const pt_mm = addr >= TASK_SIZE ?
			&init_mm : current->mm;

		mmap_read_lock(pt_mm);
		show_pte(KERN_ALERT, pt_mm, addr);
		mmap_read_unlock(pt_mm);
	} else {
		// .. keep nothing change
		show_pte(KERN_ALERT, current->mm, addr);
	}
```

I have read this article:
Link: https://docs.kernel.org/mm/process_addrs.html
`mmap_read_lock(&init_mm)` should be able to ensure that the kernel
address's page tables can be traversed. But I'm not quite sure if
`mmap_read_lock(&current->mm)` provides protection for user-space non-VMA
addresses?

Also cc to mm maintainers:
Cc: David Hildenbrand <david@kernel.org>
Cc: Lorenzo Stoakes <ljs@kernel.org>
Cc: Liam R. Howlett <liam@infradead.org>
Cc: Vlastimil Babka <vbabka@kernel.org>
Cc: Mike Rapoport <rppt@kernel.org>
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Linus Walleij <linusw@kernel.org>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/2] ARM: mm: fix use-after-free in __do_user_fault() under CONFIG_DEBUG_USER
  2026-07-06 13:32 ` [PATCH v3 1/2] ARM: mm: fix use-after-free in __do_user_fault() under CONFIG_DEBUG_USER Xie Yuanbin
@ 2026-07-07 11:48   ` Qi Xi
  2026-07-07 11:57     ` Russell King
  2026-07-07 12:46   ` Lorenzo Stoakes
  1 sibling, 1 reply; 12+ messages in thread
From: Qi Xi @ 2026-07-07 11:48 UTC (permalink / raw)
  To: Xie Yuanbin, akpm, linux, david, ljs, liam, vbabka, rppt, surenb,
	mhocko, linusw
  Cc: linux-arm-kernel, linux-kernel, sunnanyong, linux-mm, lilinjie8,
	liaohua4



On 06/07/2026 21:32, Xie Yuanbin wrote:
> On Fri, 26 Jun 2026 15:30:47 +0800, Qi Xi wrote:
>> @@ -181,7 +181,9 @@ __do_user_fault(unsigned long addr, unsigned int fsr, unsigned int sig,
>>   		pr_err("8<--- cut here ---\n");
>>   		pr_err("%s: unhandled page fault (%d) at 0x%08lx, code 0x%03x\n",
>>   		       tsk->comm, sig, addr, fsr);
>> +		mmap_read_lock(tsk->mm);
>>   		show_pte(KERN_ERR, tsk->mm, addr);
>> +		mmap_read_unlock(tsk->mm);
>>   		show_regs(regs);
>>   	}
>>   #endif
> I found that this fix does not completely solve the problem. For a user
> fault, the addr could also be a kernel address. For arm32/x86, the kernel
> address space and user address space share the same pgd page table,
> but the kernel address space's page table is not protected by
> current->mm->mmap_lock.
>
> I have written a use case to construct and verify this point. When A user
> program accesses a kernel address and triggers __do_user_fault(),
> show_pte() will directly print the kernel page table.
>
> So, I suggest that:
> ```c
> 	if (user_mode(regs)) {
> 		struct mm_struct *const pt_mm = addr >= TASK_SIZE ?
> 			&init_mm : current->mm;
>
> 		mmap_read_lock(pt_mm);
> 		show_pte(KERN_ALERT, pt_mm, addr);
> 		mmap_read_unlock(pt_mm);
> 	} else {
> 		// .. keep nothing change
> 		show_pte(KERN_ALERT, current->mm, addr);
> 	}
> ```
>
> I have read this article:
> Link: https://docs.kernel.org/mm/process_addrs.html
> `mmap_read_lock(&init_mm)` should be able to ensure that the kernel
> address's page tables can be traversed. But I'm not quite sure if
> `mmap_read_lock(&current->mm)` provides protection for user-space non-VMA
> addresses?
You're right. And I think the fix is to simply skip show_pte() for 
kernel addresses.
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);
	}

For a user-mode fault on a kernel address, printing kernel page tables via
show_pte() does not help. And The fault address is already printed above.
So it's safe and reasonable to skip it.

Same pattern applies to __do_user_fault().

> Also cc to mm maintainers:
> Cc: David Hildenbrand <david@kernel.org>
> Cc: Lorenzo Stoakes <ljs@kernel.org>
> Cc: Liam R. Howlett <liam@infradead.org>
> Cc: Vlastimil Babka <vbabka@kernel.org>
> Cc: Mike Rapoport <rppt@kernel.org>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Linus Walleij <linusw@kernel.org>



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/2] ARM: mm: fix use-after-free in __do_user_fault() under CONFIG_DEBUG_USER
  2026-07-07 11:48   ` Qi Xi
@ 2026-07-07 11:57     ` Russell King
  2026-07-07 12:47       ` Qi Xi
                         ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Russell King @ 2026-07-07 11:57 UTC (permalink / raw)
  To: Qi Xi
  Cc: Xie Yuanbin, akpm, david, ljs, liam, vbabka, rppt, surenb, mhocko,
	linusw, linux-arm-kernel, linux-kernel, sunnanyong, linux-mm,
	lilinjie8, liaohua4

On Tue, Jul 07, 2026 at 07:48:12PM +0800, Qi Xi wrote:
> 
> 
> On 06/07/2026 21:32, Xie Yuanbin wrote:
> > On Fri, 26 Jun 2026 15:30:47 +0800, Qi Xi wrote:
> > > @@ -181,7 +181,9 @@ __do_user_fault(unsigned long addr, unsigned int fsr, unsigned int sig,
> > >   		pr_err("8<--- cut here ---\n");
> > >   		pr_err("%s: unhandled page fault (%d) at 0x%08lx, code 0x%03x\n",
> > >   		       tsk->comm, sig, addr, fsr);
> > > +		mmap_read_lock(tsk->mm);
> > >   		show_pte(KERN_ERR, tsk->mm, addr);
> > > +		mmap_read_unlock(tsk->mm);
> > >   		show_regs(regs);
> > >   	}
> > >   #endif
> > I found that this fix does not completely solve the problem. For a user
> > fault, the addr could also be a kernel address. For arm32/x86, the kernel
> > address space and user address space share the same pgd page table,
> > but the kernel address space's page table is not protected by
> > current->mm->mmap_lock.
> > 
> > I have written a use case to construct and verify this point. When A user
> > program accesses a kernel address and triggers __do_user_fault(),
> > show_pte() will directly print the kernel page table.
> > 
> > So, I suggest that:
> > ```c
> > 	if (user_mode(regs)) {
> > 		struct mm_struct *const pt_mm = addr >= TASK_SIZE ?
> > 			&init_mm : current->mm;
> > 
> > 		mmap_read_lock(pt_mm);
> > 		show_pte(KERN_ALERT, pt_mm, addr);
> > 		mmap_read_unlock(pt_mm);
> > 	} else {
> > 		// .. keep nothing change
> > 		show_pte(KERN_ALERT, current->mm, addr);
> > 	}
> > ```
> > 
> > I have read this article:
> > Link: https://docs.kernel.org/mm/process_addrs.html
> > `mmap_read_lock(&init_mm)` should be able to ensure that the kernel
> > address's page tables can be traversed. But I'm not quite sure if
> > `mmap_read_lock(&current->mm)` provides protection for user-space non-VMA
> > addresses?
> You're right. And I think the fix is to simply skip show_pte() for kernel
> addresses.

No. This information is useful debug for kernel oops.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/2] ARM: mm: fix use-after-free in __do_user_fault() under CONFIG_DEBUG_USER
  2026-07-06 13:32 ` [PATCH v3 1/2] ARM: mm: fix use-after-free in __do_user_fault() under CONFIG_DEBUG_USER Xie Yuanbin
  2026-07-07 11:48   ` Qi Xi
@ 2026-07-07 12:46   ` Lorenzo Stoakes
  2026-07-07 13:35     ` Xie Yuanbin
  1 sibling, 1 reply; 12+ messages in thread
From: Lorenzo Stoakes @ 2026-07-07 12:46 UTC (permalink / raw)
  To: Xie Yuanbin
  Cc: xiqi2, akpm, linux, david, liam, vbabka, rppt, surenb, mhocko,
	linusw, linux-arm-kernel, linux-kernel, sunnanyong, linux-mm,
	lilinjie8, liaohua4

On Mon, Jul 06, 2026 at 09:32:47PM +0800, Xie Yuanbin wrote:
> On Fri, 26 Jun 2026 15:30:47 +0800, Qi Xi wrote:
> > @@ -181,7 +181,9 @@ __do_user_fault(unsigned long addr, unsigned int fsr, unsigned int sig,
> >  		pr_err("8<--- cut here ---\n");
> >  		pr_err("%s: unhandled page fault (%d) at 0x%08lx, code 0x%03x\n",
> >  		       tsk->comm, sig, addr, fsr);
> > +		mmap_read_lock(tsk->mm);
> >  		show_pte(KERN_ERR, tsk->mm, addr);
> > +		mmap_read_unlock(tsk->mm);
> >  		show_regs(regs);
> >  	}
> >  #endif
>
> I found that this fix does not completely solve the problem. For a user
> fault, the addr could also be a kernel address. For arm32/x86, the kernel
> address space and user address space share the same pgd page table,
> but the kernel address space's page table is not protected by
> current->mm->mmap_lock.
>
> I have written a use case to construct and verify this point. When A user
> program accesses a kernel address and triggers __do_user_fault(),
> show_pte() will directly print the kernel page table.
>
> So, I suggest that:
> ```c
> 	if (user_mode(regs)) {
> 		struct mm_struct *const pt_mm = addr >= TASK_SIZE ?
> 			&init_mm : current->mm;
>
> 		mmap_read_lock(pt_mm);
> 		show_pte(KERN_ALERT, pt_mm, addr);
> 		mmap_read_unlock(pt_mm);
> 	} else {
> 		// .. keep nothing change
> 		show_pte(KERN_ALERT, current->mm, addr);
> 	}
> ```
>
> I have read this article:
> Link: https://docs.kernel.org/mm/process_addrs.html
> `mmap_read_lock(&init_mm)` should be able to ensure that the kernel
> address's page tables can be traversed. But I'm not quite sure if

I added a section specifically about this -

https://docs.kernel.org/mm/process_addrs.html#traversing-non-vma-page-tables

But note:

"Since, aside from vmalloc and memory hot plug, kernel page tables are not torn
down all that often - this usually suffices, however any caller of this
functionality must ensure that any additionally required locks are acquired in
advance."

With the latter part being particularly important - you really need to be sure
you aren't going to be raced on page table teardown by anything.

However:

	* You're safe from vmalloc trying to install a huge page table (only way
	  it removes intermediate page tables) since !HAVE_ARCH_HUGE_VMAP.

	* And since arm32 !ARCH_ENABLE_MEMORY_HOTPLUG you're safe from that too
          :)

(Really I think you should be using walk_page_range_debug() here ultimately but
that's a future refactor).

BUT see below:

> `mmap_read_lock(&current->mm)` provides protection for user-space non-VMA
> addresses?

OK so this _does_ need addressing, and I covered it in the document:

	We also permit a truly unusual case is the traversal of non-VMA ranges
	in userland ranges, as provided for by walk_page_range_debug().

	We must take great care in this case, as the munmap() implementation
	detaches VMAs under an mmap write lock before tearing down page tables
	under a downgraded mmap read lock.

	This means such an operation could race with this, and thus an mmap
	write lock is required.

I.e. you need a write lock.

So in conclusion the patch should be:

diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index e62cc4be5a..1f2a85e1fa 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -181,7 +181,9 @@ __do_user_fault(unsigned long addr, unsigned int fsr, unsigned int sig,
 		pr_err("8<--- cut here ---\n");
 		pr_err("%s: unhandled page fault (%d) at 0x%08lx, code 0x%03x\n",
 		       tsk->comm, sig, addr, fsr);
+		mmap_write_lock(tsk->mm);
 		show_pte(KERN_ERR, tsk->mm, addr);
+		mmap_write_unlock(tsk->mm);
 		show_regs(regs);
 	}
 #endif

>
> Also cc to mm maintainers:
> Cc: David Hildenbrand <david@kernel.org>
> Cc: Lorenzo Stoakes <ljs@kernel.org>
> Cc: Liam R. Howlett <liam@infradead.org>
> Cc: Vlastimil Babka <vbabka@kernel.org>
> Cc: Mike Rapoport <rppt@kernel.org>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Linus Walleij <linusw@kernel.org>

Thanks :)

Cheers, Lorenzo


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/2] ARM: mm: fix use-after-free in __do_user_fault() under CONFIG_DEBUG_USER
  2026-07-07 11:57     ` Russell King
@ 2026-07-07 12:47       ` Qi Xi
  2026-07-07 12:47       ` Lorenzo Stoakes
  2026-07-07 13:14       ` Xie Yuanbin
  2 siblings, 0 replies; 12+ messages in thread
From: Qi Xi @ 2026-07-07 12:47 UTC (permalink / raw)
  To: Russell King
  Cc: Xie Yuanbin, akpm, david, ljs, liam, vbabka, rppt, surenb, mhocko,
	linusw, linux-arm-kernel, linux-kernel, sunnanyong, linux-mm,
	lilinjie8, liaohua4


On 07/07/2026 19:57, Russell King wrote:
> On Tue, Jul 07, 2026 at 07:48:12PM +0800, Qi Xi wrote:
>>
>> On 06/07/2026 21:32, Xie Yuanbin wrote:
>>> On Fri, 26 Jun 2026 15:30:47 +0800, Qi Xi wrote:
>>>> @@ -181,7 +181,9 @@ __do_user_fault(unsigned long addr, unsigned int fsr, unsigned int sig,
>>>>    		pr_err("8<--- cut here ---\n");
>>>>    		pr_err("%s: unhandled page fault (%d) at 0x%08lx, code 0x%03x\n",
>>>>    		       tsk->comm, sig, addr, fsr);
>>>> +		mmap_read_lock(tsk->mm);
>>>>    		show_pte(KERN_ERR, tsk->mm, addr);
>>>> +		mmap_read_unlock(tsk->mm);
>>>>    		show_regs(regs);
>>>>    	}
>>>>    #endif
>>> I found that this fix does not completely solve the problem. For a user
>>> fault, the addr could also be a kernel address. For arm32/x86, the kernel
>>> address space and user address space share the same pgd page table,
>>> but the kernel address space's page table is not protected by
>>> current->mm->mmap_lock.
>>>
>>> I have written a use case to construct and verify this point. When A user
>>> program accesses a kernel address and triggers __do_user_fault(),
>>> show_pte() will directly print the kernel page table.
>>>
>>> So, I suggest that:
>>> ```c
>>> 	if (user_mode(regs)) {
>>> 		struct mm_struct *const pt_mm = addr >= TASK_SIZE ?
>>> 			&init_mm : current->mm;
>>>
>>> 		mmap_read_lock(pt_mm);
>>> 		show_pte(KERN_ALERT, pt_mm, addr);
>>> 		mmap_read_unlock(pt_mm);
>>> 	} else {
>>> 		// .. keep nothing change
>>> 		show_pte(KERN_ALERT, current->mm, addr);
>>> 	}
>>> ```
>>>
>>> I have read this article:
>>> Link: https://docs.kernel.org/mm/process_addrs.html
>>> `mmap_read_lock(&init_mm)` should be able to ensure that the kernel
>>> address's page tables can be traversed. But I'm not quite sure if
>>> `mmap_read_lock(&current->mm)` provides protection for user-space non-VMA
>>> addresses?
>> You're right. And I think the fix is to simply skip show_pte() for kernel
>> addresses.
> No. This information is useful debug for kernel oops.
>
For addr >= TASK_SIZE, concurrent munmap cannot free the kernel page tables.
So there is no uaf risk, and show_pte() is still called without the lock 
as before.

Does the following change look acceptable?
if (user_mode(regs) && addr < TASK_SIZE) {
mmap_read_lock(current->mm);
show_pte(KERN_ALERT, current->mm, addr);
mmap_read_unlock(current->mm);
} else {
// .. keep nothing change
show_pte(KERN_ALERT, current->mm, addr);
}


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/2] ARM: mm: fix use-after-free in __do_user_fault() under CONFIG_DEBUG_USER
  2026-07-07 11:57     ` Russell King
  2026-07-07 12:47       ` Qi Xi
@ 2026-07-07 12:47       ` Lorenzo Stoakes
  2026-07-07 13:14       ` Xie Yuanbin
  2 siblings, 0 replies; 12+ messages in thread
From: Lorenzo Stoakes @ 2026-07-07 12:47 UTC (permalink / raw)
  To: Russell King
  Cc: Qi Xi, Xie Yuanbin, akpm, david, liam, vbabka, rppt, surenb,
	mhocko, linusw, linux-arm-kernel, linux-kernel, sunnanyong,
	linux-mm, lilinjie8, liaohua4

On Tue, Jul 07, 2026 at 12:57:45PM +0100, Russell King wrote:
> On Tue, Jul 07, 2026 at 07:48:12PM +0800, Qi Xi wrote:
> > You're right. And I think the fix is to simply skip show_pte() for kernel
> > addresses.
>
> No. This information is useful debug for kernel oops.

Yup see my reply, just upgrade it to an mmap write lock and you should be
safe to keep this.

>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Cheers, Lorenzo


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/2] ARM: mm: fix use-after-free in __do_user_fault() under CONFIG_DEBUG_USER
  2026-07-07 11:57     ` Russell King
  2026-07-07 12:47       ` Qi Xi
  2026-07-07 12:47       ` Lorenzo Stoakes
@ 2026-07-07 13:14       ` Xie Yuanbin
  2026-07-07 13:20         ` Lorenzo Stoakes
  2 siblings, 1 reply; 12+ messages in thread
From: Xie Yuanbin @ 2026-07-07 13:14 UTC (permalink / raw)
  To: linux
  Cc: akpm, david, liam, liaohua4, lilinjie8, linusw, linux-arm-kernel,
	linux-kernel, linux-mm, ljs, mhocko, rppt, sunnanyong, surenb,
	vbabka, xieyuanbin1, xiqi2

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?


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/2] ARM: mm: fix use-after-free in __do_user_fault() under CONFIG_DEBUG_USER
  2026-07-07 13:14       ` Xie Yuanbin
@ 2026-07-07 13:20         ` Lorenzo Stoakes
  2026-07-07 14:04           ` Xie Yuanbin
  2026-07-07 15:34           ` Russell King
  0 siblings, 2 replies; 12+ messages in thread
From: Lorenzo Stoakes @ 2026-07-07 13:20 UTC (permalink / raw)
  To: Xie Yuanbin
  Cc: linux, akpm, david, liam, liaohua4, lilinjie8, linusw,
	linux-arm-kernel, linux-kernel, linux-mm, mhocko, rppt,
	sunnanyong, surenb, vbabka, xiqi2

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?

Thanks, Lorenzo


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/2] ARM: mm: fix use-after-free in __do_user_fault() under CONFIG_DEBUG_USER
  2026-07-07 12:46   ` Lorenzo Stoakes
@ 2026-07-07 13:35     ` Xie Yuanbin
  0 siblings, 0 replies; 12+ messages in thread
From: Xie Yuanbin @ 2026-07-07 13:35 UTC (permalink / raw)
  To: ljs
  Cc: akpm, david, liam, liaohua4, lilinjie8, linusw, linux-arm-kernel,
	linux-kernel, linux-mm, linux, mhocko, rppt, sunnanyong, surenb,
	vbabka, xieyuanbin1, xiqi2

On Tue, 7 Jul 2026 13:46:19 +0100, Lorenzo Stoakes wrote:
> On Mon, Jul 06, 2026 at 09:32:47PM +0800, Xie Yuanbin wrote:
>> I have read this article:
>> Link: https://docs.kernel.org/mm/process_addrs.html
>> `mmap_read_lock(&init_mm)` should be able to ensure that the kernel
>> address's page tables can be traversed. But I'm not quite sure if
>
> I added a section specifically about this -
>
> https://docs.kernel.org/mm/process_addrs.html#traversing-non-vma-page-tables
>
> But note:
>
> "Since, aside from vmalloc and memory hot plug, kernel page tables are not torn
> down all that often - this usually suffices, however any caller of this
> functionality must ensure that any additionally required locks are acquired in
> advance."
>
> With the latter part being particularly important - you really need to be sure
> you aren't going to be raced on page table teardown by anything.
>
> However:
>
> 	* You're safe from vmalloc trying to install a huge page table (only way
>	  it removes intermediate page tables) since !HAVE_ARCH_HUGE_VMAP.
>
>	* And since arm32 !ARCH_ENABLE_MEMORY_HOTPLUG you're safe from that too
>          :)
>
> (Really I think you should be using walk_page_range_debug() here ultimately but
> that's a future refactor).
>
> BUT see below:
>
>> `mmap_read_lock(&current->mm)` provides protection for user-space non-VMA
>> addresses?
>
> OK so this _does_ need addressing, and I covered it in the document:
>
>	We also permit a truly unusual case is the traversal of non-VMA ranges
>	in userland ranges, as provided for by walk_page_range_debug().
>
>	We must take great care in this case, as the munmap() implementation
>	detaches VMAs under an mmap write lock before tearing down page tables
>	under a downgraded mmap read lock.
>
>	This means such an operation could race with this, and thus an mmap
>	write lock is required.
>
> I.e. you need a write lock.

Thank you very much for your reply. Now I fully understand: to traverse
the page tables of non-VMA addr in user address space, the mmap write
lock is required.

But I still want like to ask a question:
> However:
>
> 	* You're safe from vmalloc trying to install a huge page table (only way
> 	  it removes intermediate page tables) since !HAVE_ARCH_HUGE_VMAP.
>
>	* And since arm32 !ARCH_ENABLE_MEMORY_HOTPLUG you're safe from that too
>          :)

If (just hypothetically), the ARM32 architecture supports huge pages
and memory hotplug, what kind of lock do I need to safely traverse the
page tables of non-VMA addr in kernel space?

Thanks again.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/2] ARM: mm: fix use-after-free in __do_user_fault() under CONFIG_DEBUG_USER
  2026-07-07 13:20         ` Lorenzo Stoakes
@ 2026-07-07 14:04           ` Xie Yuanbin
  2026-07-07 15:34           ` Russell King
  1 sibling, 0 replies; 12+ messages in thread
From: Xie Yuanbin @ 2026-07-07 14:04 UTC (permalink / raw)
  To: ljs, akpm
  Cc: david, liam, liaohua4, lilinjie8, linusw, linux-arm-kernel,
	linux-kernel, linux-mm, linux, mhocko, rppt, sunnanyong, surenb,
	vbabka, xieyuanbin1, xiqi2

On Tue, 7 Jul 2026 14:20:19 +0100, Lorenzo Stoakes wrote:
> 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?

I think it is not the same thing as the previous discussion regarding
which locks are needed for `show_pte()`.

Now, for this bug, we have two places that need to be fixed:
1. __do_user_fault()->show_pte();
2. do_DataAbort()->show_pte();

For the first one, it must be a user fault, the judgment of
user_mode(regs) can be omitted.

For the second one, it may be a user fault or a kernel fault. If it is
a kernel fault, it also means that this is a kernel oops.

For kernel oops, according to Russell's opinion, we do not need to fix
this bug, which means we do not need to acquire any locks, because the
kernel may die soon. However, show_pte() still needs to be retained
because it is useful for debugging the kernel.

For user faults, we need to fix it:
1. For user space addr (addr < TASK_SIZE), we need to acquire
   current->mm's write lock, before show_pte(), I understand it now.
2. For kernel space addr (addr >= TASK_SIZE), maybe we can omit
   show_pte()? After all, user-mode programs can access any kernel address
   to trigger user faults, but we dump the page tables of kernel
   addresses, would that be inappropriate,
   (e.g., posing potential security risks)?


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/2] ARM: mm: fix use-after-free in __do_user_fault() under CONFIG_DEBUG_USER
  2026-07-07 13:20         ` Lorenzo Stoakes
  2026-07-07 14:04           ` Xie Yuanbin
@ 2026-07-07 15:34           ` Russell King
  2026-07-10  2:32             ` Qi Xi
  1 sibling, 1 reply; 12+ messages in thread
From: Russell King @ 2026-07-07 15:34 UTC (permalink / raw)
  To: Lorenzo Stoakes
  Cc: Xie Yuanbin, akpm, david, liam, liaohua4, lilinjie8, linusw,
	linux-arm-kernel, linux-kernel, linux-mm, mhocko, rppt,
	sunnanyong, surenb, vbabka, xiqi2

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/2] ARM: mm: fix use-after-free in __do_user_fault() under CONFIG_DEBUG_USER
  2026-07-07 15:34           ` Russell King
@ 2026-07-10  2:32             ` Qi Xi
  0 siblings, 0 replies; 12+ messages in thread
From: Qi Xi @ 2026-07-10  2:32 UTC (permalink / raw)
  To: Russell King, Lorenzo Stoakes
  Cc: Xie Yuanbin, akpm, david, liam, liaohua4, lilinjie8, linusw,
	linux-arm-kernel, linux-kernel, linux-mm, mhocko, rppt,
	sunnanyong, surenb, vbabka, xiqi2, Kefeng Wang


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.

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.



^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2026-07-10  2:32 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20260626073048.3595106-2-xiqi2@huawei.com>
2026-07-06 13:32 ` [PATCH v3 1/2] ARM: mm: fix use-after-free in __do_user_fault() under CONFIG_DEBUG_USER Xie Yuanbin
2026-07-07 11:48   ` Qi Xi
2026-07-07 11:57     ` Russell King
2026-07-07 12:47       ` Qi Xi
2026-07-07 12:47       ` Lorenzo Stoakes
2026-07-07 13:14       ` Xie Yuanbin
2026-07-07 13:20         ` Lorenzo Stoakes
2026-07-07 14:04           ` Xie Yuanbin
2026-07-07 15:34           ` Russell King
2026-07-10  2:32             ` Qi Xi
2026-07-07 12:46   ` Lorenzo Stoakes
2026-07-07 13:35     ` Xie Yuanbin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox