linux-kernel - Re: [PATCH 08/12] KVM: nSVM: Use KVM-governed feature framework to track "vVM{SAVE,LOAD} enabled"

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: <Y/ji6MAlEmbNfZzf@google.com>
Date:   Fri, 24 Feb 2023 08:16:40 -0800
From:   Sean Christopherson <seanjc@...gle.com>
To:     Yu Zhang <yu.c.zhang@...ux.intel.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Maxim Levitsky <mlevitsk@...hat.com>
Subject: Re: [PATCH 08/12] KVM: nSVM: Use KVM-governed feature framework to
 track "vVM{SAVE,LOAD} enabled"

On Fri, Feb 24, 2023, Yu Zhang wrote:
> On Wed, Feb 22, 2023 at 08:39:01AM -0800, Sean Christopherson wrote:
> > +Maxim
> > 
> > On Wed, Feb 22, 2023, Yu Zhang wrote:
> > > On Tue, Feb 21, 2023 at 03:48:07PM -0800, Sean Christopherson wrote:
> > Nope, my interpretation is wrong.  vmload_vmsave_interception() clears the upper
> > bits of SYSENTER_{EIP,ESP}
> > 
> > 	if (vmload) {
> > 		svm_copy_vmloadsave_state(svm->vmcb, vmcb12);
> > 		svm->sysenter_eip_hi = 0;
> > 		svm->sysenter_esp_hi = 0;
> > 	} else {
> > 		svm_copy_vmloadsave_state(vmcb12, svm->vmcb);
> > 	}
> > 
> > From commit adc2a23734ac ("KVM: nSVM: improve SYSENTER emulation on AMD"):
> >     
> >     3. Disable vmload/vmsave virtualization if vendor=GenuineIntel.
> >        (It is somewhat insane to set vendor=GenuineIntel and still enable
> >        SVM for the guest but well whatever).
> >        Then zero the high 32 bit parts when kvm intercepts and emulates vmload.
> > 
> > Presumably AMD hardware loads only the lower 32 bits, which would leave garbage
> > in the upper bits and even leak state from L1 to L2 (again ignoring the fact that
> > exposing SVM to an Intel vCPU is bonkers).
> Is it because L1 is a VM migrated from Intel platform to AMD's?

I believe so.

> So w/o commit adc2a23734ac ("KVM: nSVM: improve SYSENTER emulation on AMD"):
> 1> L1 could be a "GenuineIntel" with SVM capability (bizarre as it is), running
> in 64-bit mode.
> 2> With no interception of MSR writes to the SYSENTER_EIP/ESP, L1 may set its
> SYSENTER_EIP/ESP to a 64-bit value successfully (though sysenter/sysexit may
> fail).

Yes, though the MSRs don't need to be passed through, KVM emulates the full 64 bits
if the guest CPUID model is Intel.

> 3> L2 could be in 32-bit mode. And if virtual vmload/vmsave is enabled for L1,
> only lower 32 bits of those MSRs will be loaded, leaking the higher 32 bits.
> 
> Is above scenario the reason of Maxim's fix?

Yes, that's my understanding.

> But why it is related to nested migration? 

I understand why it's related, but I don't understand why we bothered to add "support"
for this.

In theory, if L1 is migrated by L0 while L1 is running an L2 that uses SYSENTER,
problems will occur.  I'm a bit lost as to how this matters in practice, as KVM
doesn't support cross-vendor nested virtualization, and if L1 can be enlightened
to the point where it can switch from VMX=>SVM during migration, what's the point
of doing a migration?