On 18/01/2018 16:32, Paolo Bonzini wrote:
> On 18/01/2018 14:48, Peter Zijlstra wrote:
>> From: Ashok Raj <ashok....@intel.com>
>>
>> Add MSR passthrough for MSR_IA32_PRED_CMD and place branch predictor
>> barriers on switching between VMs to avoid inter VM specte-v2 attacks.
>>
>> [peterz: rebase and changelog rewrite]
>>
>> Cc: Asit Mallick <asit.k.mall...@intel.com>
>> Cc: Dave Hansen <dave.han...@intel.com>
>> Cc: Arjan Van De Ven <arjan.van.de....@intel.com>
>> Cc: Tim Chen <tim.c.c...@linux.intel.com>
>> Cc: Linus Torvalds <torva...@linux-foundation.org>
>> Cc: Andrea Arcangeli <aarca...@redhat.com>
>> Cc: Andi Kleen <a...@linux.intel.com>
>> Cc: Thomas Gleixner <t...@linutronix.de>
>> Cc: Dan Williams <dan.j.willi...@intel.com>
>> Cc: Jun Nakajima <jun.nakaj...@intel.com>
>> Cc: Andy Lutomirski <l...@kernel.org>
>> Cc: Greg KH <gre...@linuxfoundation.org>
>> Cc: David Woodhouse <d...@amazon.co.uk>
>> Cc: Paolo Bonzini <pbonz...@redhat.com>
>> Signed-off-by: Ashok Raj <ashok....@intel.com>
>> Signed-off-by: Peter Zijlstra (Intel) <pet...@infradead.org>
>> ---
>> arch/x86/kvm/svm.c | 8 ++++++++
>> arch/x86/kvm/vmx.c | 8 ++++++++
>> 2 files changed, 16 insertions(+)
>
> This patch is missing the AMD-specific CPUID bit.
>
> In addition, it is not bisectable because guests will see the SPEC_CTRL CPUID
> bit after patch 32 even if PRED_CMD is not supported yet.
>
> The simplest solutions are:
>
> 1) add the indirect_branch_prediction_barrier() calls first, and squash
> everything else including the AMD CPUID bit into a single patch.
>
> 2) place the IBPB in this series, and only add stop/restart_indirect_branch_
> speculation() to the vmexit and vmentry paths. We will do the guest enabling
> in kvm.git after this is ready, it should only take a week and we'll ensure
> it is backportable to 4.14 and 4.15.
>
> 3) same as (2) except we'll send the guest enabling through TIP.
>
> Paolo
>
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -252,6 +252,7 @@ static const struct svm_direct_access_ms
>> { .index = MSR_SYSCALL_MASK, .always = true },
>> #endif
>> { .index = MSR_IA32_SPEC_CTRL, .always = true },
>> + { .index = MSR_IA32_PRED_CMD, .always = true },
>> { .index = MSR_IA32_LASTBRANCHFROMIP, .always = false },
>> { .index = MSR_IA32_LASTBRANCHTOIP, .always = false },
>> { .index = MSR_IA32_LASTINTFROMIP, .always = false },
>> @@ -532,6 +533,7 @@ struct svm_cpu_data {
>> struct kvm_ldttss_desc *tss_desc;
>>
>> struct page *save_area;
>> + struct vmcb *current_vmcb;
>> };
>>
>> static DEFINE_PER_CPU(struct svm_cpu_data *, svm_data);
>> @@ -1709,11 +1711,13 @@ static void svm_free_vcpu(struct kvm_vcp
>> __free_pages(virt_to_page(svm->nested.msrpm), MSRPM_ALLOC_ORDER);
>> kvm_vcpu_uninit(vcpu);
>> kmem_cache_free(kvm_vcpu_cache, svm);
>> + indirect_branch_prediction_barrier();
>> }
>>
>> static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>> {
>> struct vcpu_svm *svm = to_svm(vcpu);
>> + struct svm_cpu_data *sd = per_cpu(svm_data, cpu);
>> int i;
>>
>> if (unlikely(cpu != vcpu->cpu)) {
>> @@ -1742,6 +1746,10 @@ static void svm_vcpu_load(struct kvm_vcp
>> if (static_cpu_has(X86_FEATURE_RDTSCP))
>> wrmsrl(MSR_TSC_AUX, svm->tsc_aux);
>>
>> + if (sd->current_vmcb != svm->vmcb) {
>> + sd->current_vmcb = svm->vmcb;
>> + indirect_branch_prediction_barrier();
>> + }
>> avic_vcpu_load(vcpu, cpu);
>> }
>>
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -2280,6 +2280,7 @@ static void vmx_vcpu_load(struct kvm_vcp
>> if (per_cpu(current_vmcs, cpu) != vmx->loaded_vmcs->vmcs) {
>> per_cpu(current_vmcs, cpu) = vmx->loaded_vmcs->vmcs;
>> vmcs_load(vmx->loaded_vmcs->vmcs);
>> + indirect_branch_prediction_barrier();
>> }
>>
>> if (!already_loaded) {
>> @@ -3837,6 +3838,11 @@ static void free_loaded_vmcs(struct load
>> free_vmcs(loaded_vmcs->vmcs);
>> loaded_vmcs->vmcs = NULL;
>> WARN_ON(loaded_vmcs->shadow_vmcs != NULL);
>> + /*
>> + * The VMCS could be recycled, causing a false negative in vmx_vcpu_load
>> + * block speculative execution.
>> + */
>> + indirect_branch_prediction_barrier();
>
> This IBPB is not needed, as loaded_vmcs->NULL is now NULL and there will be a
> barrier the next time vmx_vcpu_load is called on this CPU.
Without retpolines, KVM userspace is not protected from the guest
poisoning the BTB, because there is no IBRS-barrier on the vmexit path.
So there are two more IBPBs that are needed if retpolines are enabled:
1) in kvm_sched_out
2) at the end of vcpu_run
Thanks,
Paolo
