mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH] mm/ksm: avoid unsafe anon rmap dereference in KSM merge
@ 2026-11-02 12:32 Lorenzo Stoakes (ARM)
  2026-11-02 12:49 ` David Hildenbrand (Arm)
  0 siblings, 1 reply; 4+ messages in thread
From: Lorenzo Stoakes (ARM) @ 2026-11-02 12:32 UTC (permalink / raw)
  To: David Hildenbrand, Xu Xin, Chengming Zhou, Andrew Morton, Rik van Riel
  Cc: linux-mm, linux-kernel, ljs, Zhao Gongyi

When KSM examines candidate VMAs in scan_get_next_rmap_item(), it
accumulates (mm, address) pairs in ksm_rmap_item objects, assigning them
via get_next_rmap_item(), pinning folios via a page walk in
ksm_next_page_pmd_entry().

Throughout this operation, scan_get_next_rmap_item() holds mmap read lock,
which it drops afterwards.

After the mmap lock is dropped the VMAs might be freed and remapped where
the VMA belonging to the folio was previously mapped.

mremap()'s ability to change where anon folios are mapped is dealt with by
KSM by essentially avoiding the problem - prep_move_vma() unshares KSM
folios, and the scanning logic is tolerant of concurrent remaps, discarding
stale ksm_rmap_item objects and allocating new ones at the new address.

This doesn't account for a new VMA being mapped over an old one where a
folio was previously mapped, so KSM accounts for this by checking things at
the page table level.

However, once an anon_vma's reference count reaches zero, it is freed, but
nothing clears folio->mapping, which is where the KSM pin interacts badly
with the current KSM implementation.

page_address_in_vma() is consistently used to look up the folio's address,
but page_address_in_vma() explicitly states:

"It is the caller's responsibility to check the page is actually within the
VMA."

And KSM violates this, as it cannot know this to be true. This results in
this function performing a use-after-free:

		struct anon_vma *anon_vma = folio_anon_vma(folio);

		...

		if (!vma_has_anon_rmap(vma) || !anon_vma ||
		    vma->anon_vma->root != anon_vma->root)
			return -EFAULT;

As anon_vma->root is dereferenced when anon_vma can already have been
freed.

This is currently benign in practice, as anon_vma's are slab-allocated
objects so this will always be within the direct mapping and arbitrary
memory will be treated as if it were a pointer to be compared.

This either results in an -EFAULT or a spurious match, which will be
rejected at the page table level.

However we mustn't UAF even if it's harmless, and KASAN rightly flags this.

It turns out we don't even need to use page_address_in_vma() as we already
know the folio's address, it simply needs to be passed through.

This change therefore updates KSM to do this, sidestepping the issue
altogether.

Explicitly check for KSM folios in try_to_merge_one_page(), which are
already merged, and by definition never have an anon_vma.

This was previously handled implicitly by page_address_in_vma() as
folio_anon_vma() returns NULL for those.

In many cases cmp_and_merge_page() will reject these, but it's possible for
KSM folios to fall into the unstable tree there, so eliminate that
possibility by checking for that early.

This maintains existing behaviour.

The bug was introduced by commit 21d0d443cdc1 ("rmap: resurrect
page_address_in_vma anon_vma check") which first dereferenced
folio->mapping unsafely, but commit b8c8ba73c68b ("slub: Introduce
CONFIG_SLUB_RCU_DEBUG") made it visible with CONFIG_SLUB_RCU_DEBUG enabled.

Fixes: 21d0d443cdc1 ("rmap: resurrect page_address_in_vma anon_vma check")
Reported-by: Zhao Gongyi <zhaogongyi@bytedance.com>
Closes: https://lore.kernel.org/all/20261031182256.40205-1-zhaogongyi@bytedance.com/
Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
---
David - this seems benign in practice, so I'm not sure whether a backport
is required here.
---
 mm/ksm.c | 43 ++++++++++++++++++-------------------------
 1 file changed, 18 insertions(+), 25 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index d6f736a1aa2f..620455e4754a 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -1294,10 +1294,10 @@ static u32 calc_checksum(struct page *page)
 }
 
 static int write_protect_page(struct vm_area_struct *vma, struct folio *folio,
-			      pte_t *orig_pte)
+			      unsigned long addr, pte_t *orig_pte)
 {
 	struct mm_struct *mm = vma->vm_mm;
-	DEFINE_FOLIO_VMA_WALK(pvmw, folio, vma, 0, 0);
+	DEFINE_FOLIO_VMA_WALK(pvmw, folio, vma, addr, 0);
 	int swapped;
 	int err = -EFAULT;
 	struct mmu_notifier_range range;
@@ -1307,10 +1307,6 @@ static int write_protect_page(struct vm_area_struct *vma, struct folio *folio,
 	if (WARN_ON_ONCE(folio_test_large(folio)))
 		return err;
 
-	pvmw.address = page_address_in_vma(folio, folio_page(folio, 0), vma);
-	if (pvmw.address == -EFAULT)
-		goto out;
-
 	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm, pvmw.address,
 				pvmw.address + PAGE_SIZE);
 	mmu_notifier_invalidate_range_start(&range);
@@ -1380,7 +1376,6 @@ static int write_protect_page(struct vm_area_struct *vma, struct folio *folio,
 	page_vma_mapped_walk_done(&pvmw);
 out_mn:
 	mmu_notifier_invalidate_range_end(&range);
-out:
 	return err;
 }
 
@@ -1389,12 +1384,13 @@ static int write_protect_page(struct vm_area_struct *vma, struct folio *folio,
  * @vma:      vma that holds the pte pointing to page
  * @page:     the page we are replacing by kpage
  * @kpage:    the ksm page we replace page by
+ * @addr:     the address at which @page is mapped
  * @orig_pte: the original value of the pte
  *
  * Returns 0 on success, -EFAULT on failure.
  */
 static int replace_page(struct vm_area_struct *vma, struct page *page,
-			struct page *kpage, pte_t orig_pte)
+		struct page *kpage, unsigned long addr, pte_t orig_pte)
 {
 	struct folio *kfolio = page_folio(kpage);
 	struct mm_struct *mm = vma->vm_mm;
@@ -1404,14 +1400,9 @@ static int replace_page(struct vm_area_struct *vma, struct page *page,
 	pte_t *ptep;
 	pte_t newpte;
 	spinlock_t *ptl;
-	unsigned long addr;
 	int err = -EFAULT;
 	struct mmu_notifier_range range;
 
-	addr = page_address_in_vma(folio, page, vma);
-	if (addr == -EFAULT)
-		goto out;
-
 	pmd = mm_find_pmd(mm, addr);
 	if (!pmd)
 		goto out;
@@ -1499,6 +1490,7 @@ static int replace_page(struct vm_area_struct *vma, struct page *page,
 /*
  * try_to_merge_one_page - take two pages and merge them into one
  * @vma: the vma that holds the pte pointing to page
+ * @addr:     the address at which @page is mapped
  * @page: the PageAnon page that we want to replace with kpage
  * @kpage: the KSM page that we want to map instead of page,
  *         or NULL the first time when we want to use page as kpage.
@@ -1506,7 +1498,7 @@ static int replace_page(struct vm_area_struct *vma, struct page *page,
  * This function returns 0 if the pages were merged, -EFAULT otherwise.
  */
 static int try_to_merge_one_page(struct vm_area_struct *vma,
-				 struct page *page, struct page *kpage)
+		unsigned long addr, struct page *page, struct page *kpage)
 {
 	struct folio *folio = page_folio(page);
 	pte_t orig_pte = __pte(0);
@@ -1515,7 +1507,7 @@ static int try_to_merge_one_page(struct vm_area_struct *vma,
 	if (page == kpage)			/* ksm page forked */
 		return 0;
 
-	if (!folio_test_anon(folio))
+	if (!folio_test_anon(folio) || folio_test_ksm(folio))
 		goto out;
 
 	/*
@@ -1539,7 +1531,7 @@ static int try_to_merge_one_page(struct vm_area_struct *vma,
 	 * ptes are necessarily already write-protected.  But in either
 	 * case, we need to lock and check page_count is not raised.
 	 */
-	if (write_protect_page(vma, folio, &orig_pte) == 0) {
+	if (write_protect_page(vma, folio, addr, &orig_pte) == 0) {
 		if (!kpage) {
 			/*
 			 * While we hold folio lock, upgrade folio from
@@ -1556,7 +1548,7 @@ static int try_to_merge_one_page(struct vm_area_struct *vma,
 				folio_mark_dirty(folio);
 			err = 0;
 		} else if (pages_identical(page, kpage))
-			err = replace_page(vma, page, kpage, orig_pte);
+			err = replace_page(vma, page, kpage, addr, orig_pte);
 	}
 
 out_unlock:
@@ -1572,6 +1564,7 @@ static int try_to_merge_one_page(struct vm_area_struct *vma,
 static int try_to_merge_with_zero_page(struct ksm_rmap_item *rmap_item,
 				       struct page *page)
 {
+	const unsigned long addr = rmap_item->address & PAGE_MASK;
 	struct mm_struct *mm = rmap_item->mm;
 	int err = -EFAULT;
 
@@ -1583,13 +1576,12 @@ static int try_to_merge_with_zero_page(struct ksm_rmap_item *rmap_item,
 		struct vm_area_struct *vma;
 
 		mmap_read_lock(mm);
-		vma = find_mergeable_vma(mm, rmap_item->address);
+		vma = find_mergeable_vma(mm, addr);
 		if (vma) {
-			err = try_to_merge_one_page(vma, page,
-					ZERO_PAGE(rmap_item->address));
-			trace_ksm_merge_one_page(
-				page_to_pfn(ZERO_PAGE(rmap_item->address)),
-				rmap_item, mm, err);
+			err = try_to_merge_one_page(vma, addr, page,
+						    ZERO_PAGE(addr));
+			trace_ksm_merge_one_page(page_to_pfn(ZERO_PAGE(addr)),
+						 rmap_item, mm, err);
 		} else {
 			/*
 			 * If the vma is out of date, we do not need to
@@ -1612,16 +1604,17 @@ static int try_to_merge_with_zero_page(struct ksm_rmap_item *rmap_item,
 static int try_to_merge_with_ksm_page(struct ksm_rmap_item *rmap_item,
 				      struct page *page, struct page *kpage)
 {
+	const unsigned long addr = rmap_item->address & PAGE_MASK;
 	struct mm_struct *mm = rmap_item->mm;
 	struct vm_area_struct *vma;
 	int err = -EFAULT;
 
 	mmap_read_lock(mm);
-	vma = find_mergeable_vma(mm, rmap_item->address);
+	vma = find_mergeable_vma(mm, addr);
 	if (!vma)
 		goto out;
 
-	err = try_to_merge_one_page(vma, page, kpage);
+	err = try_to_merge_one_page(vma, addr, page, kpage);
 	if (err)
 		goto out;
 

---
base-commit: 34f88a114888c9dba49e45f2b0c2a3e83c4c871c
change-id: 20261102-ksm-anon-rmap-fix-b54bb68df6ec

Cheers,
-- 
Lorenzo Stoakes (ARM) <ljs@kernel.org>


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

* Re: [PATCH] mm/ksm: avoid unsafe anon rmap dereference in KSM merge
  2026-11-02 12:32 [PATCH] mm/ksm: avoid unsafe anon rmap dereference in KSM merge Lorenzo Stoakes (ARM)
@ 2026-11-02 12:49 ` David Hildenbrand (Arm)
  2026-11-02 12:55   ` Lorenzo Stoakes (ARM)
  0 siblings, 1 reply; 4+ messages in thread
From: David Hildenbrand (Arm) @ 2026-11-02 12:49 UTC (permalink / raw)
  To: Lorenzo Stoakes (ARM),
	Xu Xin, Chengming Zhou, Andrew Morton, Rik van Riel
  Cc: linux-mm, linux-kernel, Zhao Gongyi

On 11/2/26 13:32, Lorenzo Stoakes (ARM) wrote:
> When KSM examines candidate VMAs in scan_get_next_rmap_item(), it
> accumulates (mm, address) pairs in ksm_rmap_item objects, assigning them
> via get_next_rmap_item(), pinning folios via a page walk in
> ksm_next_page_pmd_entry().
> 
> Throughout this operation, scan_get_next_rmap_item() holds mmap read lock,
> which it drops afterwards.
> 
> After the mmap lock is dropped the VMAs might be freed and remapped where
> the VMA belonging to the folio was previously mapped.
> 
> mremap()'s ability to change where anon folios are mapped is dealt with by
> KSM by essentially avoiding the problem - prep_move_vma() unshares KSM
> folios, and the scanning logic is tolerant of concurrent remaps, discarding
> stale ksm_rmap_item objects and allocating new ones at the new address.
> 
> This doesn't account for a new VMA being mapped over an old one where a
> folio was previously mapped, so KSM accounts for this by checking things at
> the page table level.
> 
> However, once an anon_vma's reference count reaches zero, it is freed, but
> nothing clears folio->mapping, which is where the KSM pin interacts badly
> with the current KSM implementation.
> 
> page_address_in_vma() is consistently used to look up the folio's address,
> but page_address_in_vma() explicitly states:
> 
> "It is the caller's responsibility to check the page is actually within the
> VMA."
> 
> And KSM violates this, as it cannot know this to be true. This results in
> this function performing a use-after-free:
> 
> 		struct anon_vma *anon_vma = folio_anon_vma(folio);
> 
> 		...
> 
> 		if (!vma_has_anon_rmap(vma) || !anon_vma ||
> 		    vma->anon_vma->root != anon_vma->root)
> 			return -EFAULT;
> 
> As anon_vma->root is dereferenced when anon_vma can already have been
> freed.
> 
> This is currently benign in practice, as anon_vma's are slab-allocated
> objects so this will always be within the direct mapping and arbitrary
> memory will be treated as if it were a pointer to be compared.
> 
> This either results in an -EFAULT or a spurious match, which will be
> rejected at the page table level.
> 
> However we mustn't UAF even if it's harmless, and KASAN rightly flags this.
> 
> It turns out we don't even need to use page_address_in_vma() as we already
> know the folio's address, it simply needs to be passed through.
> 
> This change therefore updates KSM to do this, sidestepping the issue
> altogether.
> 
> Explicitly check for KSM folios in try_to_merge_one_page(), which are
> already merged, and by definition never have an anon_vma.
> 
> This was previously handled implicitly by page_address_in_vma() as
> folio_anon_vma() returns NULL for those.
> 
> In many cases cmp_and_merge_page() will reject these, but it's possible for
> KSM folios to fall into the unstable tree there, so eliminate that
> possibility by checking for that early.
> 
> This maintains existing behaviour.
> 
> The bug was introduced by commit 21d0d443cdc1 ("rmap: resurrect
> page_address_in_vma anon_vma check") which first dereferenced
> folio->mapping unsafely, but commit b8c8ba73c68b ("slub: Introduce
> CONFIG_SLUB_RCU_DEBUG") made it visible with CONFIG_SLUB_RCU_DEBUG enabled.
> 
> Fixes: 21d0d443cdc1 ("rmap: resurrect page_address_in_vma anon_vma check")
> Reported-by: Zhao Gongyi <zhaogongyi@bytedance.com>
> Closes: https://lore.kernel.org/all/20261031182256.40205-1-zhaogongyi@bytedance.com/
> Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
> ---
> David - this seems benign in practice, so I'm not sure whether a backport
> is required here.

Ok, thanks. Is there a similar problem as with the other anon_vma->root issue
where a false positive match could be problematic?

I suspect not.

In that case I would pick this for the next merge window after a couple of days.

[...]

>  {
>  	struct folio *folio = page_folio(page);
>  	pte_t orig_pte = __pte(0);
> @@ -1515,7 +1507,7 @@ static int try_to_merge_one_page(struct vm_area_struct *vma,
>  	if (page == kpage)			/* ksm page forked */
>  		return 0;
>  
> -	if (!folio_test_anon(folio))
> +	if (!folio_test_anon(folio) || folio_test_ksm(folio))
>  		goto out;
Okay, that makes sense. We don't have to check under the folio lock as KSM
doesn't merge concurrently.

Do we want to assert in write_protect_page() that we are not getting called with
a KSM folio, similar to what we do with large folios in there?

-- 
Cheers,

David

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

* Re: [PATCH] mm/ksm: avoid unsafe anon rmap dereference in KSM merge
  2026-11-02 12:49 ` David Hildenbrand (Arm)
@ 2026-11-02 12:55   ` Lorenzo Stoakes (ARM)
  2026-11-02 13:39     ` David Hildenbrand (Arm)
  0 siblings, 1 reply; 4+ messages in thread
From: Lorenzo Stoakes (ARM) @ 2026-11-02 12:55 UTC (permalink / raw)
  To: David Hildenbrand (Arm)
  Cc: Xu Xin, Chengming Zhou, Andrew Morton, Rik van Riel, linux-mm,
	linux-kernel, Zhao Gongyi

On Mon, Nov 02, 2026 at 01:49:02PM +0100, David Hildenbrand (Arm) wrote:
> On 11/2/26 13:32, Lorenzo Stoakes (ARM) wrote:
> > When KSM examines candidate VMAs in scan_get_next_rmap_item(), it
> > accumulates (mm, address) pairs in ksm_rmap_item objects, assigning them
> > via get_next_rmap_item(), pinning folios via a page walk in
> > ksm_next_page_pmd_entry().
> >
> > Throughout this operation, scan_get_next_rmap_item() holds mmap read lock,
> > which it drops afterwards.
> >
> > After the mmap lock is dropped the VMAs might be freed and remapped where
> > the VMA belonging to the folio was previously mapped.
> >
> > mremap()'s ability to change where anon folios are mapped is dealt with by
> > KSM by essentially avoiding the problem - prep_move_vma() unshares KSM
> > folios, and the scanning logic is tolerant of concurrent remaps, discarding
> > stale ksm_rmap_item objects and allocating new ones at the new address.
> >
> > This doesn't account for a new VMA being mapped over an old one where a
> > folio was previously mapped, so KSM accounts for this by checking things at
> > the page table level.
> >
> > However, once an anon_vma's reference count reaches zero, it is freed, but
> > nothing clears folio->mapping, which is where the KSM pin interacts badly
> > with the current KSM implementation.
> >
> > page_address_in_vma() is consistently used to look up the folio's address,
> > but page_address_in_vma() explicitly states:
> >
> > "It is the caller's responsibility to check the page is actually within the
> > VMA."
> >
> > And KSM violates this, as it cannot know this to be true. This results in
> > this function performing a use-after-free:
> >
> > 		struct anon_vma *anon_vma = folio_anon_vma(folio);
> >
> > 		...
> >
> > 		if (!vma_has_anon_rmap(vma) || !anon_vma ||
> > 		    vma->anon_vma->root != anon_vma->root)
> > 			return -EFAULT;
> >
> > As anon_vma->root is dereferenced when anon_vma can already have been
> > freed.
> >
> > This is currently benign in practice, as anon_vma's are slab-allocated
> > objects so this will always be within the direct mapping and arbitrary
> > memory will be treated as if it were a pointer to be compared.
> >
> > This either results in an -EFAULT or a spurious match, which will be
> > rejected at the page table level.
> >
> > However we mustn't UAF even if it's harmless, and KASAN rightly flags this.
> >
> > It turns out we don't even need to use page_address_in_vma() as we already
> > know the folio's address, it simply needs to be passed through.
> >
> > This change therefore updates KSM to do this, sidestepping the issue
> > altogether.
> >
> > Explicitly check for KSM folios in try_to_merge_one_page(), which are
> > already merged, and by definition never have an anon_vma.
> >
> > This was previously handled implicitly by page_address_in_vma() as
> > folio_anon_vma() returns NULL for those.
> >
> > In many cases cmp_and_merge_page() will reject these, but it's possible for
> > KSM folios to fall into the unstable tree there, so eliminate that
> > possibility by checking for that early.
> >
> > This maintains existing behaviour.
> >
> > The bug was introduced by commit 21d0d443cdc1 ("rmap: resurrect
> > page_address_in_vma anon_vma check") which first dereferenced
> > folio->mapping unsafely, but commit b8c8ba73c68b ("slub: Introduce
> > CONFIG_SLUB_RCU_DEBUG") made it visible with CONFIG_SLUB_RCU_DEBUG enabled.
> >
> > Fixes: 21d0d443cdc1 ("rmap: resurrect page_address_in_vma anon_vma check")
> > Reported-by: Zhao Gongyi <zhaogongyi@bytedance.com>
> > Closes: https://lore.kernel.org/all/20261031182256.40205-1-zhaogongyi@bytedance.com/
> > Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
> > ---
> > David - this seems benign in practice, so I'm not sure whether a backport
> > is required here.
>
> Ok, thanks. Is there a similar problem as with the other anon_vma->root issue
> where a false positive match could be problematic?
>
> I suspect not.

No, as it checks stuff at the page table level later.

>
> In that case I would pick this for the next merge window after a couple of days.

Yup, this is far from urgent.

>
> [...]
>
> >  {
> >  	struct folio *folio = page_folio(page);
> >  	pte_t orig_pte = __pte(0);
> > @@ -1515,7 +1507,7 @@ static int try_to_merge_one_page(struct vm_area_struct *vma,
> >  	if (page == kpage)			/* ksm page forked */
> >  		return 0;
> >
> > -	if (!folio_test_anon(folio))
> > +	if (!folio_test_anon(folio) || folio_test_ksm(folio))
> >  		goto out;
> Okay, that makes sense. We don't have to check under the folio lock as KSM
> doesn't merge concurrently.
>
> Do we want to assert in write_protect_page() that we are not getting called with
> a KSM folio, similar to what we do with large folios in there?

Sure could do. I wonder whether that existing check should just be a
VM_WARN_ON_ONCE() anyway, seems a bit silly to if (WARN_ON_ONCE(...)) return
err; there.

Do you want a respin for that or happy to add?

--
Cheers, Lorenzo

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

* Re: [PATCH] mm/ksm: avoid unsafe anon rmap dereference in KSM merge
  2026-11-02 12:55   ` Lorenzo Stoakes (ARM)
@ 2026-11-02 13:39     ` David Hildenbrand (Arm)
  0 siblings, 0 replies; 4+ messages in thread
From: David Hildenbrand (Arm) @ 2026-11-02 13:39 UTC (permalink / raw)
  To: Lorenzo Stoakes (ARM)
  Cc: Xu Xin, Chengming Zhou, Andrew Morton, Rik van Riel, linux-mm,
	linux-kernel, Zhao Gongyi

On 11/2/26 13:55, Lorenzo Stoakes (ARM) wrote:
> On Mon, Nov 02, 2026 at 01:49:02PM +0100, David Hildenbrand (Arm) wrote:
>> On 11/2/26 13:32, Lorenzo Stoakes (ARM) wrote:
>>> When KSM examines candidate VMAs in scan_get_next_rmap_item(), it
>>> accumulates (mm, address) pairs in ksm_rmap_item objects, assigning them
>>> via get_next_rmap_item(), pinning folios via a page walk in
>>> ksm_next_page_pmd_entry().
>>>
>>> Throughout this operation, scan_get_next_rmap_item() holds mmap read lock,
>>> which it drops afterwards.
>>>
>>> After the mmap lock is dropped the VMAs might be freed and remapped where
>>> the VMA belonging to the folio was previously mapped.
>>>
>>> mremap()'s ability to change where anon folios are mapped is dealt with by
>>> KSM by essentially avoiding the problem - prep_move_vma() unshares KSM
>>> folios, and the scanning logic is tolerant of concurrent remaps, discarding
>>> stale ksm_rmap_item objects and allocating new ones at the new address.
>>>
>>> This doesn't account for a new VMA being mapped over an old one where a
>>> folio was previously mapped, so KSM accounts for this by checking things at
>>> the page table level.
>>>
>>> However, once an anon_vma's reference count reaches zero, it is freed, but
>>> nothing clears folio->mapping, which is where the KSM pin interacts badly
>>> with the current KSM implementation.
>>>
>>> page_address_in_vma() is consistently used to look up the folio's address,
>>> but page_address_in_vma() explicitly states:
>>>
>>> "It is the caller's responsibility to check the page is actually within the
>>> VMA."
>>>
>>> And KSM violates this, as it cannot know this to be true. This results in
>>> this function performing a use-after-free:
>>>
>>> 		struct anon_vma *anon_vma = folio_anon_vma(folio);
>>>
>>> 		...
>>>
>>> 		if (!vma_has_anon_rmap(vma) || !anon_vma ||
>>> 		    vma->anon_vma->root != anon_vma->root)
>>> 			return -EFAULT;
>>>
>>> As anon_vma->root is dereferenced when anon_vma can already have been
>>> freed.
>>>
>>> This is currently benign in practice, as anon_vma's are slab-allocated
>>> objects so this will always be within the direct mapping and arbitrary
>>> memory will be treated as if it were a pointer to be compared.
>>>
>>> This either results in an -EFAULT or a spurious match, which will be
>>> rejected at the page table level.
>>>
>>> However we mustn't UAF even if it's harmless, and KASAN rightly flags this.
>>>
>>> It turns out we don't even need to use page_address_in_vma() as we already
>>> know the folio's address, it simply needs to be passed through.
>>>
>>> This change therefore updates KSM to do this, sidestepping the issue
>>> altogether.
>>>
>>> Explicitly check for KSM folios in try_to_merge_one_page(), which are
>>> already merged, and by definition never have an anon_vma.
>>>
>>> This was previously handled implicitly by page_address_in_vma() as
>>> folio_anon_vma() returns NULL for those.
>>>
>>> In many cases cmp_and_merge_page() will reject these, but it's possible for
>>> KSM folios to fall into the unstable tree there, so eliminate that
>>> possibility by checking for that early.
>>>
>>> This maintains existing behaviour.
>>>
>>> The bug was introduced by commit 21d0d443cdc1 ("rmap: resurrect
>>> page_address_in_vma anon_vma check") which first dereferenced
>>> folio->mapping unsafely, but commit b8c8ba73c68b ("slub: Introduce
>>> CONFIG_SLUB_RCU_DEBUG") made it visible with CONFIG_SLUB_RCU_DEBUG enabled.
>>>
>>> Fixes: 21d0d443cdc1 ("rmap: resurrect page_address_in_vma anon_vma check")
>>> Reported-by: Zhao Gongyi <zhaogongyi@bytedance.com>
>>> Closes: https://lore.kernel.org/all/20261031182256.40205-1-zhaogongyi@bytedance.com/
>>> Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
>>> ---
>>> David - this seems benign in practice, so I'm not sure whether a backport
>>> is required here.
>>
>> Ok, thanks. Is there a similar problem as with the other anon_vma->root issue
>> where a false positive match could be problematic?
>>
>> I suspect not.
> 
> No, as it checks stuff at the page table level later.
> 
>>
>> In that case I would pick this for the next merge window after a couple of days.
> 
> Yup, this is far from urgent.
> 
>>
>> [...]
>>
>>>  {
>>>  	struct folio *folio = page_folio(page);
>>>  	pte_t orig_pte = __pte(0);
>>> @@ -1515,7 +1507,7 @@ static int try_to_merge_one_page(struct vm_area_struct *vma,
>>>  	if (page == kpage)			/* ksm page forked */
>>>  		return 0;
>>>
>>> -	if (!folio_test_anon(folio))
>>> +	if (!folio_test_anon(folio) || folio_test_ksm(folio))
>>>  		goto out;
>> Okay, that makes sense. We don't have to check under the folio lock as KSM
>> doesn't merge concurrently.
>>
>> Do we want to assert in write_protect_page() that we are not getting called with
>> a KSM folio, similar to what we do with large folios in there?
> 
> Sure could do. I wonder whether that existing check should just be a
> VM_WARN_ON_ONCE() anyway, seems a bit silly to if (WARN_ON_ONCE(...)) return
> err; there.

Just what I was thinking.

> 
> Do you want a respin for that or happy to add?
I can fix that up when applying. If there are more requests for changes, let's
do a resend.

-- 
Cheers,

David

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

end of thread, other threads:[~2026-11-02 13:39 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-11-02 12:32 [PATCH] mm/ksm: avoid unsafe anon rmap dereference in KSM merge Lorenzo Stoakes (ARM)
2026-11-02 12:49 ` David Hildenbrand (Arm)
2026-11-02 12:55   ` Lorenzo Stoakes (ARM)
2026-11-02 13:39     ` David Hildenbrand (Arm)

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

all inboxes | Powered by JetHome®