linux-kernel - Re: [PATCH] KVM: nVMX: Skip IBPB when switching between vmcs01 and vmcs02

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] [day] [month] [year] [list]

Message-ID: <20200501135806.GA3798@linux.intel.com>
Date:   Fri, 1 May 2020 06:58:06 -0700
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Alexander Graf <graf@...zon.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        KarimAllah Raslan <karahmed@...zon.de>
Subject: Re: [PATCH] KVM: nVMX: Skip IBPB when switching between vmcs01 and
 vmcs02

On Thu, Apr 30, 2020 at 11:22:20PM +0200, Alexander Graf wrote:
> 
> On 30.04.20 22:41, Sean Christopherson wrote:
> >
> >diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> >index 3ab6ca6062ce..818dd8ba5e9f 100644
> >--- a/arch/x86/kvm/vmx/vmx.c
> >+++ b/arch/x86/kvm/vmx/vmx.c
> >@@ -1311,10 +1311,12 @@ static void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu)
> >                 pi_set_on(pi_desc);
> >  }
> >
> >-void vmx_vcpu_load_vmcs(struct kvm_vcpu *vcpu, int cpu)
> >+void vmx_vcpu_load_vmcs(struct kvm_vcpu *vcpu, int cpu,
> >+                       struct loaded_vmcs *buddy)
> >  {
> >         struct vcpu_vmx *vmx = to_vmx(vcpu);
> >         bool already_loaded = vmx->loaded_vmcs->cpu == cpu;
> >+       struct vmcs *prev;
> >
> >         if (!already_loaded) {
> >                 loaded_vmcs_clear(vmx->loaded_vmcs);
> >@@ -1333,10 +1335,12 @@ void vmx_vcpu_load_vmcs(struct kvm_vcpu *vcpu, int cpu)
> >                 local_irq_enable();
> >         }
> >
> >-       if (per_cpu(current_vmcs, cpu) != vmx->loaded_vmcs->vmcs) {
> >+       prev = per_cpu(current_vmcs, cpu);
> >+       if (prev != vmx->loaded_vmcs->vmcs) {
> >                 per_cpu(current_vmcs, cpu) = vmx->loaded_vmcs->vmcs;
> >                 vmcs_load(vmx->loaded_vmcs->vmcs);
> >-               indirect_branch_prediction_barrier();
> >+               if (!buddy || buddy->vmcs != prev)
> >+                       indirect_branch_prediction_barrier();
> 
> I fail to understand the logic here though. What exactly are you trying to
> catch? We only do the barrier when the current_vmcs as loaded by
> vmx_vcpu_load_vmcs is different from the vmcs of the context that was
> issuing the vmcs load.
> 
> Isn't this a really complicated way to say "Don't flush for nested"? Why not
> just make it explicit and pass in a bool that says "nested = true" from
> vmx_switch_vmcs()? Is there any case I'm missing where that would be unsafe?o

I don't think so, the 'buddy' check was added out of paranoia, and partly
because I was rushing.  I originally had a 'bool nested_switch' as well.

I think I like the boolean approach better, the above check should never
fail in the nested switch case.  I'll add a WARN in vmx_switch_vmcs() with
a note in the patch to let Paolo know it's likely paranoia and can be
ripped out at his discretion.