Re: [PATCH] mm/ksm: avoid anon_vma UAF during page merging

[xu . xin]

> KASAN reports a use-after-free in page_address_in_vma() while ksmd
> merges pages. ksmd keeps a folio reference across an mmap_lock drop,
> during which mremap() can remove its last mapping and free its
> anon_vma. page_address_in_vma() then dereferences the stale pointer
> from folio->mapping.
> 
> KSM needs only a candidate address. write_protect_page() checks that
> the PTE maps the folio, and replace_page() verifies the saved PTE
> before replacing it. Compute the candidate from the folio index and
> VMA instead of dereferencing the folio-side anon_vma. Reject KSM
> folios because their page index is not meaningful.
> 
> Fixes: 21d0d443cdc1 ("rmap: resurrect page_address_in_vma anon_vma check")
> Signed-off-by: Zhao Gongyi <zhaogongyi@xxxxxxxxxxxxx>
> ---
> Testing:
>   Unpatched mm-new: fixed-pool KSM/mremap reproduced the KASAN UAF.
>   Patched mm-new: the same workload ran for 12 minutes without KASAN,
>   BUG or WARNING; fork/COW KSM smoke passed.

Thanks for the report.

I’d like to ask whether this issue is easy to reproduce on 6.18 since
I suspect the accuracy of the “fixes” tag in the commit description.

Besides, adding an extra folio_test_ksm() check is unneeded, see reasons below. 

> 
>  mm/ksm.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/ksm.c b/mm/ksm.c
> index d6f736a..f8af5ee 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -1307,7 +1307,7 @@ 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);
> +	pvmw.address = vma_anon_address(vma, folio_pgoff(folio), 1);
>  	if (pvmw.address == -EFAULT)
>  		goto out;
>  
> @@ -1408,7 +1408,7 @@ static int replace_page(struct vm_area_struct *vma, struct page *page,
>  	int err = -EFAULT;
>  	struct mmu_notifier_range range;
>  
> -	addr = page_address_in_vma(folio, page, vma);
> +	addr = vma_anon_address(vma, page_pgoff(folio, page), 1);
>  	if (addr == -EFAULT)
>  		goto out;
>  
> @@ -1515,7 +1515,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;

The check folio_test_ksm(folio)) in try_to_merge_one_page is unnecessary, because before
try_to_merge_one_page() is called, cmp_and_merge_page() invokes folio_stable_node(), which already checks
whether this is a KSM page. Only KSM fork pages are allowed to pass through; even if they do pass, they
will ultimately be intercepted by the if (page == kpage) check in try_to_merge_one_page.