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

[Yan Zhao]

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?

> > > +	 * 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.

> > > +	 *
> > > +	 *  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 (*)?