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

["David Hildenbrand (Arm)" <david@kernel.org>]

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