Re: [PATCH] KVM: SEV: Fix page dirtying in sev_gmem_post_populate()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Leaked SEV-SNP pages (via snp_leak_pages) are incorrectly returned to the buddy allocator, causing host panics (RMP violations) upon reuse.
- [High] Unprivileged host userspace can trigger a kernel warning (WARN_ON_ONCE) in sev_gmem_post_populate() by passing a NULL user address.
--

--- Patch [1]: [PATCH] KVM: SEV: Fix page dirtying in sev_gmem_post_populate() ---
commit 6e5ffd2f92a045c6125ad2d714ea329e1a9b8c9a
Author: Jann Horn <[email protected]>

KVM: SEV: Fix page dirtying in sev_gmem_post_populate()

This patch fixes a potential use-after-free by replacing set_page_dirty()
with set_page_dirty_lock() for pages obtained from get_user_pages_fast().
This ensures the page's mapping cannot concurrently go away during the
dirtying process.

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 0f0ea7896af5b..49feef14787af 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2337,7 +2337,7 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
>  	bool assigned = false;
>  	int level;
>  	int ret;
>  
>  	if (WARN_ON_ONCE(sev_populate_args->type != KVM_SEV_SNP_PAGE_TYPE_ZERO && !src_page))

[Severity: High]
This isn't a bug introduced by this patch, but can an unprivileged host
userspace process trigger this WARN_ON_ONCE by passing a NULL user address?

If a userspace process invokes the KVM_SEV_SNP_LAUNCH_UPDATE ioctl with
params.type == KVM_SEV_SNP_PAGE_TYPE_CPUID and params.uaddr == 0, the
snp_launch_update() function computes src = NULL and bypasses the
PAGE_ALIGNED check:

arch/x86/kvm/svm/sev.c:snp_launch_update() {
    src = params.type == KVM_SEV_SNP_PAGE_TYPE_ZERO ? NULL :
          u64_to_user_ptr(params.uaddr);
    if (!PAGE_ALIGNED(src))
        return -EINVAL;
}

Since 0 is page-aligned, src remains NULL. When kvm_gmem_populate() is
subsequently called, it skips fetching user pages because src is NULL:

virt/kvm/guest_memfd.c:kvm_gmem_populate() {
    if (src) {
        unsigned long uaddr = (unsigned long)src + i * PAGE_SIZE;
        ...
    }
}

This leaves src_page as NULL when it is passed to sev_gmem_post_populate(),
which triggers the WARN_ON_ONCE. Does this allow a malicious user to trigger
a host-side denial of service if the kernel boots with panic_on_warn?

>  		return -EINVAL;

[ ... ]

> @@ -2381,15 +2381,15 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
>  	 * Copy this page back into the source buffer so userspace can use this
>  	 * information to provide information on which CPUID leaves/fields
>  	 * failed CPUID validation.
>  	 */
>  	if (ret && !snp_page_reclaim(kvm, pfn) &&

[Severity: Critical]
This is a pre-existing issue, but does snp_page_reclaim() properly manage
reference counts for leaked pages?

If snp_page_reclaim() fails, it permanently leaks the page via
snp_leak_pages(). However, the page is added to the snp_leaked_pages_list
without taking an additional reference:

arch/x86/virt/svm/sev.c:__snp_leak_pages() {
    if (likely(!PageCompound(page)) || ...)
        list_add_tail(&page->buddy_list, &snp_leaked_pages_list);
}

For pages from guest_memfd, the owning subsystem controls the reference. When
operations drop references to the leaked folio (such as when the file is
closed or truncated), the reference count can drop to zero:

virt/kvm/guest_memfd.c:__kvm_gmem_populate() {
out_put_folio:
    folio_put(folio);
}

Could this cause the leaked SEV-SNP page to be erroneously returned to the
buddy allocator while still in a hardware-enforced private state, leading to
fatal RMP violations upon subsequent reuse?

>  	    sev_populate_args->type == KVM_SEV_SNP_PAGE_TYPE_CPUID &&
>  	    sev_populate_args->fw_error == SEV_RET_INVALID_PARAM) {
>  		void *src_vaddr = kmap_local_page(src_page);
>  		void *dst_vaddr = kmap_local_pfn(pfn);
>  
>  		memcpy(src_vaddr, dst_vaddr, PAGE_SIZE);
> -		set_page_dirty(src_page);
> +		set_page_dirty_lock(src_page);
>  
>  		kunmap_local(dst_vaddr);
>  		kunmap_local(src_vaddr);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1