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 <[email protected]> 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.d...@amazon.co.uk?part=4
