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

[Yan Zhao]

On Wed, Jun 03, 2026 at 05:10:59PM -0700, Sean Christopherson wrote:
> On Mon, Nov 17, 2025, Yan Zhao wrote:
> > 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?
> 
> Yes, that's the race.  Just because two chunks of code are mutually exclusive
> doesn't mean there's no race, it just changes the granularity of the race, and/or
> how it manifests.
> 
> I consider it a race because in the failing case, there's simply no way for
> release() and unbind() to run sequentially.  I.e. the only way to encounter the
> bug was to run two operations concurrently, *and* for a specific ordering to
> occur.
Thanks for the explanation!

> > > No functional change intended.
> > > 
> > > Cc: Yan Zhao <yan.y.zhao@xxxxxxxxx>
> > > Cc: Vishal Annapurve <vannapurve@xxxxxxxxxx>
> > > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > > ---
> > >  virt/kvm/guest_memfd.c | 20 ++++++++++++++------
> > >  1 file changed, 14 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > > index fdaea3422c30..2e09d7ec0cfc 100644
> > > --- a/virt/kvm/guest_memfd.c
> > > +++ b/virt/kvm/guest_memfd.c
> > > @@ -338,17 +338,25 @@ static int kvm_gmem_release(struct inode *inode, struct file *file)
> > >  	 * dereferencing the slot for existing bindings needs to be protected
> > >  	 * against memslot updates, specifically so that unbind doesn't race
> > >  	 * and free the memslot (kvm_gmem_get_file() will return NULL).
> > > -	 *
> > > -	 * Since .release is called only when the reference count is zero,
> > > -	 * after which file_ref_get() and get_file_active() fail,
> > > -	 * kvm_gmem_get_pfn() cannot be using the file concurrently.
> > > -	 * file_ref_put() provides a full barrier, and get_file_active() the
> > > -	 * matching acquire barrier.
> > >  	 */
> > 
> > >  	mutex_lock(&kvm->slots_lock);
> > >  
> > >  	filemap_invalidate_lock(inode->i_mapping);
> > > 
> > > +	/*
> > > +	 * Note!  synchronize_srcu() is _not_ needed after nullifying memslot
> > > +	 * bindings as slot->gmem.file cannot be set back to a non-null value
> > > +	 * without the memslot first being deleted.  I.e. this relies on the
> > > +	 * synchronize_srcu_expedited() in kvm_swap_active_memslots() to ensure
> > > +	 * kvm_gmem_get_pfn() (which runs with kvm->srcu held for read) can't
> > > +	 * grab a reference to slot->gmem.file even if the struct file object
> > > +	 * is reallocated.
> > Do you mean that as kvm_gmem_get_pfn() can't find a stale slot, it can't grab
> > reference to stale slot->gmem.file, even if slot->gmem.file has been set to a
> > different value, i.e., after invoking unbind(), bind() ?
> 
> I was specifically trying to say this:
> 
>  1. slot->gmem.file == old  ==
>     synchronize_srcu()       |===> window #1 for readers
>  2. slot->gmem.file == NULL ==
>     synchronize_srcu()       |===> window #2 for readers
>  3. slot->gmem.file == new  ==
> 
> kvm_gmem_get_pfn() can run in two "windows".  In the window #1, it can see @old
> or NULL.  In window #2, it can see NULL or @new.  The synchronization ensures
> it can't grab a reference to @old once window #2 has been opened.
Thanks for the clarification.

> > But I'm not sure why to put the kvm_gmem_get_pfn() relying on
> > synchronize_srcu_expedited() in kvm_swap_active_memslots() in the comment of
> > release().  Without the guard of kvm_gmem_get_file(), kvm_gmem_get_pfn() may
> > need to provide some RCU read-critial section too for release() to wait by
> > synchronize_srcu()?
> 
> No, what I'm saying is that without synchronize_srcu() to create distinct
> window #1 and window #2 above, KVM would need this:
> 
> diff --git virt/kvm/guest_memfd.c virt/kvm/guest_memfd.c
> index a1cb72e66288..e66e431bdb98 100644
> --- virt/kvm/guest_memfd.c
> +++ virt/kvm/guest_memfd.c
> @@ -347,6 +347,8 @@ static int kvm_gmem_release(struct inode *inode, struct file *file)
>         xa_for_each(&f->bindings, index, slot)
>                 WRITE_ONCE(slot->gmem.file, NULL);
>  
> +       synchronize_srcu();
> +
>         /*
>          * All in-flight operations are gone and new bindings can be created.
>          * Zap all SPTEs pointed at by this file.  Do not free the backing
> 
> Otherwise there is no guarantee in-flight readers can't reach the old
> slot->gmem.file.
Hmm, however, if the reader (kvm_gmem_get_pfn()) has grabbed a reference to @old,
kvm_gmem_release() can't come; if kvm_gmem_release() comes, kvm_gmem_get_pfn()
should not have been able to successfully invoke get_file_active() on @old.
After kvm_gmem_release() returns, any subsequent calls to get_file_active() on
@old should fail and return NULL.

So, even if the reader can see @old, we're still fine without synchronize_srcu(),
as get_file_active() and SLAB_TYPESAFE_BY_RCU should have provide the safety.

Am I missing something?