Re: [PATCH 3/4] KVM: guest_memfd: Add `write` parameter to kvm_gmem_populate()

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The integer `write` parameter is passed directly as the `gup_flags` bitmask to `get_user_pages_fast()`, treating a bitmask as a boolean.

Pre-existing issues:
- [Medium] Modified user pages acquired via `get_user_pages_fast()` are not marked dirty before their reference is dropped, leading to potential data loss.
--

commit 49441863078cd5114f78eb8c75f1d31489417864
Author: Joerg Roedel <joerg.roedel@amd.com>

KVM: guest_memfd: Add write parameter to kvm_gmem_populate()

This commit adds a write parameter to kvm_gmem_populate() to optionally
acquire a writable reference to the user-space page via get_user_pages_fast().
This ensures page permissions are enforced when KVM needs to write error
details back to userspace.

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 69c9d6d546b28..7a245a402a1b2 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
[ ... ]
> @@ -893,7 +893,7 @@ 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;
>  
> -			ret = get_user_pages_fast(uaddr, 1, 0, &src_page);
> +			ret = get_user_pages_fast(uaddr, 1, write, &src_page);

[Severity: Low]
Does this pass an integer parameter directly into the gup_flags argument?
While the integer value 1 coincidentally evaluates to FOLL_WRITE (1 << 0),
is it safe to rely on the numeric value of an internal macro definition here?
Would it be more robust to explicitly pass FOLL_WRITE based on the write
parameter?

[Severity: Medium]
This is a pre-existing issue, but since we are explicitly handling writable
pages here, are we missing a dirty page notification before the reference
is dropped?

If a user-provided CPUID page fails validation during a SEV-SNP VM launch,
sev_gmem_post_populate() modifies the page directly via memcpy() bypassing
the user page tables. However, kvm_gmem_populate() drops the reference
shortly after:

virt/kvm/guest_memfd.c:kvm_gmem_populate() {
    ...
    ret = __kvm_gmem_populate(...);
    
    if (src_page)
        put_page(src_page);
    ...
}

Does this drop the reference without calling set_page_dirty_lock() or an
equivalent dirtying function? If so, could the memory management subsystem
reclaim the unmarked page and cause userspace to read stale data instead of
the detailed error information?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623091556.1500930-1-joro@8bytes.org?part=3