linux-kernel - Re: [PATCH v2 2/4] KVM: nSVM: correctly restore nested_run

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]

Date:   Thu, 7 Jan 2021 12:19:30 -0800
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Maxim Levitsky <mlevitsk@...hat.com>, kvm@...r.kernel.org,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, Borislav Petkov <bp@...en8.de>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        Ingo Molnar <mingo@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Joerg Roedel <joro@...tes.org>,
        Jim Mattson <jmattson@...gle.com>
Subject: Re: [PATCH v2 2/4] KVM: nSVM: correctly restore nested_run_pending
 on migration

On Thu, Jan 07, 2021, Paolo Bonzini wrote:
> On 07/01/21 10:38, Maxim Levitsky wrote:
> > The code to store it on the migration exists, but no code was restoring it.
> > 
> > One of the side effects of fixing this is that L1->L2 injected events
> > are no longer lost when migration happens with nested run pending.
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@...hat.com>
> > ---
> >   arch/x86/kvm/svm/nested.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > index ee4f2082ad1bd..cc3130ab612e5 100644
> > --- a/arch/x86/kvm/svm/nested.c
> > +++ b/arch/x86/kvm/svm/nested.c
> > @@ -1200,6 +1200,10 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
> >   	 * in the registers, the save area of the nested state instead
> >   	 * contains saved L1 state.
> >   	 */
> > +
> > +	svm->nested.nested_run_pending =
> > +		!!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING);
> > +
> >   	copy_vmcb_control_area(&hsave->control, &svm->vmcb->control);
> >   	hsave->save = *save;
> > 
> 
> Nice fix and we need to do it anyway.
> 
> That said, the v1 change had some appeal to it.

Which v1 change are you referring to?

> In the VMX case (if properly implemented) it would allow removing the weird
> nested_run_pending case from prepare_vmcs02_early.  I think it's a valuable
> invariant that there are no events in the VMCS after each KVM_RUN iteration,
> and this special case is breaking the invariant.

Hmm, as weird as that code is, I think it's actually the most architecturally
correct behavior.  Technically, the clearing of VM_ENTRY_INTR_INFO.VALID
shouldn't be visible in vmcs12 until a nested VM-Exit occurs, e.g. copying the
vmcs02 value to vmcs12 in vmx_get_nested_state() would work, but it's wrong at
the same time.  Ditto for L1 (or L2) writing vmcs12.VM_ENTRY_INTR_INFO while L2
is running (ignoring the SDM's very clear warning that doing so is bad); from
L1/L2's perspective, there is no VM-Entry so writing vmcs12.VM_ENTRY_INTR_INFO
should never generate an event in L2, even on CPUs without VMCS caching.