Re: [PATCH 1/3] KVM: guest_memfd: Use write permissions when GUP-ing source pages

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Modifications to the pinned user space source page (`src_page`) are not tracked with `set_page_dirty_lock()`, risking silent data loss if the page is evicted.
- [High] Userspace can maliciously trigger a `WARN_ON_ONCE` kernel panic by passing a zero `uaddr` for non-ZERO page types, leading to a potential denial of service.
--

commit 6a7184de221e96f7e4e45a9996f99d0f23dabd6a
Author: Sean Christopherson <seanjc@google.com>

KVM: guest_memfd: Use write permissions when GUP-ing source pages

sev_gmem_post_populate() may write to the source page if there was an error
while performing SNP_LAUNCH_UPDATE.

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2469,6 +2469,7 @@ static int snp_launch_update(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	sev_populate_args.type = params.type;
>  
>  	count = kvm_gmem_populate(kvm, params.gfn_start, src, npages,
> +				  params.type == KVM_SEV_SNP_PAGE_TYPE_CPUID,
>  				  sev_gmem_post_populate, &sev_populate_args);

[Severity: High]
This is a pre-existing issue, but could userspace intentionally trigger a
WARN_ON_ONCE kernel panic here by passing a zero uaddr for non-ZERO page
types?

In snp_launch_update(), if userspace provides params.uaddr = 0 alongside a
non-ZERO page type (e.g., KVM_SEV_SNP_PAGE_TYPE_CPUID), src is set to NULL
because 0 bypasses the PAGE_ALIGNED(src) validation.

When kvm_gmem_populate() is called with src = NULL, it skips
get_user_pages_fast() and leaves src_page as NULL. The __kvm_gmem_populate()
function then invokes the sev_gmem_post_populate() callback with
src_page = NULL, which triggers the WARN_ON_ONCE in the callback. On
production systems configured with panic_on_warn=1, this could allow an
unprivileged local user to crash the host.

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
[ ... ]
> @@ -892,8 +893,9 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
>  
>  		if (src) {
>  			unsigned long uaddr = (unsigned long)src + i * PAGE_SIZE;
> +			unsigned int flags = may_writeback_src ? FOLL_WRITE : 0;
>  
> -			ret = get_user_pages_fast(uaddr, 1, 0, &src_page);
> +			ret = get_user_pages_fast(uaddr, 1, flags, &src_page);

[Severity: High]
This is a pre-existing issue, but does this miss tracking modifications to
the pinned user space source page (src_page)?

While the FOLL_WRITE flag correctly handles the copy-on-write,
in sev_gmem_post_populate(), if the SEV-SNP firmware returns an invalid
parameter error for a CPUID page, KVM copies the expected values into the
user-provided src_page via kmap_local_page() and memcpy().

However, kvm_gmem_populate() uses get_user_pages_fast() to pin the page
and subsequently calls put_page(src_page) without explicitly marking the
page as dirty (e.g., using set_page_dirty_lock()).

Because the modification bypasses the user's page tables and directly
writes to the kernel's direct map, the user PTE's hardware dirty bit
remains unset. If the page is file-backed or swap-backed and memory
pressure forces it to be reclaimed, the memory subsystem might see both
the PTE and the struct page as clean, discarding the page without writing
the changes to the backing store. This would cause the user-space process
to silently read stale CPUID data.

>  			if (ret < 0)
>  				break;
>  			if (ret != 1) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522-fix-sev-gmem-post-populate-v1-0-9fc8d6437b65@google.com?part=1