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

[David Hildenbrand (Arm)]

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.

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