[MODERATED] [patch 04/11] [PATCH v2 04/10] Linux Patch #4

[MODERATED] [patch 04/11] [PATCH v2 04/10] 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.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v2: New patch
---
 arch/x86/kvm/svm.c | 6 +++---
 arch/x86/kvm/vmx.c | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index be9c839e2c89..f666b4c21559 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -5401,7 +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)
+	if (svm->spec_ctrl || need_spec_ctrl_acc())
 		native_wrmsrl(MSR_IA32_SPEC_CTRL, svm->spec_ctrl);
 
 	asm volatile (
@@ -5514,8 +5514,8 @@ 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);
+	if (svm->spec_ctrl || need_spec_ctrl_acc())
+		native_wrmsrl(MSR_IA32_SPEC_CTRL, clear_spec_ctrl(SPEC_CTRL_IBRS));
 
 	/* 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..5ba310eefe2b 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -9466,7 +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)
+	if (vmx->spec_ctrl || need_spec_ctrl_acc())
 		native_wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
 
 	vmx->__launched = vmx->loaded_vmcs->launched;
@@ -9605,8 +9605,8 @@ 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);
+	if (vmx->spec_ctrl || need_spec_ctrl_acc())
+		native_wrmsrl(MSR_IA32_SPEC_CTRL, clear_spec_ctrl(SPEC_CTRL_IBRS));
 
 	/* Eliminate branch target predictions from guest mode */
 	vmexit_fill_RSB();
-- 
2.14.3

[MODERATED] Re: [patch 04/11] [PATCH v2 04/10] Linux Patch #4

[Borislav Petkov]

On Thu, Apr 19, 2018 at 10:25:44PM -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.
> 
> 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.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> v2: New patch
> ---
>  arch/x86/kvm/svm.c | 6 +++---
>  arch/x86/kvm/vmx.c | 6 +++---
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index be9c839e2c89..f666b4c21559 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -5401,7 +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)
> +	if (svm->spec_ctrl || need_spec_ctrl_acc())
>  		native_wrmsrl(MSR_IA32_SPEC_CTRL, svm->spec_ctrl);

Instead of those silly set/clear helpers, why not do this:

	if (svm->spec_ctrl)
		x86_enable_ibrs();

and hide in x86_enable_ibrs() all that logic of checking
x86_spec_ctrl_base and writing the MSR and whatever else we will need in
the future?

This way you don't need to export helper functions any bit defines
around the place - just the enable/disable functions which hide the
whole logic?

>  	asm volatile (
> @@ -5514,8 +5514,8 @@ 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);
> +	if (svm->spec_ctrl || need_spec_ctrl_acc())
> +		native_wrmsrl(MSR_IA32_SPEC_CTRL, clear_spec_ctrl(SPEC_CTRL_IBRS));

Same here:

	if (svm->spec_ctrl)
		x86_disable_ibrs();

And so on...

-- 
Regards/Gruss,
    Boris.

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

[MODERATED] Re: [patch 04/11] [PATCH v2 04/10] Linux Patch #4

[Konrad Rzeszutek Wilk]

On Fri, Apr 20, 2018 at 06:15:33PM +0200, speck for Borislav Petkov wrote:
> On Thu, Apr 19, 2018 at 10:25:44PM -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.
> > 
> > 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.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> > v2: New patch
> > ---
> >  arch/x86/kvm/svm.c | 6 +++---
> >  arch/x86/kvm/vmx.c | 6 +++---
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > index be9c839e2c89..f666b4c21559 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -5401,7 +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)
> > +	if (svm->spec_ctrl || need_spec_ctrl_acc())
> >  		native_wrmsrl(MSR_IA32_SPEC_CTRL, svm->spec_ctrl);
> 
> Instead of those silly set/clear helpers, why not do this:
> 
> 	if (svm->spec_ctrl)
> 		x86_enable_ibrs();

Wouldn't we leak our MD state to the guest? That is the guest
may have cleared everything (svm->spec_ctrl is zero when we VMEXIT),
and now we would be running it with MD bit set?
> 
> and hide in x86_enable_ibrs() all that logic of checking
> x86_spec_ctrl_base and writing the MSR and whatever else we will need in
> the future?

I think we need two helpers with this:


x86_set_spec_ctrl_for_unpriv(svm->spec_ctrl);
asm (..
	VMENTER
	VMEXIT event
);

x86_set_spec_ctrl_for_priv(svm->spec_ctrl);

?

> 
> This way you don't need to export helper functions any bit defines
> around the place - just the enable/disable functions which hide the
> whole logic?
> 
> >  	asm volatile (
> > @@ -5514,8 +5514,8 @@ 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);
> > +	if (svm->spec_ctrl || need_spec_ctrl_acc())
> > +		native_wrmsrl(MSR_IA32_SPEC_CTRL, clear_spec_ctrl(SPEC_CTRL_IBRS));
> 
> Same here:
> 
> 	if (svm->spec_ctrl)
> 		x86_disable_ibrs();
> 
> And so on...
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
> -- 

[MODERATED] Re: [patch 04/11] [PATCH v2 04/10] Linux Patch #4

[Borislav Petkov]

On Fri, Apr 20, 2018 at 12:39:40PM -0400, speck for Konrad Rzeszutek Wilk wrote:
> Wouldn't we leak our MD state to the guest? That is the guest
> may have cleared everything (svm->spec_ctrl is zero when we VMEXIT),
> and now we would be running it with MD bit set?

No, you pass the requested bits:

		x86_enable_ibrs(svm->spec_ctrl);

and that function then picks apart which bits the host supports and sets
them accordingly and filters out the reserved bits. You might call the
function then:

	x86_set_spec_ctrl();

and its counterpart

	x86_restore_spec_ctrl();

or whatever. The restore side would simply clear the IBRS bit as we
don't enable it on the host. It will restore the MD setting for the host
too.

-- 
Regards/Gruss,
    Boris.

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

[MODERATED] Re: [patch 04/11] [PATCH v2 04/10] Linux Patch #4

[Konrad Rzeszutek Wilk]

On Fri, Apr 20, 2018 at 07:03:31PM +0200, speck for Borislav Petkov wrote:
> On Fri, Apr 20, 2018 at 12:39:40PM -0400, speck for Konrad Rzeszutek Wilk wrote:
> > Wouldn't we leak our MD state to the guest? That is the guest
> > may have cleared everything (svm->spec_ctrl is zero when we VMEXIT),
> > and now we would be running it with MD bit set?
> 
> No, you pass the requested bits:
> 
> 		x86_enable_ibrs(svm->spec_ctrl);

I was thinking of the conditional:

if (svm->spec_ctrl) <=== HERE
	x86_enable_ibrs(svm->spec_ctrl);

Which meant that if the guest had set SPEC_CTRL to zero we would
never enter the x86_enable_ibrs function at all and VMENTER in the
guest with SPEC_CTRL MDD bit enabled (as we never restored
the guest SPEC_CTRL which is zero). Aka, leaking our state in it.

Hence thinking to ditch the conditional at all and just have those
two accessory functions.
> 
> and that function then picks apart which bits the host supports and sets
> them accordingly and filters out the reserved bits. You might call the
> function then:
> 
> 	x86_set_spec_ctrl();

> 
> and its counterpart
> 
> 	x86_restore_spec_ctrl();

Right. I am going to assume it would have an 'u64' as parameter.
> 
> or whatever. The restore side would simply clear the IBRS bit as we
> don't enable it on the host. It will restore the MD setting for the host
> too.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
> -- 

[MODERATED] Re: [patch 04/11] [PATCH v2 04/10] Linux Patch #4

[Jon Masters]

[-- Attachment #1: Type: text/plain, Size: 1253 bytes --]

On 04/20/2018 01:17 PM, speck for Konrad Rzeszutek Wilk wrote:
> On Fri, Apr 20, 2018 at 07:03:31PM +0200, speck for Borislav Petkov wrote:

>> and that function then picks apart which bits the host supports and sets
>> them accordingly and filters out the reserved bits. You might call the
>> function then:
>>
>> 	x86_set_spec_ctrl();
> 
>>
>> and its counterpart
>>
>> 	x86_restore_spec_ctrl();
> 
> Right. I am going to assume it would have an 'u64' as parameter.

Looking at this, I think we actually want this to be:

	x86_set_guest_spec_ctrl(u64 guest_spec_ctrl);
	x86_restore_kernel_spec_ctrl(u64 guest_spec_ctrl);

The logic will be to take whatever we saved at boot (x86_spec_ctrl_base)
and then flip whatever bits are requested (or none). I suppose it's
reasonable/desirable to not allow a guest to set bits we don't know
about in case those would somehow impact us. So for now that means
checking IBRS and MDD bits on guest entry and restoring kernel if they
don't happen to match when coming back out of a guest.

Paolo: you'll also want to medium term ponder the init path for
svm->spec_ctrl because zero /may/ not make sense forever.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

[MODERATED] Re: [patch 04/11] [PATCH v2 04/10] Linux Patch #4

[Jon Masters]

[-- Attachment #1: Type: text/plain, Size: 1678 bytes --]

On 04/20/2018 12:39 PM, speck for Konrad Rzeszutek Wilk wrote:
> On Fri, Apr 20, 2018 at 06:15:33PM +0200, speck for Borislav Petkov wrote:
>> On Thu, Apr 19, 2018 at 10:25:44PM -0400, speck for konrad.wilk_at_oracle.com wrote:
>>> KVM/SVM/VMX/x86/spectre_v2: Support the combination of guest IBRS and ours.

>>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>>> index be9c839e2c89..f666b4c21559 100644
>>> --- a/arch/x86/kvm/svm.c
>>> +++ b/arch/x86/kvm/svm.c
>>> @@ -5401,7 +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)
>>> +	if (svm->spec_ctrl || need_spec_ctrl_acc())
>>>  		native_wrmsrl(MSR_IA32_SPEC_CTRL, svm->spec_ctrl);
>>
>> Instead of those silly set/clear helpers, why not do this:
>>
>> 	if (svm->spec_ctrl)
>> 		x86_enable_ibrs();
> 
> Wouldn't we leak our MD state to the guest? That is the guest
> may have cleared everything (svm->spec_ctrl is zero when we VMEXIT),
> and now we would be running it with MD bit set?

Indeed, we would. And we should only do that if we mean to intentionally
override whatever the guest is doing. Longer term, as more bits are
added to SPEC_CTRL you're going to end up wanting Konrad's original
version because there's bound to be more bits.

btw, I do think there's value in being able to force a guest to run with
e.g. MD on but not much until Intel come back to us about masking. As it
stands, once we give SPEC_CTRL to a guest, it can just clobber it.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop