Re: [PATCH] KVM: SEV: Don't return a still-assigned gmem page to the host

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Calling snp_leak_pages() on a pinned guest_memfd folio corrupts the system-wide LRU list by overwriting the folio's LRU pointers.
- [High] Unconditional PAGE_SIZE-stepping PFN iteration on large folios results in duplicate folio_get() increments on the same folio and failure to track leaked pages.
--

commit 5d574de6d0d362139fa648b3657e2f24c06fd5ee
Author: Hyunwoo Kim <imv4bel@gmail.com>

KVM: SEV: Don't return a still-assigned gmem page to the host

This commit addresses an issue where guest_memfd frees a page that is
still assigned to the guest. To prevent returning it to the host, the
page is pinned and leaked.

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index f59c65abe3cfa..dbde6dce79b76 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
[ ... ]
> @@ -5036,8 +5036,12 @@ void sev_gmem_invalidate(kvm_pfn_t start, kvm_pfn_t end)
>  
>  		rc = rmp_make_shared(pfn, use_2m_update ? PG_LEVEL_2M : PG_LEVEL_4K);
>  		if (WARN_ONCE(rc, "SEV: Failed to update RMP entry for PFN 0x%llx error %d\n",
> -			      pfn, rc))
> +			      pfn, rc)) {
> +			/* Still assigned to the guest; pin and leak rather than freeing. */
> +			folio_get(page_folio(pfn_to_page(pfn)));
> +			snp_leak_pages(pfn, use_2m_update ? PTRS_PER_PMD : 1);

[Severity: Critical]
Does calling snp_leak_pages() on a pinned guest_memfd folio corrupt the
system-wide LRU list?

Memory allocated by guest_memfd is added to the page cache, which
unconditionally places the memory on the global unevictable LRU list. When
guest_memfd memory is freed, the folio has been removed from the page cache
but is still actively linked in the global LRU list until its reference
count drops to zero.

By calling folio_get() here, the reference count is elevated, keeping the
folio on the LRU list. Then, snp_leak_pages() unconditionally links the
page into a tracking list via list_add_tail(&page->buddy_list, ...).

Because buddy_list and lru share the exact same union inside struct page
and struct folio, modifying buddy_list silently overwrites the lru.next and
lru.prev pointers of the folio, severing the global LRU linked list.

[Severity: High]
Does this code also double-account references and fail to track the leaked
pages when processing large folios?

When guest_memfd allocates a large 2MB PMD-size folio, but the RMP table
has it mapped as 4K entries, use_2m_update will be false and the loop
unconditionally steps by 1 PFN.

Because page_folio(pfn_to_page(pfn)) returns the head folio for any subpage,
folio_get() will be redundantly called up to 512 times on the exact same
head folio.

Furthermore, passing use_2m_update ? PTRS_PER_PMD : 1 (which is 1) to
snp_leak_pages() results in the internal check compound_nr(page) <= npages
evaluating to 512 <= 1 (false). This causes the entire compound page to be
completely skipped and omitted from the snp_leaked_pages_list.

>  			goto next_pfn;
> +		}
>  
>  		/*
>  		 * SEV-ES avoids host/guest cache coherency issues through

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/aimMWzAf5b3luM0b@v4bel?part=1