Re: [PATCH] KVM: guest_memfd: Elaborate on how release() vs. get_pfn() is safe against UAF

[Yan Zhao]

On Tue, Jun 02, 2026 at 05:40:00PM -0700, Ackerley Tng wrote:
> Yan Zhao <yan.y.zhao@xxxxxxxxx> writes:
> 
> Coming here from https://lore.kernel.org/all/ahn6qysOGfa74A2E@xxxxxxxxxx/.
> 
> > On Thu, Nov 13, 2025 at 03:22:29PM -0800, Sean Christopherson wrote:
> >> Add more context and information to the comment in kvm_gmem_release() that
> >> explains why there's no synchronization on RCU _or_ kvm->srcu.  Point (b)
> >> from commit 67b43038ce14 ("KVM: guest_memfd: Remove RCU-protected attribute
> >> from slot->gmem.file")
> >>
> >>       b) kvm->srcu ensures that kvm_gmem_unbind() and freeing of a memslot
> >>          occur after the memslot is no longer visible to kvm_gmem_get_pfn().
> >>
> >> is especially difficult to fully grok, particularly in light of commit
> >> ae431059e75d ("KVM: guest_memfd: Remove bindings on memslot deletion when
> >> gmem is dying"), which addressed a race between unbind() and release().
> > As mentioned in commit ae431059e75d ("KVM: guest_memfd: Remove bindings on
> > memslot deletion when gmem is dying"), unbind() and release() are mutually
> > exclusive, i.e., both protected by slots_lock, mentioning "race" here is
> > confusing. So, that commit addressed the mishandling in unbind() after
> > kvm_gmem_get_file() returning NULL?
> >
> 
> I agree that the use of the word "race" here is odd since both unbind
> and release() are mutually exclusive and protected by slots_lock.
> 
> Looking at ae431059e75d again, there are really 3 changes:
> 
> 1. Refactoring out __kvm_gmem_unbind(), which is mostly about setting
>    bindings in the xarray to NULL and setting slot->gmem.file to NULL,
> 2. Checking that if slot->gmem.file is NULL, skip unbinding
> 3. Checking that if kvm_gmem_get_file() returns NULL, just unbind
>    without taking filemap_invalidate_lock().
> 
> (2.) is explained in the comment
> 
> 	/*
> 	 * Nothing to do if the underlying file was _already_ closed, as
> 	 * kvm_gmem_release() invalidates and nullifies all bindings.
> 	 */
> 
> The real fix is in (3.), I think? Could you explain how
> kvm_gmem_get_file() works? Why would the file be dying with the
> slots_lock making the unbind and release mutually exclusive?
Below is my understanding:

kvm_gmem_unbind() is invoked under slots_lock.
kvm_gmem_release() holds slots_lock before it accesses any slot and sets
slot->gmem.file to NULL.

When kvm_gmem_get_file() returns NULL, it could be
1) kvm_gmem_release() is to be invoked soon.
2) kvm_gmem_release() is being invoked, before holding slots_lock.
3) kvm_gmem_release() is being invoked, holding slots_lock.
4) kvm_gmem_release() is being invoked, after releasing slots_lock.
5) kvm_gmem_release() has been invoked and completed.

So, when kvm_gmem_unbind() finds slot->gmem.file != NULL while
kvm_gmem_get_file() returns NULL, it could only be 1) or 2). And at that point,
the remaining user of the slot->gmem.file should only be kvm_gmem_release().

For both 1) and 2), kvm_gmem_unbind() and kvm_gmem_release() can't operate on
f->bindings simultaneously. kvm_gmem_unbind() needs to remove the slot from
slot->gmem.file's binding. Otherwise, after kvm_gmem_unbind() returns, the slot
will be deleted and kvm_gmem_release() will later hold slots_lock and cause
use-after-free.