Re: [RFC PATCH 1/3] KVM: guest_memfd: Use memslot id to keep track of associated memslots

[Alexandru Elisei <alexandru.elisei@arm.com>]

On Thu, Jul 02, 2026 at 02:47:57PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] Stale SPTEs are left mapped during concurrent memslot deletion, causing a Use-After-Free when backing folios are freed.
> - [Low] Silent suppression of kernel warnings for memslot 0 mismatches.
> --
> 
> commit ca8b0f095e48299dd0d910de62ee16780d361eec
> Author: Alexandru Elisei <alexandru.elisei@arm.com>
> 
> KVM: guest_memfd: Use memslot id to keep track of associated memslots
>     
> To enable memslot operations, KVM maintains two arrays of memslots, and an
> RCU pointer to the active (in use) array. Changes are made first to the
> inactive array, and the RCU pointer is updated to point to the inactive
> array, which becomes active.
> 
> > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > index db57c5766ab60..43ef8e908aaf6 100644
> > --- a/virt/kvm/guest_memfd.c
> > +++ b/virt/kvm/guest_memfd.c
> 
> [ ... ]
> 
> > @@ -165,9 +189,15 @@ static void __kvm_gmem_invalidate_start(struct gmem_file *f, pgoff_t start,
> >  	struct kvm_memory_slot *slot;
> >  	struct kvm *kvm = f->kvm;
> >  	unsigned long index;
> > +	void *entry;
> > +
> > +	xa_for_each_range(&f->bindings, index, entry, start, end - 1) {
> > +		pgoff_t pgoff;
> >  
> > -	xa_for_each_range(&f->bindings, index, slot, start, end - 1) {
> > -		pgoff_t pgoff = slot->gmem.pgoff;
> > +		slot = xa_value_to_memslot(kvm, entry);
> > +		if (!slot)
> > +			continue;
> > +		pgoff = slot->gmem.pgoff;
> 
> [Severity: Critical]
> Could this leave stale SPTEs mapped during concurrent memslot deletion,
> leading to a Use-After-Free?
> 
> If a guest_memfd hole punch executes concurrently while a KVM_MR_DELETE
> operation is waiting in synchronize_srcu(), xa_value_to_memslot() will return
> the invalid_slot. Because kvm_copy_memslot() does not copy the conditionally
> compiled gmem struct, invalid_slot->gmem.pgoff is 0.
> 
> When __kvm_gmem_invalidate_start() calculates the gfn_range using a 0 pgoff,
> it can result in start >= end, causing kvm_mmu_unmap_gfn_range() to skip
> zapping the SPTEs. When the backing folios are then freed by
> truncate_inode_pages_range(), the guest could access freed host memory.

I think that's a genuine bug. I could teach kvm_copy_memslot() about
guest_memfd, and also copy slot->gmem.

> 
> [ ... ]
> 
> > @@ -765,8 +830,8 @@ static struct folio *__kvm_gmem_get_pfn(struct file *file,
> >  		return ERR_PTR(-EFAULT);
> >  	}
> >  
> > -	if (xa_load(&f->bindings, index) != slot) {
> > -		WARN_ON_ONCE(xa_load(&f->bindings, index));
> > +	if (xa_load(&f->bindings, index) != memslot_to_xa_value(slot)) {
> > +		WARN_ON_ONCE(xa_to_value(xa_load(&f->bindings, index)));
> 
> [Severity: Low]
> Will this suppress the intended warning when the corruption involves memslot 0?
> 
> If the xarray state erroneously points to memslot 0 (as_id=0, id=0),
> xa_load() returns xa_mk_value(0). xa_to_value() unwraps this to 0, causing
> WARN_ON_ONCE(0) to evaluate to false and fail to warn.
> 
> (This logic would also similarly suppress the warning in kvm_gmem_change_flags()
> when there is a structural state inconsistency involving memslot 0).

I can replace the WARN_ON_ONCE with WARN_ONCE().

Thanks,
Alex

> 
> >  		return ERR_PTR(-EIO);
> >  	}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260702142912.6395-1-alexandru.elisei@arm.com?part=1