Re: [PATCH RFC 04/17] KVM: guest_memfd: Add helper to attach resource provider file

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] kvm: Missing .release_folio in guest_memfd causes host-to-guest UAF
- [High] kvm: ABI regression due to conditionally enforcing uninitialized resource_fd
--

commit dd40066faee48c23e279dbcc865f3d9effd7f680
Author: Ackerley Tng <ackerleytng@google.com>

KVM: guest_memfd: Add helper to attach resource provider file

This commit adds a helper to attach a resource provider file to a guest_memfd
inode. It takes a refcount on the resource_fd to maintain the reference for
guest_memfd and introduces a release callback on inode destruction to allow
provider cleanup.

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 00f3bd0cf21e6..0e0d7e93f4148 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c

[ ... ]

> @@ -1057,7 +1099,10 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
>  	if (size <= 0 || !PAGE_ALIGNED(size))
>  		return -EINVAL;
>  
> -	return __kvm_gmem_create(kvm, size, flags);
> +	if (!(flags & GUEST_MEMFD_FLAG_USE_RESOURCE) && args->resource_fd)
> +		return -EINVAL;

[Severity: High]
Does this introduce a backwards-compatibility issue with the ABI?

Older userspace applications compiled against older kernel headers might call
KVM_CREATE_GUEST_MEMFD with an uninitialized stack structure. Previously,
KVM did not check the reserved fields, so stack garbage was ignored.

By enforcing args->resource_fd to be 0 when GUEST_MEMFD_FLAG_USE_RESOURCE is
not set, won't this cause unexpected -EINVAL failures for old applications
passing uninitialized stack garbage?

Also, this patch does not validate the remaining pad and reserved[5] fields,
which could lead to this exact problem repeating when those fields are
repurposed in the future.

> +
> +	return __kvm_gmem_create(kvm, size, flags, args->resource_fd);
>  }

[ ... ]

> @@ -1401,6 +1446,8 @@ static void kvm_gmem_destroy_inode(struct inode *inode)
>  {
>  	struct gmem_inode *gi = GMEM_I(inode);
>  
> +	gmem_provider_release(gi);
> +

[Severity: Critical]
Is there a missing .release_folio callback in kvm_gmem_aops?

An earlier patch in this series set
mapping_set_release_always(inode->i_mapping) in kvm_gmem_init_inode(), but
kvm_gmem_aops does not implement the .release_folio callback:

static const struct address_space_operations kvm_gmem_aops = {
	.dirty_folio = noop_dirty_folio,
	.migrate_folio	= kvm_gmem_migrate_folio,
	.error_remove_folio = kvm_gmem_error_folio,
	.invalidate_folio = kvm_gmem_invalidate_folio,
#ifdef CONFIG_HAVE_KVM_ARCH_GMEM_RECLAIM
	.free_folio = kvm_gmem_free_folio,
#endif
};

If userspace calls fadvise(POSIX_FADV_DONTNEED) on the guest_memfd file
(or if root triggers drop_caches), this will trigger mapping_evict_folio()
which then calls filemap_release_folio().

Without .release_folio, VFS falls back to try_to_free_buffers(). This
successfully evicts the folio and frees the physical page, but completely
bypasses KVM SPTE invalidation. Can this lead to a host-to-guest
use-after-free, where a guest retains read/write access to host physical
pages after they are freed and reallocated to other processes?

>  	mpol_free_shared_policy(&gi->policy);
>  
>  	/*

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925-gmem-tmpfs-backend-v1-0-d36159822d18@google.com?part=4