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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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)
> --