[MODERATED] [PATCH v4 04/10] [PATCH v4 4/9] Linux Patch #4

[MODERATED] [PATCH v4 04/10] [PATCH v4 4/9] Linux Patch #4

[konrad.wilk]

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.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
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.
---
 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(-)

diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index 53db3010daa3..f5fb6783b26c 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -228,6 +228,16 @@ enum spectre_v2_mitigation {
 extern void x86_set_spec_ctrl(u64);
 extern u64 x86_get_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[];
 
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index db4ce57598fd..a153192944c9 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -125,6 +125,24 @@ u64 x86_get_spec_ctrl(void)
 }
 EXPORT_SYMBOL_GPL(x86_get_spec_ctrl);
 
+void x86_set_guest_spec_ctrl(u64 guest_spec_ctrl)
+{
+	if (x86_get_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_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;
 
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index be9c839e2c89..6229c7796699 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -5401,8 +5401,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"
@@ -5514,8 +5513,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();
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 657c93409042..153a03f28ba2 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -9466,8 +9466,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;
 	asm(
@@ -9605,8 +9604,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();
-- 
2.14.3

[MODERATED] Re: [PATCH v4 04/10] [PATCH v4 4/9] Linux Patch #4

[Borislav Petkov]

On Mon, Apr 23, 2018 at 11:16:05PM -0400, speck for konrad.wilk_at_oracle.com wrote:
> KVM/SVM/VMX/x86/spectre_v2: Support the combination of guest IBRS and ours.
> 
> 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.

...

> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index db4ce57598fd..a153192944c9 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -125,6 +125,24 @@ u64 x86_get_spec_ctrl(void)
>  }
>  EXPORT_SYMBOL_GPL(x86_get_spec_ctrl);
>  
> +void x86_set_guest_spec_ctrl(u64 guest_spec_ctrl)
> +{
> +	if (x86_get_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_spec_ctrl() == guest_spec_ctrl)
> +		return;
> +	else
> +		wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
> +}
> +EXPORT_SYMBOL_GPL(x86_restore_host_spec_ctrl);
> +

Ok, you've added the x86_[gs]et_spec_ctrl() accessors, good, but then
you moved the x86_(set_guest|restore_host)_spec_ctrl() VM helpers here
too, which makes the aforementioned accessors superfluous...

... unless you're planning to use x86_[gs]et_spec_ctrl() somewhere else.
I don't see any other usage in the current patchset though...

What's up?

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[MODERATED] Re: [PATCH v4 04/10] [PATCH v4 4/9] Linux Patch #4

[Konrad Rzeszutek Wilk]

On Tue, Apr 24, 2018 at 01:30:37PM +0200, speck for Borislav Petkov wrote:
> On Mon, Apr 23, 2018 at 11:16:05PM -0400, speck for konrad.wilk_at_oracle.com wrote:
> > KVM/SVM/VMX/x86/spectre_v2: Support the combination of guest IBRS and ours.
> > 
> > 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.
> 
> ...
> 
> > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> > index db4ce57598fd..a153192944c9 100644
> > --- a/arch/x86/kernel/cpu/bugs.c
> > +++ b/arch/x86/kernel/cpu/bugs.c
> > @@ -125,6 +125,24 @@ u64 x86_get_spec_ctrl(void)
> >  }
> >  EXPORT_SYMBOL_GPL(x86_get_spec_ctrl);
> >  
> > +void x86_set_guest_spec_ctrl(u64 guest_spec_ctrl)
> > +{
> > +	if (x86_get_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_spec_ctrl() == guest_spec_ctrl)
> > +		return;
> > +	else
> > +		wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
> > +}
> > +EXPORT_SYMBOL_GPL(x86_restore_host_spec_ctrl);
> > +
> 
> Ok, you've added the x86_[gs]et_spec_ctrl() accessors, good, but then
> you moved the x86_(set_guest|restore_host)_spec_ctrl() VM helpers here
> too, which makes the aforementioned accessors superfluous...

We need three of them:

 1) x86_set_guest_spec_ctrl and x86_restore_host_spec_ctrl for
   VMEXIT/VMENTER - and they need the conditional to make sure they
   are allright. And no checking for valid values as the guest
   may want to set to some we don't recognize.

 2). Something in the init_intel codebase to set the MSR while
   respecting the boot-time one. Right now the patch uses
   x86_set_spec_ctrl. But we could get rid of it and just use
   x86_get_spec_ctrl() and OR it with SPEC_CTRL_RDS value.

> 
> ... unless you're planning to use x86_[gs]et_spec_ctrl() somewhere else.
> I don't see any other usage in the current patchset though...

In the #7 - that is when Intel wants to set the MSR it uses
the x86_set_spec_ctrl.

> 
> What's up?
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
> --