From mboxrd@z Thu Jan  1 00:00:00 1970
Return-path: <ak@linux.intel.com>
Received: from mga05.intel.com ([192.55.52.43])
	by Galois.linutronix.de with esmtps (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256)
	(Exim 4.80)
	(envelope-from <ak@linux.intel.com>)
	id 1fGpkS-0001OU-SH
	for speck@linutronix.de; Thu, 10 May 2018 19:53:01 +0200
Date: Thu, 10 May 2018 10:52:57 -0700
From: Andi Kleen <ak@linux.intel.com>
Subject: [MODERATED] Re: [patch V11 05/16] SSB 5
Message-ID: <20180510175257.GD13616@tassilo.jf.intel.com>
References: <20180502215102.192655950@linutronix.de>
 <20180502215416.459915781@linutronix.de>
MIME-Version: 1.0
In-Reply-To: <20180502215416.459915781@linutronix.de>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
To: speck@linutronix.de
List-ID: <speck.linutronix.de>

Hi,

I went over this patch again and I'm not sure i understand
how it works.


On Wed, May 02, 2018 at 11:51:07PM +0200, speck for Thomas Gleixner wrote:
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -9720,8 +9720,7 @@ static void __noclone vmx_vcpu_run(struc
>  	 * 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_spec_ctrl_set_guest(vmx->spec_ctrl);
>  
>  	vmx->__launched = vmx->loaded_vmcs->launched;
>  
> @@ -9869,8 +9868,7 @@ static void __noclone vmx_vcpu_run(struc
>  	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_spec_ctrl_restore_host(vmx->spec_ctrl);

So we assume that vmx->spec_ctrl is always the correct value of the guest.


But vmx_set_msr does

	        vmx->spec_ctrl = data;

                if (!data)
                        break;

                /*
                 * For non-nested:
                 * When it's written (to non-zero) for the first time, pass
                 * it through.
                 *
                 * For nested:
                 * The handling of the MSR bitmap for L2 guests is done in
                 * nested_vmx_merge_msr_bitmap. We should not touch the
                 * vmcs02.msr_bitmap here since it gets completely overwritten
                 * in the merging. We update the vmcs01 here for L1 as well
                 * since it will end up touching the MSR anyway now.
                 */
                vmx_disable_intercept_for_msr(vmx->vmcs01.msr_bitmap,
                                              MSR_IA32_SPEC_CTRL,
                                              MSR_TYPE_RW);

That means only the value of the first write in the guest is saved in vmx->spec_ctrl.

But what happens when a later write is different from the first write?

The host wouldn't notice it and force it back to the original value on 
the next VMENTRY, silently losing the right state.

Now with three different bits that can be set dynamically this is highly likely
that the first write is different than later writes.

On Linux it was ok before SSBD because the only bit that could be set was IBRS
or IBRS_ALL, but on other OS that use STIBP for only some processes it could also
lose the value. With MDD in the picture it can happen on Linux too.

Do I miss something here?

-Andi