Re: [PATCH] mm/ksm: avoid unsafe anon rmap dereference in KSM merge

[Lorenzo Stoakes (ARM)]

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@xxxxxxxxxxxxx>
> > Closes: https://lore.kernel.org/all/20261031182256.40205-1-zhaogongyi@xxxxxxxxxxxxx/
> > Signed-off-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>
> > ---
> > 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