All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Yan Zhao <yan.y.zhao@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	 Vishal Annapurve <vannapurve@google.com>
Subject: Re: [PATCH] KVM: guest_memfd: Elaborate on how release() vs. get_pfn() is safe against UAF
Date: Tue, 9 Jun 2026 10:25:22 -0700	[thread overview]
Message-ID: <aihMgs0Zj4kS7YWv@google.com> (raw)
In-Reply-To: <aifPUFncRYYpURL1@yzhao56-desk.sh.intel.com>

On Tue, Jun 09, 2026, Yan Zhao wrote:
> On Mon, Jun 08, 2026 at 06:41:23PM -0700, Sean Christopherson wrote:
> > On Fri, Jun 05, 2026, Yan Zhao wrote:
> > 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> 

Not really?  There are no guarantees that the old slot and new slot have the same
GPA range and the same gfn=>gmem bindings.  E.g. __kvm_gmem_get_pfn() could get
invoked with an out-of-range index, or worse could provide the wrong pfn due to
computing .

It's kind of an impossible question to answer though, because it's sooo far down
a hypothetical path where a KVM bug basically means all bets are off.

  reply	other threads:[~2026-06-09 17:25 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-11-13 23:22 [PATCH] KVM: guest_memfd: Elaborate on how release() vs. get_pfn() is safe against UAF Sean Christopherson
2025-11-17 11:36 ` Yan Zhao
2026-06-03  0:40   ` Ackerley Tng
2026-06-03 11:42     ` Yan Zhao
2026-06-03 15:11       ` Ackerley Tng
2026-06-05  8:52         ` Yan Zhao
2026-06-04  0:10   ` Sean Christopherson
2026-06-05  8:32     ` Yan Zhao
2026-06-09  1:41       ` Sean Christopherson
2026-06-09  8:31         ` Yan Zhao
2026-06-09 17:25           ` Sean Christopherson [this message]
2026-06-03 15:37 ` Ackerley Tng
2026-06-03 23:29   ` Sean Christopherson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aihMgs0Zj4kS7YWv@google.com \
    --to=seanjc@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=vannapurve@google.com \
    --cc=yan.y.zhao@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.