linux-kernel - Re: [PATCH 16/54] KVM: x86/mmu: Drop smep_andnot

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YNONVux/a+/fuw0I@google.com>
Date:   Wed, 23 Jun 2021 19:36:54 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Yu Zhang <yu.c.zhang@...ux.intel.com>,
        Maxim Levitsky <mlevitsk@...hat.com>
Subject: Re: [PATCH 16/54] KVM: x86/mmu: Drop smep_andnot_wp check from "uses
 NX" for shadow MMUs

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@...gle.com>
> > ---
> >   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.