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

[sashiko-bot@kernel.org]

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.

[ ... ]

> @@ -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).

>  		return ERR_PTR(-EIO);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702142912.6395-1-alexandru.elisei@arm.com?part=1