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

[Yan Zhao <yan.y.zhao@intel.com>]

On Mon, Jun 08, 2026 at 06:41:23PM -0700, Sean Christopherson wrote:
> 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.
Ok. Thanks for the explanation. I get your point now.

My previous confusion came from the fact that if kvm_set_memory_region() does
not wait for all users of the slot being deleted to exit the SRCU read critical
section before committing the slot deletion, KVM would be wildly broken
regardless of whether gmem is released.

> 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.
If slot->gmem.file can be set back to a non-null value without the memslot first
being deleted, synchronize_srcu() in release() isn't required, right?

  CPU0                                    CPU1
  kvm_gmem_get_pfn()                     
    f = X (from slot->gmem.file)

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

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

   <KVM proceeds here should be fine with slot + new file>