linux-kernel - Re: [PATCH] mm/userfaultfd: fix missing PTE unmap for non-migration entries

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aItjffoR7molh3QF@lappy>
Date: Thu, 31 Jul 2025 08:37:17 -0400
From: Sasha Levin <sashal@...nel.org>
To: David Hildenbrand <david@...hat.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, peterx@...hat.com,
	aarcange@...hat.com, surenb@...gle.com, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] mm/userfaultfd: fix missing PTE unmap for non-migration
 entries

On Tue, Jul 08, 2025 at 05:42:16PM +0200, David Hildenbrand wrote:
>On 08.07.25 17:33, Sasha Levin wrote:
>>On Tue, Jul 08, 2025 at 05:10:44PM +0200, David Hildenbrand wrote:
>>>On 01.07.25 02:57, Andrew Morton wrote:
>>>>On Sun, 29 Jun 2025 23:19:58 -0400 Sasha Levin <sashal@...nel.org> wrote:
>>>>
>>>>>When handling non-swap entries in move_pages_pte(), the error handling
>>>>>for entries that are NOT migration entries fails to unmap the page table
>>>>>entries before jumping to the error handling label.
>>>>>
>>>>>This results in a kmap/kunmap imbalance which on CONFIG_HIGHPTE systems
>>>>>triggers a WARNING in kunmap_local_indexed() because the kmap stack is
>>>>>corrupted.
>>>>>
>>>>>Example call trace on ARM32 (CONFIG_HIGHPTE enabled):
>>>>>   WARNING: CPU: 1 PID: 633 at mm/highmem.c:622 kunmap_local_indexed+0x178/0x17c
>>>>>   Call trace:
>>>>>     kunmap_local_indexed from move_pages+0x964/0x19f4
>>>>>     move_pages from userfaultfd_ioctl+0x129c/0x2144
>>>>>     userfaultfd_ioctl from sys_ioctl+0x558/0xd24
>>>>>
>>>>>The issue was introduced with the UFFDIO_MOVE feature but became more
>>>>>frequent with the addition of guard pages (commit 7c53dfbdb024 ("mm: add
>>>>>PTE_MARKER_GUARD PTE marker")) which made the non-migration entry code
>>>>>path more commonly executed during userfaultfd operations.
>>>>>
>>>>>Fix this by ensuring PTEs are properly unmapped in all non-swap entry
>>>>>paths before jumping to the error handling label, not just for migration
>>>>>entries.
>>>>
>>>>I don't get it.
>>>>
>>>>>--- a/mm/userfaultfd.c
>>>>>+++ b/mm/userfaultfd.c
>>>>>@@ -1384,14 +1384,15 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
>>>>>  		entry = pte_to_swp_entry(orig_src_pte);
>>>>>  		if (non_swap_entry(entry)) {
>>>>>+			pte_unmap(src_pte);
>>>>>+			pte_unmap(dst_pte);
>>>>>+			src_pte = dst_pte = NULL;
>>>>>  			if (is_migration_entry(entry)) {
>>>>>-				pte_unmap(src_pte);
>>>>>-				pte_unmap(dst_pte);
>>>>>-				src_pte = dst_pte = NULL;
>>>>>  				migration_entry_wait(mm, src_pmd, src_addr);
>>>>>  				err = -EAGAIN;
>>>>>-			} else
>>>>>+			} else {
>>>>>  				err = -EFAULT;
>>>>>+			}
>>>>>  			goto out;
>>>>
>>>>where we have
>>>>
>>>>out:
>>>>	...
>>>>	if (dst_pte)
>>>>		pte_unmap(dst_pte);
>>>>	if (src_pte)
>>>>		pte_unmap(src_pte);
>>>
>>>AI slop?
>>
>>Nah, this one is sadly all me :(
>
>Haha, sorry :P

So as I was getting nowhere with this, I asked AI to help me :)

If you're not interested in reading LLM generated code, feel free to
stop reading now...

After it went over the logs, and a few prompts to point it the right
way, it ended up generating a patch (below) that made sense, and fixed
the warning that LKFT was being able to trigger.

If anyone who's more familiar with the code than me (and the AI) agrees
with the patch and ways to throw their Reviewed-by, I'll send out the
patch.

If the below patch is completely bogus then I'm sorry and I'll buy you a
beer at LPC :)


 From 70f7eae079a5203857b96d6c64bb72b0f566d4de Mon Sep 17 00:00:00 2001
From: Sasha Levin <sashal@...nel.org>
Date: Wed, 30 Jul 2025 20:41:54 -0400
Subject: [PATCH] mm/userfaultfd: fix kmap_local LIFO ordering for
  CONFIG_HIGHPTE

With CONFIG_HIGHPTE on 32-bit ARM, move_pages_pte() maps PTE pages using
kmap_local_page(), which requires unmapping in Last-In-First-Out order.

The current code maps dst_pte first, then src_pte, but unmaps them in
the same order (dst_pte, src_pte), violating the LIFO requirement.
This causes the warning in kunmap_local_indexed():

   WARNING: CPU: 0 PID: 604 at mm/highmem.c:622 kunmap_local_indexed+0x178/0x17c
   addr != __fix_to_virt(FIX_KMAP_BEGIN + idx)

Fix this by reversing the unmap order to respect LIFO ordering.

This issue follows the same pattern as similar fixes:
- commit eca6828403b8 ("crypto: skcipher - fix mismatch between mapping and unmapping order")
- commit 8cf57c6df818 ("nilfs2: eliminate staggered calls to kunmap in nilfs_rename")

Both of which addressed the same fundamental requirement that kmap_local
operations must follow LIFO ordering.

Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
Co-developed-by: Claude claude-opus-4-20250514
Signed-off-by: Sasha Levin <sashal@...nel.org>
---
  mm/userfaultfd.c | 9 +++++++--
  1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index 8253978ee0fb..bf7a57ea71e0 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -1453,10 +1453,15 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
  		folio_unlock(src_folio);
  		folio_put(src_folio);
  	}
-	if (dst_pte)
-		pte_unmap(dst_pte);
+	/*
+	 * Unmap in reverse order (LIFO) to maintain proper kmap_local
+	 * index ordering when CONFIG_HIGHPTE is enabled. We mapped dst_pte
+	 * first, then src_pte, so we must unmap src_pte first, then dst_pte.
+	 */
  	if (src_pte)
  		pte_unmap(src_pte);
+	if (dst_pte)
+		pte_unmap(dst_pte);
  	mmu_notifier_invalidate_range_end(&range);
  	if (si)
  		put_swap_device(si);


-- 
Thanks,
Sasha