Re: [PATCH RFC] KVM: nSVM: Fix L1 state corruption upon return from SMM

From: Paolo Bonzini <pbonzini@redhat.com>
To: Maxim Levitsky <mlevitsk@redhat.com>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	Michael Roth <mdroth@linux.vnet.ibm.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>,
	Cathy Avery <cavery@redhat.com>,
	Emanuele Giuseppe Esposito <eesposit@redhat.com>,
	linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	Sean Christopherson <seanjc@google.com>
Subject: Re: [PATCH RFC] KVM: nSVM: Fix L1 state corruption upon return from SMM
Date: Thu, 24 Jun 2021 12:38:11 +0200	[thread overview]
Message-ID: <a8945898-9fcb-19f1-1ba1-c9be55e04580@redhat.com> (raw)
In-Reply-To: <83affeedb9a3d091bece8f5fdd5373342298dcd3.camel@redhat.com>

On 24/06/21 10:20, Maxim Levitsky wrote:
> Something else to note, just for our information is that KVM
> these days does vmsave/vmload to VM_HSAVE_PA to store/restore
> the additional host state, something that is frowned upon in the spec,
> but there is some justification of doing this in the commit message,
> citing an old spec which allowed this.

True that.  And there is no mention in the specification for VMRUN that 
the host state-save area is a subset of the VMCB format (i.e., that it 
uses VMCB offsets for whatever subset of the state it saves in the 
VMCB), so the spec reference in the commit message is incorrect.  It 
would be nice if the spec guaranteed that.  Michael, Tom?

In fact, Vitaly's patch *will* overwrite the vmsave/vmload parts of 
VM_HSAVE_PA, and it will store the L2 values rather than the L1 values, 
because KVM always does its vmload/vmrun/vmsave sequence using 
vmload(vmcs01) and vmsave(vmcs01)!  So that has to be changed to use 
code similar to svm_set_nested_state (which can be moved to a separate 
function and reused):

         dest->es = src->es;
         dest->cs = src->cs;
         dest->ss = src->ss;
         dest->ds = src->ds;
         dest->gdtr = src->gdtr;
         dest->idtr = src->idtr;
         dest->rflags = src->rflags | X86_EFLAGS_FIXED;
         dest->efer = src->efer;
         dest->cr0 = src->cr0;
         dest->cr3 = src->cr3;
         dest->cr4 = src->cr4;
         dest->rax = src->rax;
         dest->rsp = src->rsp;
         dest->rip = src->rip;
         dest->cpl = 0;

Paolo