On Fri, Dec 18, 2020, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@xxxxxxxxxx> writes:
>
> > Return -1 from the get_walk() helpers if the shadow walk doesn't fill at
> > least one spte, which can theoretically happen if the walk hits a
> > not-present PTPDR. Returning the root level in such a case will cause
>
> PDPTR
Doh.
> > get_mmio_spte() to return garbage (uninitialized stack data). In
> > practice, such a scenario should be impossible as KVM shouldn't get a
> > reserved-bit page fault with a not-present PDPTR.
> >
> > Note, using mmu->root_level in get_walk() is wrong for other reasons,
> > too, but that's now a moot point.
> >
> > Fixes: 95fb5b0258b7 ("kvm: x86/mmu: Support MMIO in the TDP MMU")
> > Cc: Ben Gardon <bgardon@xxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > ---
> > arch/x86/kvm/mmu/mmu.c | 7 ++++++-
> > arch/x86/kvm/mmu/tdp_mmu.c | 2 +-
> > 2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index 7a6ae9e90bd7..a48cd12c01d7 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -3488,7 +3488,7 @@ static bool mmio_info_in_cache(struct kvm_vcpu *vcpu, u64 addr, bool direct)
> > static int get_walk(struct kvm_vcpu *vcpu, u64 addr, u64 *sptes)
> > {
> > struct kvm_shadow_walk_iterator iterator;
> > - int leaf = vcpu->arch.mmu->root_level;
> > + int leaf = -1;
> > u64 spte;
> >
> >
> > @@ -3532,6 +3532,11 @@ static bool get_mmio_spte(struct kvm_vcpu *vcpu, u64 addr, u64 *sptep)
> > else
> > leaf = get_walk(vcpu, addr, sptes);
> >
> > + if (unlikely(leaf < 0)) {
> > + *sptep = 0ull;
> > + return reserved;
> > + }
>
> When SPTE=0 is returned from get_mmio_spte(), handle_mmio_page_fault()
> will return RET_PF_RETRY -- should it be RET_PF_INVALID instead?
No, RET_PF_RETRY is the most appropriate. A pae_root entry will only be zero if
the corresponding guest PDPTR is !PRESENT, i.e. the page fault is effectively in
the guest context. The reason I say it should be an impossible condition is
because KVM should also reset the MMU whenever it snapshots the guest's PDPTRs,
i.e. it should be impossible to install a MMIO SPTE if the relevant PDPTR is
!PRESENT, and all MMIO SPTEs should be wiped out if the PDPTRs are reloaded.
I suppose by that argument, this should be a WARN_ON_ONCE, but I'm not sure if
I'm _that_ confident in my analysis :-)
Related side topic, this snippet in get_mmio_spte() is dead code, as the same
check is performed by its sole caller. I'll send a patch to remove it (unless
Paolo wants a v2 of this series, in which case I'll tack it on the end).
if (!VALID_PAGE(vcpu->arch.mmu->root_hpa)) {
*sptep = 0ull;
return reserved;
}
