Re: [PATCH 4/4] KVM: x86/mmu: Add sanity check to detect stale page faults in "map private PFN"

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

On Mon, Aug 17, 2026 at 06:43:23AM -0700, Sean Christopherson wrote:
> On Mon, Aug 17, 2026, Yan Zhao wrote:
> > Sorry for the late reply!
> > 
> > On Tue, Aug 11, 2026 at 10:12:08AM -0700, Sean Christopherson wrote:
> > > On Tue, Aug 11, 2026, Yan Zhao wrote:
> > > > On Thu, Aug 06, 2026 at 02:40:50PM -0700, Sean Christopherson wrote:
> > > > > Harden the "map private PFN" flow against potentially-fatal bugs or future
> > > > > KVM changes by checking for a stale "fault" prior to actually mapping the
> > > > > PFN into the guest.  While it should be impossible for the "page fault" to
> > > > > become stale, the sanity check is cheap, whereas a broken assumption would
> > > > > have a high probability of leading to a guest-expoitable use-after-free.
> > > > > 
> > > > > Snapshot the invalidation sequence after acquiring mmu_lock to avoid false
> > > > > positives, even though doing so completely voids anys and all protection
> > > > > against unexpected invalidations.  Pretty much the entire point of
> > > > > kvm_tdp_mmu_map_private_pfn() is that it allows mapping a PFN that was
> > > > > gifted by the caller, i.e. the caller would have to mess up its one and
> > > > > only responsibility.
> > > > > 
> > > > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > > > > ---
> > > > >  arch/x86/kvm/mmu/mmu.c | 10 ++++++++++
> > > > >  1 file changed, 10 insertions(+)
> > > > > 
> > > > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > > > > index 379f570ef04f..76e3cd717324 100644
> > > > > --- a/arch/x86/kvm/mmu/mmu.c
> > > > > +++ b/arch/x86/kvm/mmu/mmu.c
> > > > > @@ -5210,6 +5210,16 @@ int kvm_tdp_mmu_map_private_pfn(struct kvm_vcpu *vcpu, gfn_t gfn, kvm_pfn_t pfn)
> > > > >  		 */
> > > > >  		WARN_ON_ONCE(kvm_test_request(KVM_REQ_MMU_FREE_OBSOLETE_ROOTS, vcpu));
> > > > >  
> > > > > +		/*
> > > > > +		 * Snapshot the invalidation sequence counter after acquiring
> > > > > +		 * mmu_lock, as guest_memfd guarantees the validity of the pfn,
> > > > > +		 * i.e. any concurrent invalidations are guaranteed to be
> > > > > +		 * irrelevant.
> > > > > +		 */
> > > > > +		fault.mmu_seq = vcpu->kvm->mmu_invalidate_seq;
> > > > Could you explain more about the conditions under which a fault is stale while
> > > > guest_memfd guarantees the validity of the pfn?
> > > > 
> > > > Given that kvm_tdp_mmu_map_private_pfn() already asserts holding slots_lock and
> > > > invalidate_lock, I can't think of one. 
> > > 
> > > A non-guest_memfd mmu_notifier invalidation bumps mmu_invalidate_seq.  And because
> > > the range-based invalidation checks are deliberately coarse, in-flight invalidations
> > > could also trigger a false positive if GFNs N and N+2 are being invalidate, while
> > > kvm_tdp_mmu_map_private_pfn() is trying to map N+1.
> > Could updating fault.mmu_seq in each iteration cause a false negative?
> 
> Not really.  I mean, yes, technically it could, but I addressed this in changelog.
> 
>  : Snapshot the invalidation sequence after acquiring mmu_lock to avoid false
>  : positives, even though doing so completely voids anys and all protection
>  : against unexpected invalidations.  Pretty much the entire point of
>  : kvm_tdp_mmu_map_private_pfn() is that it allows mapping a PFN that was
>  : gifted by the caller, i.e. the caller would have to mess up its one and
>  : only responsibility.

Thanks for the explanation. I wasn't able to gather that meaning from this
paragraph before :)

> > I may be wrong, as I am unaware of the specific bugs or future changes you
> > mentioned below in 'defense-in-depth against bugs and against future changes'.
> > Could you elaborate a little more on those bugs or future changes?
> 
> There are non, AFAIK.  It's a statement saying "things may change at some point
> in the future".
I see.

> > > > If we add the sanity check because it's cheap, why don't we save
> > > > fault.mmu_seq before getting the pfn to guard against stale pfn as well?
> > > 
> > > Because as the changelog says, the whole point of this API is to provide a stable
> > > PFN.  I want to add an is_page_fault_stale() check as defense-in-depth against bugs,
> > Ok. So adding is_page_fault_stale() is not intended to ensure that a private PFN
> > is stable (i.e., not stale).
> > Any background on why we cannot update the code when the bug/future changes
> > actually arise? :)
> 
> Because finding bugs of this nature after they've been shipped to production isn't
> very fun.
Ok.