Linux Patch #4

PATCH In review [email protected] 2 participants 2 messages Apr 27, 2018 – Apr 28, 2018

Subsystems (3)

x86 KVM x86 x86 hardware vulnerabilities

lore

Share & tools

lore

[email protected]

author

2018-04-27 00:01 UTC #0 lore raw

PATCH v6 4/10 Linux Patch #4

A guest may modify the SPEC_CTRL MSR from the value used by the kernel. Since we don't use IBRS, this means a value of zero is what we need in the host.

But the 336996-Speculative-Execution-Side-Channel-Mitigations.pdf refers to the other bits as reserved so we should respect the boot time SPEC_CTRL value and use that.

This allows us to deal with future extensions to the SPEC_CTRL interface if any at all.

Note: We are using wrmsrl instead of native_wrmsl. I does not make any difference as paravirt will over-write the callq *0xfff.. with the wrmsrl assembler code.

A copy of this document is available at
https://bugzilla.kernel.org/show_bug.cgi?id=199511

xReviewed-by Borislav Petkov <[email protected]>

Signed-off-by Konrad Rzeszutek Wilk <[email protected]>

---
v2: New patch
v3: Use the two accessory functions instead of poking at the global variable.
v4: Use x86_get_spec_ctrl instead of global variable.
v5: Use x86_get_default_spec_ctrl instead of x86_get_spec_ctrl
v6: Added Reviewed-by

---
 arch/x86/include/asm/nospec-branch.h | 10 ++++++++++
 arch/x86/kernel/cpu/bugs.c           | 18 ++++++++++++++++++
 arch/x86/kvm/svm.c                   |  6 ++----
 arch/x86/kvm/vmx.c                   |  6 ++----
 4 files changed, 32 insertions(+), 8 deletions(-)

arch/x86/include/asm/nospec-branch.h +10 -0

index 984f4a36960f..243a0c064e84 100644
@@ -228,6 +228,16 @@ enum spectre_v2_mitigation {
 extern void x86_set_spec_ctrl(u64);
 extern u64 x86_get_default_spec_ctrl(void);
 
+/*
+ * On VMENTER we must preserve whatever view of the SPEC_CTRL MSR
+ * the guest has, while on VMEXIT we restore the host view. This
+ * would be easier if SPEC_CTRL were architecturally maskable or
+ * shadowable for guests but this is not (currently) the case.
+ * Takes the guest view of SPEC_CTRL MSR as a parameter.
+ */
+extern void x86_set_guest_spec_ctrl(u64);
+extern void x86_restore_host_spec_ctrl(u64);
+
 extern char __indirect_thunk_start[];
 extern char __indirect_thunk_end[];

arch/x86/kernel/cpu/bugs.c +18 -0

index 048719d5c9b8..3c271c3e8be3 100644
@@ -123,6 +123,24 @@ u64 x86_get_default_spec_ctrl(void)
 }
 EXPORT_SYMBOL_GPL(x86_get_default_spec_ctrl);
 
+void x86_set_guest_spec_ctrl(u64 guest_spec_ctrl)
+{
+	if (x86_get_default_spec_ctrl() == guest_spec_ctrl)
+		return;
+	else
+		wrmsrl(MSR_IA32_SPEC_CTRL, guest_spec_ctrl);
+}
+EXPORT_SYMBOL_GPL(x86_set_guest_spec_ctrl);
+
+void x86_restore_host_spec_ctrl(u64 guest_spec_ctrl)
+{
+	if (x86_get_default_spec_ctrl() == guest_spec_ctrl)
+		return;
+	else
+		wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
+}
+EXPORT_SYMBOL_GPL(x86_restore_host_spec_ctrl);
+
 #ifdef RETPOLINE
 static bool spectre_v2_bad_module;

arch/x86/kvm/svm.c +2 -4

index 1fc05e428aba..164e9780abe3 100644
@@ -5557,8 +5557,7 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
 	 * is no need to worry about the conditional branch over the wrmsr
 	 * being speculatively taken.
 	 */
-	if (svm->spec_ctrl)
-		native_wrmsrl(MSR_IA32_SPEC_CTRL, svm->spec_ctrl);
+	x86_set_guest_spec_ctrl(svm->spec_ctrl);
 
 	asm volatile (
 		"push %%" _ASM_BP "; \n\t"

@@ -5670,8 +5669,7 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
 	if (unlikely(!msr_write_intercepted(vcpu, MSR_IA32_SPEC_CTRL)))
 		svm->spec_ctrl = native_read_msr(MSR_IA32_SPEC_CTRL);
 
-	if (svm->spec_ctrl)
-		native_wrmsrl(MSR_IA32_SPEC_CTRL, 0);
+	x86_restore_host_spec_ctrl(svm->spec_ctrl);
 
 	/* Eliminate branch target predictions from guest mode */
 	vmexit_fill_RSB();

arch/x86/kvm/vmx.c +2 -4

index aa66ccd6ed6c..ae73921bf015 100644
@@ -9726,8 +9726,7 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	 * is no need to worry about the conditional branch over the wrmsr
 	 * being speculatively taken.
 	 */
-	if (vmx->spec_ctrl)
-		native_wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
+	x86_set_guest_spec_ctrl(vmx->spec_ctrl);
 
 	vmx->__launched = vmx->loaded_vmcs->launched;

@@ -9875,8 +9874,7 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	if (unlikely(!msr_write_intercepted(vcpu, MSR_IA32_SPEC_CTRL)))
 		vmx->spec_ctrl = native_read_msr(MSR_IA32_SPEC_CTRL);
 
-	if (vmx->spec_ctrl)
-		native_wrmsrl(MSR_IA32_SPEC_CTRL, 0);
+	x86_restore_host_spec_ctrl(vmx->spec_ctrl);
 
 	/* Eliminate branch target predictions from guest mode */
 	vmexit_fill_RSB();

Signature · 1 line

2.14.3

Thomas Gleixner

2018-04-28 19:24 UTC #1 lore raw

arch/x86/kernel/cpu/bugs.c u64 x86_get_default_spec_ctrl(void) · line 128

Quoted 4 lines · speck for konrad.wilk_at_oracle.com

arch/x86/kernel/cpu/bugs.c +3 -0

+void x86_set_guest_spec_ctrl(u64 guest_spec_ctrl)
+{
+	if (x86_get_default_spec_ctrl() == guest_spec_ctrl)

This call is pretty pointless because the variable is defined in the same file and actually it needs to be make globaly visible anyway to avoid a function call in switch_to().

arch/x86/kernel/cpu/bugs.c u64 x86_get_default_spec_ctrl(void) · line 140

Quoted 12 lines

arch/x86/kernel/cpu/bugs.c +12 -0

+		return;
+	else
+		wrmsrl(MSR_IA32_SPEC_CTRL, guest_spec_ctrl);
+}
+EXPORT_SYMBOL_GPL(x86_set_guest_spec_ctrl);
+
+void x86_restore_host_spec_ctrl(u64 guest_spec_ctrl)
+{
+	if (x86_get_default_spec_ctrl() == guest_spec_ctrl)
+		return;
+	else
+		wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);

What's wrong with

	if (default != guest)
		wrmsrl()

Thanks,

tglx