lkml.org 
[lkml]   [2018]   [Jan]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 6/7] x86/svm: Set IBPB when running a different VCPU


On 08/01/18 20:08, Paolo Bonzini wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
>
> Set IBPB (Indirect Branch Prediction Barrier) when the current CPU is
> going to run a VCPU different from what was previously run. Nested
> virtualization uses the same VMCB for the second level guest, but the
> L1 hypervisor should be using IBRS to protect itself.
>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> arch/x86/kvm/svm.c | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 779981a00d01..dd744d896cec 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -289,6 +289,7 @@ struct amd_svm_iommu_ir {
> module_param(vgif, int, 0444);
>
> static bool __read_mostly have_spec_ctrl;
> +static bool __read_mostly have_ibpb_support;
>
> static void svm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0);
> static void svm_flush_tlb(struct kvm_vcpu *vcpu, bool invalidate_gpa);
> @@ -540,6 +541,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);
> @@ -1151,6 +1153,11 @@ static __init int svm_hardware_setup(void)
> pr_info("kvm: SPEC_CTRL available\n");
> else
> pr_info("kvm: SPEC_CTRL not available\n");
> + have_ibpb_support = have_spec_ctrl || cpu_has_ibpb_support();
> + if (have_ibpb_support)
> + pr_info("kvm: IBPB_SUPPORT available\n");
> + else
> + pr_info("kvm: IBPB_SUPPORT not available\n");
>
> return 0;
>
> @@ -1725,11 +1732,19 @@ static void svm_free_vcpu(struct kvm_vcpu *vcpu)
> __free_pages(virt_to_page(svm->nested.msrpm), MSRPM_ALLOC_ORDER);
> kvm_vcpu_uninit(vcpu);
> kmem_cache_free(kvm_vcpu_cache, svm);
> +
> + /*
> + * The VMCB could be recycled, causing a false negative in
> + * svm_vcpu_load; block speculative execution.
> + */
> + if (have_ibpb_support)
> + wrmsrl(MSR_IA32_PRED_CMD, FEATURE_SET_IBPB);
> }
>
> 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)) {
> @@ -1758,6 +1773,12 @@ static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> 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;
> + if (have_ibpb_support)
> + wrmsrl(MSR_IA32_PRED_CMD, FEATURE_SET_IBPB);
> + }
> +
> avic_vcpu_load(vcpu, cpu);
> }
>
> @@ -2798,6 +2819,11 @@ static int nested_svm_vmexit(struct vcpu_svm *svm)
> if (!nested_vmcb)
> return 1;
>
> + /*
> + * No need for IBPB here, the L1 hypervisor should be running with
> + * IBRS=1 and inserts one already when switching L2 VMs.
> + */
> +

I am not fully convinced yet this is OK from security perspective.
From the CPU point-of-view, both L1 & L2 run in the same
prediction-mode (as they are both running in SVM guest-mode). Therefore,
IBRS will not hide the BHB & BTB of L1 from L2.

This is how I understand things:
1. When transition between contexts in same prediction-mode, software is
responsible for issuing an IBPB to basically "delete" the "branch
prediction data" of the previous context such that the new context won't
be able to use it.
This is orthogonal to IBRS which makes sure new context won't use
"branch prediction data" that was created by a previous less-privileged
prediction-mode as we are talking about transitioning between 2 contexts
in same physical prediction-mode.
2. Because of (1), KVM was modified to issue IBPB when replacing active
VMCB/VMCS on CPU.
3. For the nested-virtualization case, L1 & L2 both run in same physical
prediction-mode. As from physical CPU perspective, they are both running
in SVM guest-mode. Therefore, L0 should issue IBPB when transitioning
from L1 to L2 and vice-versa.
4. In nested VMX case, this already happens because transitioning
between L1 & L2 involves changing active VMCS on CPU (from vmcs01 to
vmcs02) which already issues an IBPB.
5. However, nested SVM is reusing the same VMCB when transitioning
between L1 & L2 and therefore we should explicitly issue an IBPB in
nested_svm_vmrun() & nested_svm_vmexit().
6. One can implicitly assume that L1 hypervisor did issued a IBPB when
loading an VMCB and therefore it doesn't have to do it again in L0.
However, I see 2 problems with this approach:
(a) We don't protect old non-patched L1 hypervisors from Spectre even
though we could easily do that from L0 with small performance hit.
(b) When L1 hypervisor runs only a single L2 VM, it doesn't have to
issue an IBPB when loading the VMCB (as it didn't run any other VMCB
before) and it should be sufficient from L1 perspective to just write 1
to IBRS. However, in our nested-case, this is a security-hole.

Am I misunderstanding something?

Regards,
-Liran

> /* Exit Guest-Mode */
> leave_guest_mode(&svm->vcpu);
> svm->nested.vmcb = 0;
> @@ -3061,6 +3087,11 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
> if (!nested_vmcb)
> return false;
>
> + /*
> + * No need for IBPB here, since the nested VM is less privileged. The
> + * L1 hypervisor inserts one already when switching L2 VMs.
> + */
> +
> if (!nested_vmcb_checks(nested_vmcb)) {
> nested_vmcb->control.exit_code = SVM_EXIT_ERR;
> nested_vmcb->control.exit_code_hi = 0;
>

\
 
 \ /
  Last update: 2018-01-14 23:18    [W:0.222 / U:0.568 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site