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

[Sean Christopherson <seanjc@google.com>]

On Tue, Sep 22, 2026, Yan Zhao wrote:
> On Mon, Sep 21, 2026 at 06:40:15AM -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.
> Does "reallocated" here refer to the case (*) below?

Yes.  I'll clarify this with:

		 * is reallocated (for use as a different file).

> > > > +	 * file_ref_put() provides a full barrier, and __get_file_rcu() the
> > > > +	 * matching acquire barrier, to ensure that kvm_gmem_get_file() (via
> > > > +	 * __get_file_rcu()) sees refcount==0 or fails the "file reloaded"
> > > > +	 * check (file != NULL due to nullifying the file pointer here).
> > > > +	 *
> > > > +	 * Unlike most other users of get_file_rcu(), where callers don't care
> > > > +	 * if they race with a write, only that they have a reference to _a_
> > > > +	 * live file, kvm_gmem_get_pfn() needs to get the exact file that is
> > > > +	 * associated with the memslot.  Without the aforementioned SRCU
> > > > +	 * synchronization, the following could happen:
> > > > +	 *
> > > > +	 *  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)
> Case (*):
> 
> I thought "reallocated" refers to this case, which get_file_active() cannot
> guarantee against.

Ya.

> > > > +	 *
> > > > +	 *  get_file_active()
> > > > +	 *    file = f
> > > > +	 *    file_reloaded = f
> > > > +	 *
> > > > +	 * <KVM does weird things with an old memslot+file>
> > > > +	 *
> > > > +	 * Obviously KVM would be broken in many places if the synchronization
> > > > +	 * were omitted, but it's important to note that get_file_active() does
> > > > +	 * NOT guarantee a reference to the correct file was obtained, only
> > > > +	 * that the file doesn't point at a reallocated object.
> > > reallocated -> reloaded?
> > 
> > No, "reallocated" is correct.  From the comment for SLAB_TYPESAFE_BY_RCU:
> > 
> >   * This delays freeing the SLAB page by a grace period, it does _NOT_
> >   * delay object freeing. This means that if you do kmem_cache_free()
> >   * that memory location is free to be reused at any time. Thus it may
> >   * be possible to see another object there in the same RCU grace period.
> >   *
> >   * This feature only ensures the memory location backing the object
> >   * stays valid, the trick to using this is relying on an independent
> >   * object validation pass. Something like:
> >   *
> >   * ::
> >   *
> >   *  begin:
> >   *   rcu_read_lock();
> >   *   obj = lockless_lookup(key);
> >   *   if (obj) {
> >   *     if (!try_get_ref(obj)) // might fail for free objects
> >   *       rcu_read_unlock();
> >   *       goto begin;
> >   *
> >   *     if (obj->key != key) { // not the object we expected
> >   *       put_ref(obj);
> >   *       rcu_read_unlock();
> >   *       goto begin;
> >   *     }
> >   *   }
> >   *  rcu_read_unlock();
> > 
> > The above pseudocode is what get_file_active() is doing; it verifies the found
> > file ("obj" above) is the correct file.  Specifically, from __get_file_rcu():
> > 
> > 	 * If the pointers don't match the file has been reallocated by
> > 	 * SLAB_TYPESAFE_BY_RCU.
> Yeah, I understand this is what you mean by "reallocated" :)
> However, I think case (*) above could also be considered a form of "reallocated".
> Would it make sense to rename "reallocated" to "reassigned" in case (*)?

Oooh, I see what you're getting at.  Yes, I agree, "reallocated" isn't the best
description here.  Hmm, but neither is "reloaded" or "reassigned", because it
doesn't fully guard against the pointer being reassigned, e.g. if the pointer is
reassigned before the the first load.

How about this?

	 * Obviously KVM would be broken in many places if the synchronization
	 * were omitted, but it's important to note that get_file_active() does
	 * NOT guarantee a reference to the "original" file was obtained, only
	 * that it grabbed a reference for the returned file, i.e. didn't grab
	 * a reference for file A, but then returned a pointer to file B.

And the whole thing as a diff:

diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index 9a112daa5df9..483998f0df58 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -312,7 +312,7 @@ static int kvm_gmem_release(struct inode *inode, struct file *file)
 	 * 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.
+	 * is reallocated (for use as a different file).
 	 *
 	 * file_ref_put() provides a full barrier, and __get_file_rcu() the
 	 * matching acquire barrier, to ensure that kvm_gmem_get_file() (via
@@ -327,7 +327,7 @@ static int kvm_gmem_release(struct inode *inode, struct file *file)
 	 *
 	 *  CPU0				CPU1
 	 *  kvm_gmem_get_pfn()
-	 *    f = X (from slot->gmem.file)
+	 *    slot->gmem.file == A
 	 *					kvm_gmem_release())
 	 *					  slot->gmem.file = NULL
 	 *
@@ -336,18 +336,19 @@ static int kvm_gmem_release(struct inode *inode, struct file *file)
 	 *
 	 *					kvm_set_memory_region()
 	 *					  slot created
-	 *					  slot->gmem.file = f (alloc the same object)
+	 *					  slot->gmem.file = B (alloc the same object)
 	 *
 	 *  get_file_active()
-	 *    file = f
-	 *    file_reloaded = f
+	 *    file = B
+	 *    file_reloaded = B
 	 *
 	 * <KVM does weird things with an old memslot+file>
 	 *
 	 * Obviously KVM would be broken in many places if the synchronization
 	 * were omitted, but it's important to note that get_file_active() does
-	 * NOT guarantee a reference to the correct file was obtained, only
-	 * that the file doesn't point at a reallocated object.
+	 * NOT guarantee a reference to the "original" file was obtained, only
+	 * that it grabbed a reference for the returned file, i.e. didn't grab
+	 * a reference for file A, but then returned a pointer to file B.
 	 */
 	xa_for_each(&f->bindings, index, slot)
 		WRITE_ONCE(slot->gmem.file, NULL);