Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 3 Mar 2022 23:00:28 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Paolo Bonzini <pbonzini@redhat.com>
Cc:     Christian Borntraeger <borntraeger@linux.ibm.com>,
        Janosch Frank <frankja@linux.ibm.com>,
        David Hildenbrand <david@redhat.com>,
        Claudio Imbrenda <imbrenda@linux.ibm.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Ben Gardon <bgardon@google.com>,
        Lai Jiangshan <jiangshanlai@gmail.com>
Subject: Re: [PATCH v2 4/7] KVM: x86/mmu: Zap only obsolete roots if a root
 shadow page is zapped
Message-ID: <YiFIjNVPLVVnRatm@google.com>
References: <20220225182248.3812651-1-seanjc@google.com>
 <20220225182248.3812651-5-seanjc@google.com>
 <40a22c39-9da4-6c37-8ad0-b33970e35a2b@redhat.com>
 <ee757515-4a0f-c5cb-cd57-04983f62f499@redhat.com>
 <Yh/JdHphCLOm4evG@google.com>
 <217cc048-8ca7-2b7b-141f-f44f0d95eec5@redhat.com>
 <Yh/1hPMhqeFKO0ih@google.com>
 <0c22b156-10c5-1988-7256-a9db7871989d@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <0c22b156-10c5-1988-7256-a9db7871989d@redhat.com>
Precedence: bulk

On Thu, Mar 03, 2022, Paolo Bonzini wrote:
> On 3/2/22 23:53, Sean Christopherson wrote:
> > > 
> > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > > index c5e3f219803e..7899ca4748c7 100644
> > > --- a/arch/x86/kvm/svm/svm.c
> > > +++ b/arch/x86/kvm/svm/svm.c
> > > @@ -3857,6 +3857,9 @@ static void svm_load_mmu_pgd(struct kvm_vcpu *vcpu,
> > > hpa_t root_hpa,
> > >          unsigned long cr3;
> > > 
> > >          if (npt_enabled) {
> > > +               if (is_tdp_mmu_root(root_hpa))
> > > +                       svm->current_vmcb->asid_generation = 0;
> > > +
> > >                  svm->vmcb->control.nested_cr3 = __sme_set(root_hpa);
> > >                  vmcb_mark_dirty(svm->vmcb, VMCB_NPT);
> > > 
> > > Why not just new_asid
> > My mental coin flip came up tails?  new_asid() is definitely more intuitive.
> > 
> 
> Can you submit a patch (seems like 5.17+stable material)?

After a lot more thinking, there's no bug.  If KVM unloads all roots, e.g. fast zap,
then all vCPUs are guaranteed to go through kvm_mmu_load(), and that will flush the
current ASID.

So the only problematic path is KVM_REQ_LOAD_MMU_PGD, which has two users,
kvm_mmu_new_pgd() and load_pdptrs().  load_pdptrs() is benign because it triggers
a "false" PGD load only top get PDPTRs updated on EPT, the actual PGD doesn't change
(or rather isn't forced to change by load_pdptrs().

Nested SVM's use of kvm_mmu_new_pgd() is "ok" because KVM currently flushes on
every transition.

That leaves kvm_set_cr3() via kvm_mmu_new_pgd().  For NPT, lack of a flush is
moot because KVM shouldn't be loading a new PGD in the first place (see our other
discussion about doing:

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index cf17af4d6904..f11199b41ca8 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1212,7 +1212,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
        if (is_pae_paging(vcpu) && !load_pdptrs(vcpu, cr3))
                return 1;

-       if (cr3 != kvm_read_cr3(vcpu))
+       if (!tdp_enabled && cr3 != kvm_read_cr3(vcpu))
                kvm_mmu_new_pgd(vcpu, cr3);

        vcpu->arch.cr3 = cr3;


Non-NPT shadow paging is ok because either the MOV CR3 will do a TLB flush, or the
guest explicitly says "don't do a TLB flush", in which case KVM is off the hook
from a correctness perspective (guest's responsibility to ensure MMU in sync'd),
and is ok from a safety perspective because the legacy MMU does a remote TLB flush
if it zaps any pages, i.e. the guest can't do use-after-free.

All that said, this is another argument against dropping kvm_mmu_unload() from
kvm_mmu_reset_context()[*]: SMM would theoretically be broken on AMD due to reusing
the same ASID for both non-SMM and SMM roots/memslots.

In practice, I don't think it can actually happen, but that's mostly dumb luck.
em_rsm() temporarily transitions back to Real Mode before loading the actual
non-SMM guest state, so only SMI that arrives with CR0.PG=0 is problematic.  In
that case, TLB flushes may not be triggered by kvm_set_cr0() or kvm_set_cr4(),
but kvm_set_cr3() will always trigger a flush because the "no flush" PCID bit
will always be clear.  Well, unless the SMM handler writes the read-only SMRAM
field, at which point it deserves to die :-)

Anyways, before we transitions SMM away from kvm_mmu_reset_context(), we should
add an explicit KVM_REQ_TLB_FLUSH_CURRENT in svm_{enter,leave}_smm(), with a TODO
similar to nested_svm_transition_tlb_flush() to document that the explicit flush
can go away when KVM ensures unique ASIDs for non-SMM vs. SMM.

[*] https://lore.kernel.org/all/20220209170020.1775368-13-pbonzini@redhat.com