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

[Yan Zhao]

On Wed, Jun 03, 2026 at 08:11:26AM -0700, Ackerley Tng wrote:
> Yan Zhao <yan.y.zhao@xxxxxxxxx> writes:
> 
> > 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:
> >
> 
> Thanks for explaining!
> 
> > 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
> 
> To clarify, 3, 4, 5 are not possible because kvm_gmem_unbind() is called
> with slots_lock held, right?
Yes.

> > 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.
> 
> Thanks, this makes sense. The purpose of using kvm_gmem_get_file() is
> that if the file is _being_ closed, as stated in the comment in
> kvm_gmem_unbind(), kvm_gmem_get_file() would find the refcount to be 0
> and return NULL. (The missing part for me was that _being_ closed leads
> to kvm_gmem_get_file() finding a 0 refcount.)
> 
> 
> Annotating all of these:
LGTM.