Re: [PATCH v3 1/4] KVM: guest_memfd: Gracefully handle xarray errors when binding a memslot

[David Hildenbrand (Arm)]

On 9/4/26 02:43, Sean Christopherson wrote:
> If inserting a memslot into a guest_memfd's bindings xarray fails,
> propagate the error back to the caller, i.e. fail memslot creation as well.
> Signalling success and continuing on with memslot creation results in
> use-after-free, as the guest_memfd instance will remain reachable via the
> memslot after the file is freed (kvm_gmem_release() won't nullify the file
> pointer due to lack of a valid binding).
> 
> Opportunistically WARN and reject binding if KVM_MEMSLOT_GMEM_ONLY is
> already set, partly to guard against goofs elsewhere, but mostly so that
> KVM doesn't need to worry about clobbering flags when unwinding on failure.
> 
> Fixes: a7800aa80ea4 ("KVM: Add KVM_CREATE_GUEST_MEMFD ioctl() for guest-specific backing memory")
> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: Stefan Teodorescu <fane@xxxxxxxxxx>
> Reported-by: Dennis Tighe <dtighe@xxxxxxxxxx>
> Reported-by: Sashiko Bot <sashiko-bot@xxxxxxxxxx>
> Closes: https://lore.kernel.org/all/20260823135031.4F6DC1F000E9%40smtp.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
>  virt/kvm/guest_memfd.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index b596486d184c..0b48e9a775aa 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -612,10 +612,14 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
>  	struct inode *inode;
>  	struct file *file;
>  	int r = -EINVAL;
> +	void *xar;
>  
>  	BUILD_BUG_ON(sizeof(gpa_t) != sizeof(offset));
>  	BUILD_BUG_ON(sizeof(gfn_t) != sizeof(slot->gmem.pgoff));
>  
> +	if (WARN_ON_ONCE(slot->flags & KVM_MEMSLOT_GMEM_ONLY))
> +		return -EINVAL;
> +
>  	file = fget(fd);
>  	if (!file)
>  		return -EBADF;
> @@ -654,7 +658,15 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
>  	if (kvm_gmem_supports_mmap(inode))
>  		slot->flags |= KVM_MEMSLOT_GMEM_ONLY;
>  
> -	xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL);
> +	xar = xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL);
> +
> +	r = xa_is_err(xar) ? xa_err(xar) : 0;


r = xa_err(xar);

Should be sufficient, right?

mm/memremap.c:pagemap_range() uses that and just avoids the intermediate xar
value completely.

r = xa_err(xa_store_range(...);

> +	if (r) {
> +		xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL);
> +		slot->gmem.file = NULL;
> +		slot->gmem.pgoff = 0;
> +		slot->flags &= ~KVM_MEMSLOT_GMEM_ONLY;
> +	}
>  	filemap_invalidate_unlock(inode->i_mapping);

Apart from that

Reviewed-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>

-- 
Cheers,

David