Re: [RFC PATCH 04/11] KVM: guest_memfd: Introduce guest memory ops and route native gmem through them

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Dropped page reference in `kvm_gmem_populate_one` causes a TOCTOU Use-After-Free on physical pages.
- [High] Use-After-Free (UAF) in `kvm_gmem_unbind` due to lockless dereference of unrefcounted `slot->gmem.file`.
- [High] Missing module reference count in `kvm_gmem_ops` allows foreign backend unload, causing kernel panic.
- [High] `kvm_gmem_bind` API contract fails to initialize `slot->gmem.file`, breaking KVM core state tracking.
--

--- Patch [4]: [RFC PATCH 04/11] KVM: guest_memfd: Introduce guest memory ops and route native gmem through them ---
commit 4ae3212093e20a291a948708162e27b1bb7009da
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: guest_memfd: Introduce guest memory ops and route native gmem through them

This commit introduces a kvm_gmem_ops vtable to allow other subsystems to
supply guest memory backing. It routes the built-in guest_memfd operations
through this vtable and refactors the generic bind, unbind, and populate
flows.

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index ab8cfaec82d31..b33d2f4750426 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
[ ... ]
> @@ -621,6 +621,88 @@ static inline bool kvm_slot_has_gmem(const struct kvm_memory_slot *slot)
>  	return slot && (slot->flags & KVM_MEM_GUEST_MEMFD);
>  }
>  
> +#ifdef CONFIG_KVM_GUEST_MEMFD
> +/*
> + * struct kvm_gmem_ops - vtable a guest_memfd backing implementation exposes.
> + *
> + * Every file that can back KVM guest memory -- KVM's own guest_memfd (see
> + * kvm_gmem_create()) and any other subsystem opting in -- is created with
> + * the shared kvm_gmem_fops (see is_kvm_gmem_file()).  file->private_data on
> + * such a file points at a struct kvm_gmem_backing whose ->ops field is the
> + * per-instance vtable below.  KVM never looks any further; the rest of the
> + * per-file state (allocator, refcount, private xarray, ...) is up to the
> + * implementation and is reached via container_of() from the backing.
> + *
> + * The memory-side callbacks (bind, unbind, get_pfn, populate, invalidate)
> + * are the reason the interface exists.  The file-level callbacks (release,
> + * mmap, fallocate, ...) are dispatched by kvm_gmem_fops so that every
> + * implementation gets a private hook -- see the dispatchers in
> + * virt/kvm/guest_memfd.c.
> + *
> + * get_pfn() contract:
> + *  - *pfn is the host PFN backing @gfn.  It need not have a struct page.
> + *  - *max_order is the largest order KVM may use to map starting at @gfn.
> + *    The implementation MUST clamp it to the physical contiguity and the
> + *    HPA/GPA alignment of the backing, so KVM never builds a huge SPTE that
> + *    spans discontiguous physical memory.
> + *  - If *page is non-NULL on return the caller owns that page reference and
> + *    is responsible for put_page() after use.  If *page is left NULL the PFN
> + *    is treated as non-refcounted, and its lifetime is owned by the
> + *    implementation across bind()/unbind().
> + *
> + * Memory intended to back guest RAM MUST be reported as E820_TYPE_RAM by the
> + * host so KVM maps it write-back (and applies the memory-encryption bit on
> + * encrypted hosts) rather than treating it as MMIO.
> + */
> +struct kvm_gmem_ops {
> +	/* Memory operations. */

[Severity: High]
Is there a missing module reference count in struct kvm_gmem_ops? 

The operations table lacks an owner field (like struct module *owner) to pin
the backend module.

Since the shared kvm_gmem_fops routes file operations to a foreign backend's
vtable, could a user open the guest_memfd, unload the backend module, and
then close the file? The release dispatcher would then call into the unloaded
module's release hook, potentially causing a kernel panic.

> +	int (*bind)(struct file *file, struct kvm *kvm,
> +		    struct kvm_memory_slot *slot, loff_t offset);
[ ... ]
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index db57c5766ab60..2b277468a12f0 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
[ ... ]
> @@ -692,15 +782,51 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
[ ... ]
> +int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
> +		  unsigned int fd, uoff_t offset)
> +{
> +	const struct kvm_gmem_ops *ops;
> +	struct file *file;
> +	int r;
> +
> +	BUILD_BUG_ON(sizeof(gpa_t) != sizeof(offset));
> +	BUILD_BUG_ON(sizeof(gfn_t) != sizeof(slot->gmem.pgoff));
> +
> +	file = fget(fd);
> +	if (!file)
> +		return -EBADF;
> +
> +	ops = kvm_gmem_get_ops(file);
> +	if (!ops || !ops->bind || !ops->get_pfn) {
> +		r = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (!PAGE_ALIGNED(offset)) {
> +		r = -EINVAL;
> +		goto out;
> +	}
>  
>  	/*
> -	 * Drop the reference to the file, even on success.  The file pins KVM,
> -	 * not the other way 'round.  Active bindings are invalidated if the
> -	 * file is closed before memslots are destroyed.
> +	 * On success .bind() associates the fd with @kvm and (if the
> +	 * implementation needs it) records the reference that makes the fd
> +	 * pin the VM.  A .bind() that returns success is trusted to have
> +	 * done so; there is no separate ownership check here.
> +	 */
> +	r = ops->bind(file, kvm, slot, offset);
> +out:
> +	/*
> +	 * Drop the reference to the file even on success.  The file pins
> +	 * KVM, not the other way 'round: it is the implementation's job to
> +	 * invalidate its bindings if the fd is closed before the memslots
> +	 * are destroyed.
>  	 */
> -	r = 0;
> -err:
>  	fput(file);
>  	return r;
>  }

[Severity: High]
Does this function miss initializing slot->gmem.file on a successful bind?

KVM core code relies heavily on slot->gmem.file being populated to later call
unbind. If a foreign backend succeeds in ops->bind but doesn't intentionally
mutate KVM's internal slot->gmem.file state, KVM might never call unbind
when the memslot is subsequently freed.

Could this leak backend resources or leave dangling memslot pointers in the
backend?

> @@ -741,15 +861,41 @@ void kvm_gmem_unbind(struct kvm_memory_slot *slot)
[ ... ]
> +void kvm_gmem_unbind(struct kvm *kvm, struct kvm_memory_slot *slot)
> +{
> +	const struct kvm_gmem_ops *ops;
> +	struct file *file;
> +
> +	/*
> +	 * Nothing to do if the underlying file was _already_ closed, as
> +	 * kvm_gmem_release() invalidates and nullifies all bindings.
> +	 */
> +	file = READ_ONCE(slot->gmem.file);
> +	if (!file)
> +		return;
> +
> +	ops = kvm_gmem_get_ops(file);

[Severity: High]
Is it safe to dereference the file locklessly here?

The generic code previously used get_file_active() to safely obtain a
reference. Because kvm_gmem_bind() intentionally drops its reference to the
file on success (via fput(file)), userspace can close the guest_memfd
concurrently while the memslot is being destroyed.

Could this result in a use-after-free when kvm_gmem_get_ops() attempts to
dereference the unrefcounted file pointer?

> +	if (ops && ops->unbind)
> +		ops->unbind(file, kvm, slot);
[ ... ]
> @@ -860,11 +1020,50 @@ static long __kvm_gmem_populate(struct kvm *kvm, struct kvm_memory_slot *slot,
>  	return ret;
>  }
>  
> +static int kvm_gmem_populate_one(const struct kvm_gmem_ops *ops,
> +				      struct file *file, struct kvm *kvm,
> +				      struct kvm_memory_slot *slot, gfn_t gfn,
> +				      struct page *src_page,
> +				      kvm_gmem_populate_cb post_populate,
> +				      void *opaque)
> +{
> +	kvm_pfn_t pfn;
> +	int ret;
> +
> +	/*
> +	 * Resolve the PFN via the ops.  A populate() op may also copy
> +	 * @src_page into the backing before the arch post_populate() step
> +	 * (e.g. SNP LAUNCH_UPDATE); otherwise fall back to get_pfn().
> +	 */
> +	if (ops->populate)
> +		ret = ops->populate(file, kvm, slot, gfn, &pfn,
> +				    src_page, 0);
> +	else {
> +		struct page *ignored_page = NULL;
> +
> +		ret = ops->get_pfn(file, kvm, slot, gfn, &pfn,
> +				   &ignored_page, NULL);
> +		/*
> +		 * Populate does not consume a refcount on the destination page.
> +		 * If get_pfn() handed a refcount back through @page, drop it.
> +		 * Implementations whose PFNs aren't refcounted per-call leave
> +		 * @page NULL and this is a no-op.
> +		 */
> +		if (!ret && ignored_page)
> +			put_page(ignored_page);
> +	}
> +	if (ret)
> +		return ret;
> +
> +	return post_populate(kvm, gfn, pfn, src_page, opaque);
> +}

[Severity: Critical]
Does dropping the page reference before calling post_populate() introduce a
time-of-check to time-of-use race?

If the backend provides a physical page via get_pfn() and we drop the pin
with put_page(ignored_page) early, the page could be concurrently freed and
reallocated to another process.

Could this cause the hardware post_populate callback (such as SNP RMPUPDATE)
to operate on a physical page that now belongs to an innocent host process,
corrupting host memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/f09647f427565c1789a6979470045aab78661563.1784194290.git.dwmw@amazon.co.uk?part=4