by Sean Christopherson

[permalink] [raw]

Subject: Re: [PATCH v2 4/7] KVM: x86/mmu: Zap only obsolete roots if a root shadow page is zapped

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/[email protected]