| Message ID | 20201218003139.2167891-2-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86/mmu: Bug fixes and cleanups in get_mmio_spte() | expand |
Sean Christopherson <seanjc@google.com> 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 > 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@google.com> > Cc: stable@vger.kernel.org > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > 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? > + > rsvd_check = &vcpu->arch.mmu->shadow_zero_check; > > for (level = root; level >= leaf; level--) { > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c > index 84c8f06bec26..50cec7a15ddb 100644 > --- a/arch/x86/kvm/mmu/tdp_mmu.c > +++ b/arch/x86/kvm/mmu/tdp_mmu.c > @@ -1152,8 +1152,8 @@ int kvm_tdp_mmu_get_walk(struct kvm_vcpu *vcpu, u64 addr, u64 *sptes) > { > struct tdp_iter iter; > struct kvm_mmu *mmu = vcpu->arch.mmu; > - int leaf = vcpu->arch.mmu->shadow_root_level; > gfn_t gfn = addr >> PAGE_SHIFT; > + int leaf = -1; > > tdp_mmu_for_each_pte(iter, mmu, gfn, gfn + 1) { > leaf = iter.level;
On Fri, Dec 18, 2020, Vitaly Kuznetsov wrote: > Sean Christopherson <seanjc@google.com> 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@google.com> > > Cc: stable@vger.kernel.org > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > 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; }
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; + } + rsvd_check = &vcpu->arch.mmu->shadow_zero_check; for (level = root; level >= leaf; level--) { diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index 84c8f06bec26..50cec7a15ddb 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -1152,8 +1152,8 @@ int kvm_tdp_mmu_get_walk(struct kvm_vcpu *vcpu, u64 addr, u64 *sptes) { struct tdp_iter iter; struct kvm_mmu *mmu = vcpu->arch.mmu; - int leaf = vcpu->arch.mmu->shadow_root_level; gfn_t gfn = addr >> PAGE_SHIFT; + int leaf = -1; tdp_mmu_for_each_pte(iter, mmu, gfn, gfn + 1) { leaf = iter.level;
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 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@google.com> Cc: stable@vger.kernel.org Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/mmu/mmu.c | 7 ++++++- arch/x86/kvm/mmu/tdp_mmu.c | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-)