Skip to content

Commit 52887af

Browse files
sean-jcbonzini
sean-jc
authored and
bonzini
committed
KVM: x86: Revert MSR_IA32_FLUSH_CMD.FLUSH_L1D enabling
Revert the recently added virtualizing of MSR_IA32_FLUSH_CMD, as both the VMX and SVM are fatally buggy to guests that use MSR_IA32_FLUSH_CMD or MSR_IA32_PRED_CMD, and because the entire foundation of the logic is flawed. The most immediate problem is an inverted check on @cmd that results in rejecting legal values. SVM doubles down on bugs and drops the error, i.e. silently breaks all guest mitigations based on the command MSRs. The next issue is that neither VMX nor SVM was updated to mark MSR_IA32_FLUSH_CMD as being a possible passthrough MSR, which isn't hugely problematic, but does break MSR filtering and triggers a WARN on VMX designed to catch this exact bug. The foundational issues stem from the MSR_IA32_FLUSH_CMD code reusing logic from MSR_IA32_PRED_CMD, which in turn was likely copied from KVM's support for MSR_IA32_SPEC_CTRL. The copy+paste from MSR_IA32_SPEC_CTRL was misguided as MSR_IA32_PRED_CMD (and MSR_IA32_FLUSH_CMD) is a write-only MSR, i.e. doesn't need the same "deferred passthrough" shenanigans as MSR_IA32_SPEC_CTRL. Revert all MSR_IA32_FLUSH_CMD enabling in one fell swoop so that there is no point where KVM advertises, but does not support, L1D_FLUSH. This reverts commits 45cf86f, 723d5fb, and a807b78. Reported-by: Nathan Chancellor <nathan@kernel.org> Link: https://lkml.kernel.org/r/20230317190432.GA863767%40dev-arch.thelio-3990X Cc: Emanuele Giuseppe Esposito <eesposit@redhat.com> Cc: Pawan Gupta <pawan.kumar.gupta@linux.intel.com> Cc: Jim Mattson <jmattson@google.com> Signed-off-by: Sean Christopherson <seanjc@google.com> Tested-by: Mathias Krause <minipli@grsecurity.net> Message-Id: <20230322011440.2195485-2-seanjc@google.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
1 parent d8708b8 commit 52887af

4 files changed

Lines changed: 39 additions & 77 deletions

File tree

‎arch/x86/kvm/cpuid.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -653,7 +653,7 @@ void kvm_set_cpu_caps(void)
653653
F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | F(INTEL_STIBP) |
654654
F(MD_CLEAR) | F(AVX512_VP2INTERSECT) | F(FSRM) |
655655
F(SERIALIZE) | F(TSXLDTRK) | F(AVX512_FP16) |
656-
F(AMX_TILE) | F(AMX_INT8) | F(AMX_BF16) | F(FLUSH_L1D)
656+
F(AMX_TILE) | F(AMX_INT8) | F(AMX_BF16)
657657
);
658658

659659
/* TSC_ADJUST and ARCH_CAPABILITIES are emulated in software. */

‎arch/x86/kvm/svm/svm.c‎

Lines changed: 13 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -2869,28 +2869,6 @@ static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64 data)
28692869
return 0;
28702870
}
28712871

2872-
static int svm_set_msr_ia32_cmd(struct kvm_vcpu *vcpu, struct msr_data *msr,
2873-
bool guest_has_feat, u64 cmd,
2874-
int x86_feature_bit)
2875-
{
2876-
struct vcpu_svm *svm = to_svm(vcpu);
2877-
2878-
if (!msr->host_initiated && !guest_has_feat)
2879-
return 1;
2880-
2881-
if (!(msr->data & ~cmd))
2882-
return 1;
2883-
if (!boot_cpu_has(x86_feature_bit))
2884-
return 1;
2885-
if (!msr->data)
2886-
return 0;
2887-
2888-
wrmsrl(msr->index, cmd);
2889-
set_msr_interception(vcpu, svm->msrpm, msr->index, 0, 1);
2890-
2891-
return 0;
2892-
}
2893-
28942872
static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
28952873
{
28962874
struct vcpu_svm *svm = to_svm(vcpu);
@@ -2965,14 +2943,19 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
29652943
set_msr_interception(vcpu, svm->msrpm, MSR_IA32_SPEC_CTRL, 1, 1);
29662944
break;
29672945
case MSR_IA32_PRED_CMD:
2968-
r = svm_set_msr_ia32_cmd(vcpu, msr,
2969-
guest_has_pred_cmd_msr(vcpu),
2970-
PRED_CMD_IBPB, X86_FEATURE_IBPB);
2971-
break;
2972-
case MSR_IA32_FLUSH_CMD:
2973-
r = svm_set_msr_ia32_cmd(vcpu, msr,
2974-
guest_cpuid_has(vcpu, X86_FEATURE_FLUSH_L1D),
2975-
L1D_FLUSH, X86_FEATURE_FLUSH_L1D);
2946+
if (!msr->host_initiated &&
2947+
!guest_has_pred_cmd_msr(vcpu))
2948+
return 1;
2949+
2950+
if (data & ~PRED_CMD_IBPB)
2951+
return 1;
2952+
if (!boot_cpu_has(X86_FEATURE_IBPB))
2953+
return 1;
2954+
if (!data)
2955+
break;
2956+
2957+
wrmsrl(MSR_IA32_PRED_CMD, PRED_CMD_IBPB);
2958+
set_msr_interception(vcpu, svm->msrpm, MSR_IA32_PRED_CMD, 0, 1);
29762959
break;
29772960
case MSR_AMD64_VIRT_SPEC_CTRL:
29782961
if (!msr->host_initiated &&

‎arch/x86/kvm/vmx/nested.c‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -654,9 +654,6 @@ static inline bool nested_vmx_prepare_msr_bitmap(struct kvm_vcpu *vcpu,
654654
nested_vmx_set_intercept_for_msr(vmx, msr_bitmap_l1, msr_bitmap_l0,
655655
MSR_IA32_PRED_CMD, MSR_TYPE_W);
656656

657-
nested_vmx_set_intercept_for_msr(vmx, msr_bitmap_l1, msr_bitmap_l0,
658-
MSR_IA32_FLUSH_CMD, MSR_TYPE_W);
659-
660657
kvm_vcpu_unmap(vcpu, &vmx->nested.msr_bitmap_map, false);
661658

662659
vmx->nested.force_msr_bitmap_recalc = false;

‎arch/x86/kvm/vmx/vmx.c‎

Lines changed: 25 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -2133,39 +2133,6 @@ static u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated
21332133
return debugctl;
21342134
}
21352135

2136-
static int vmx_set_msr_ia32_cmd(struct kvm_vcpu *vcpu,
2137-
struct msr_data *msr_info,
2138-
bool guest_has_feat, u64 cmd,
2139-
int x86_feature_bit)
2140-
{
2141-
if (!msr_info->host_initiated && !guest_has_feat)
2142-
return 1;
2143-
2144-
if (!(msr_info->data & ~cmd))
2145-
return 1;
2146-
if (!boot_cpu_has(x86_feature_bit))
2147-
return 1;
2148-
if (!msr_info->data)
2149-
return 0;
2150-
2151-
wrmsrl(msr_info->index, cmd);
2152-
2153-
/*
2154-
* For non-nested:
2155-
* When it's written (to non-zero) for the first time, pass
2156-
* it through.
2157-
*
2158-
* For nested:
2159-
* The handling of the MSR bitmap for L2 guests is done in
2160-
* nested_vmx_prepare_msr_bitmap. We should not touch the
2161-
* vmcs02.msr_bitmap here since it gets completely overwritten
2162-
* in the merging.
2163-
*/
2164-
vmx_disable_intercept_for_msr(vcpu, msr_info->index, MSR_TYPE_W);
2165-
2166-
return 0;
2167-
}
2168-
21692136
/*
21702137
* Writes msr value into the appropriate "register".
21712138
* Returns 0 on success, non-0 otherwise.
@@ -2319,16 +2286,31 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
23192286
return 1;
23202287
goto find_uret_msr;
23212288
case MSR_IA32_PRED_CMD:
2322-
ret = vmx_set_msr_ia32_cmd(vcpu, msr_info,
2323-
guest_has_pred_cmd_msr(vcpu),
2324-
PRED_CMD_IBPB,
2325-
X86_FEATURE_IBPB);
2326-
break;
2327-
case MSR_IA32_FLUSH_CMD:
2328-
ret = vmx_set_msr_ia32_cmd(vcpu, msr_info,
2329-
guest_cpuid_has(vcpu, X86_FEATURE_FLUSH_L1D),
2330-
L1D_FLUSH,
2331-
X86_FEATURE_FLUSH_L1D);
2289+
if (!msr_info->host_initiated &&
2290+
!guest_has_pred_cmd_msr(vcpu))
2291+
return 1;
2292+
2293+
if (data & ~PRED_CMD_IBPB)
2294+
return 1;
2295+
if (!boot_cpu_has(X86_FEATURE_IBPB))
2296+
return 1;
2297+
if (!data)
2298+
break;
2299+
2300+
wrmsrl(MSR_IA32_PRED_CMD, PRED_CMD_IBPB);
2301+
2302+
/*
2303+
* For non-nested:
2304+
* When it's written (to non-zero) for the first time, pass
2305+
* it through.
2306+
*
2307+
* For nested:
2308+
* The handling of the MSR bitmap for L2 guests is done in
2309+
* nested_vmx_prepare_msr_bitmap. We should not touch the
2310+
* vmcs02.msr_bitmap here since it gets completely overwritten
2311+
* in the merging.
2312+
*/
2313+
vmx_disable_intercept_for_msr(vcpu, MSR_IA32_PRED_CMD, MSR_TYPE_W);
23322314
break;
23332315
case MSR_IA32_CR_PAT:
23342316
if (!kvm_pat_valid(data))

0 commit comments

Comments
 (0)