Re: [PATCH 4/4] KVM: x86/mmu: Add sanity check to detect stale page faults in "map private PFN"
Yan Zhao <[email protected]>
| Newsgroups | org.kernel.vger.kvm,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <anpr/[email protected]> |
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 <[email protected]> > --- > 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. 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? > + if (is_page_fault_stale(vcpu, &fault)) > + continue; > + > r = kvm_tdp_mmu_map(vcpu, &fault); > } while (r == RET_PF_RETRY); >