Re: [PATCH 16/54] KVM: x86/mmu: Drop smep_andnot_wp check from "uses NX" for shadow MMUs

From: Sean Christopherson
Date: Wed Jun 23 2021 - 15:37:13 EST


On Wed, Jun 23, 2021, Paolo Bonzini wrote:
> On 22/06/21 19:57, Sean Christopherson wrote:
> > Drop the smep_andnot_wp role check from the "uses NX" calculation now
> > that all non-nested shadow MMUs treat NX as used via the !TDP check.
> >
> > The shadow MMU for nested NPT, which shares the helper, does not need to
> > deal with SMEP (or WP) as NPT walks are always "user" accesses and WP is
> > explicitly noted as being ignored:
> >
> > Table walks for guest page tables are always treated as user writes at
> > the nested page table level.
> >
> > A table walk for the guest page itself is always treated as a user
> > access at the nested page table level
> >
> > The host hCR0.WP bit is ignored under nested paging.
> >
> > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > ---
> > arch/x86/kvm/mmu/mmu.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index 96c16a6e0044..ca7680d1ea24 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -4223,8 +4223,7 @@ reset_shadow_zero_bits_mask(struct kvm_vcpu *vcpu, struct kvm_mmu *context)
> > * NX can be used by any non-nested shadow MMU to avoid having to reset
> > * MMU contexts. Note, KVM forces EFER.NX=1 when TDP is disabled.
> > */
> > - bool uses_nx = context->nx || !tdp_enabled ||
> > - context->mmu_role.base.smep_andnot_wp;
> > + bool uses_nx = context->nx || !tdp_enabled;
> > struct rsvd_bits_validate *shadow_zero_check;
> > int i;
> >
>
> Good idea, but why not squash it into patch 2?

Because that patch is marked for stable and dropping the smep_andnot_wp is not
necessary to fix the bug. At worst, the too-liberal uses_nx will suppress the
WARN in handle_mmio_page_fault() because this is for checking KVM's SPTEs, not
the guest's SPTEs, i.e. KVM won't miss a guest reserved NX #PF.

That said, I'm not at all opposed to squashing this. I have a feeling I originally
split the patches because I wasn't super confident about either change, and never
revisited them.