linux-kernel - Re: [PATCH 4/8] kvm: vmx: Set IBPB when running a different VCPU

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:   Mon, 15 Jan 2018 10:21:08 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     "Woodhouse, David" <dwmw@...zon.co.uk>,
        "jmattson@...gle.com" <jmattson@...gle.com>,
        "kernellwp@...il.com" <kernellwp@...il.com>
Cc:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "tim.c.chen@...ux.intel.com" <tim.c.chen@...ux.intel.com>,
        "Liguori, Anthony" <aliguori@...zon.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "liran.alon@...cle.com" <liran.alon@...cle.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "thomas.lendacky@....com" <thomas.lendacky@....com>,
        "rkrcmar@...hat.com" <rkrcmar@...hat.com>
Subject: Re: [PATCH 4/8] kvm: vmx: Set IBPB when running a different VCPU

On 13/01/2018 10:29, Woodhouse, David wrote:
> On Fri, 2018-01-12 at 09:03 -0800, Jim Mattson wrote:
>> The point behind the IPBP in vmx_vcpu_load is to prevent one VCPU from
>> steering the speculative execution of the next. If the VMCS address is
>> recycled, vmx_vcpu_load doesn't realize that the VCPUs are different,
>> and so it won't issue the IPBP.
> 
> I don't understand the sequence of events that could lead to this.
> 
> If the VMCS is freed, surely per_cpu(current_vmcs, cpu) has to be
> cleared? If the VMCS is freed while it's still *active* on a CPU,
> that's a bug, surely? And if that CPU is later offlined and clears the
> VMCS, it's going to scribble on freed (and potentially re-used) memory.

You're right, svm.c needs it but vmx.c does not (because AMD has no
vmclear equivalent).

>>> +       if (have_spec_ctrl)
>>> +               wrmsrl(MSR_IA32_PRED_CMD, PRED_CMD_IBPB);
> 
> Also, I think the same condition applies to the conditional branches
> over the IBPB-frobbing, as it does to setting IBRS. You can eschew the
> 'else lfence' only if you put in a comment showing that you've proved
> it's safe. Many of the other bits like this are being done with
> alternatives, which avoids that concern completely.
> 
> But really, I don't like this series much. Don't say "let's do this
> until upstream supports...". Just fix it up properly, and add the
> generic X86_FEATURE_IBPB bit and use it. We have *too* many separate
> tiny patch sets, and we need to be getting our act together and putting
> it all in one.

I agree, this series is not meant to be committed.  I only posted to get
early reviews (and it was very effective for that purpose).

Paolo