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

[Sean Christopherson]

On Fri, Jun 05, 2026, Yan Zhao wrote:
> On Wed, Jun 03, 2026 at 05:10:59PM -0700, Sean Christopherson wrote:
> > > > +	/*
> > > > +	 * 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? 

In kvm_gmem_get_pfn(), because slots_lock isn't held, this could happen if
kvm_set_memory_region() didn't synchronize_srcu().
 
  CPU0                                    CPU1
  kvm_gmem_get_pfn()                     
    f = X (from slot->gmem.file)

                                          kvm_gmem_release())
                                            slot->gmem.file = NULL

                                          kvm_set_memory_region()
                                            slot deleted

                                          kvm_set_memory_region()
                                            slot created
                                            slot->gmem.file = f (alloc the same object)

  get_file_active()
    file = f
    file_reloaded = f

  <KVM does weird things with an old memslot+file> 

Obviously KVM would be wildly broken for other reasons, but just saying
get_file_active() takes care of everything is misleading because it only handles
reallocation of the object, it doesn't guarantee a reference to the correct file
was obtained.

E.g. AFAICT, users of get_mm_exe_file() don't care if they race with
replace_mm_exe_file(), they only care that they have a reference to a live file.

KVM on the other hand cares that kvm_gmem_get_pfn() gets the exact file that is
associated with its memslot point.