ACK/CMNT: [SRU][Xenial][PATCH 2/3] UBUNTU: SAUCE: x86/speculation: Cleanup IBRS runtime control handling

Wed Dec 5 10:41:28 UTC 2018

On Tue, 4 Dec 2018 13:48:37 -0600
Tyler Hicks <tyhicks at canonical.com> wrote:

> On 2018-12-03 14:17:49, Juerg Haefliger wrote:
> > > > +#define ubuntu_restrict_branch_speculation_start()			\
> > > > +do {									\
> > > > +	u64 val = x86_spec_ctrl_base | SPEC_CTRL_IBRS;			\
> > > > +									\
> > > > +	if (ibrs_enabled)						\
> > > > +		native_wrmsrl(MSR_IA32_SPEC_CTRL, val);			\
> > > > +} while (0)
> > > > +
> > > > +#define ubuntu_restrict_branch_speculation_end()			\
> > > > +do {									\
> > > > +	u64 val = x86_spec_ctrl_base;					\
> > > > +									\
> > > > +	if (ibrs_enabled)						\
> > > > +		native_wrmsrl(MSR_IA32_SPEC_CTRL, val);			\
> > > > + } while (0)  
> > 
> > I just realized that the above is incorrect. If ibrs_enabled == 2 then we set
> > the IBRS bit on every CPU and should not fiddle with them here, no?
> > 
> > So it should be:
> >   if (ibrs_enabled == 1)
> >       native_wrmsrl(MSR_IA32_SPEC_CTRL, val);  
> 
> Yes, I think that's correct.

After running some more tests I take my earlier statement back. Primarily
because it would change the current behaviour. So I leave it as it is until
somebody reports an issue.

> > 
> >   
> > > >  	/*
> > > >  	 * Retpoline means the kernel is safe because it has no indirect
> > > >  	 * branches. Enhanced IBRS protects firmware too, so, enable restricted
> > > > @@ -463,9 +446,23 @@ specv2_set_mode:
> > > >  	 * the CPU supports Enhanced IBRS, kernel might un-intentionally not
> > > >  	 * enable IBRS around firmware calls.
> > > >  	 */
> > > > -	if (boot_cpu_has(X86_FEATURE_IBRS) && mode != SPECTRE_V2_IBRS_ENHANCED) {
> > > > -		setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
> > > > -		pr_info("Enabling Restricted Speculation for firmware calls\n");
> > > > +	if (boot_cpu_has(X86_FEATURE_IBRS)) {
> > > > +		if (mode != SPECTRE_V2_IBRS_ENHANCED) {
> > > > +			setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
> > > > +			pr_info("Enabling Restricted Speculation for firmware calls\n");
> > > > +		}
> > > > +		if (noibrs ||
> > > > +		    mode == SPECTRE_V2_RETPOLINE_GENERIC ||
> > > > +		    mode == SPECTRE_V2_RETPOLINE_AMD ||
> > > > +		    mode == SPECTRE_V2_IBRS_ENHANCED) {
> > > > +			/*
> > > > +			 * IBRS disabled via commandline or the kernel is
> > > > +			 * retpoline compiled or the HW supports enhanced IBRS
> > > > +			 */
> > > > +			set_ibrs_enabled(0);    
> > > 
> > > It took me a while to understand that IBRS Enhanced handling here (and
> > > then all the subtle details at runtime...). I now see that the IBRS bit
> > > of the IA32_SPEC_CTRL MSR is set earlier in this function if IBRS
> > > Enhanced is supported. Additionally, the SPEC_CTRL_IBRS bit is set in
> > > the x86_spec_ctrl_base bitmask. The above call to set_ibrs_enabled(0)
> > > ensures that ibrs_enabled is set to 0. That means that the IBRS bit of
> > > the IA32_SPEC_CTRL MSR will not be twiddled in future calls to
> > > ubuntu_restrict_branch_speculation_{start,end}() which is the correct
> > > behavior for IBRS Enhanced but it is a bit confusing since ibrs_enabled
> > > is 0.  
> > 
> > Hmm... The call set_ibrs_enabled(0) will clear the IBRS bit on all CPUs.
> > On hindsight, that seems wrong in the presence of enhanced IBRS. Maybe we
> > should disable the runtime control completely if mode ==
> > SPECTRE_V2_IBRS_ENHANCED?  
> 
> The call set_ibrs_enabled(0) won't clear the IBRS bit because
> x86_spec_ctrl_base has the SPEC_CTRL_IBRS bit set when Enhanced IBRS is
> supported.

Oh, right. Convoluted... :-(

> However, one of the main benefits of Enhanced IBRS is that
> you write to the MSR once and then don't have to toggle it at all so
> set_ibrs_enabled(0) will be inefficient.
> >   
> > > > +int set_ibrs_enabled(unsigned int val)
> > > > +{
> > > > +	int error = 0;
> > > > +	unsigned int cpu;
> > > > +
> > > > +	mutex_lock(&spec_ctrl_mutex);
> > > > +
> > > > +	/* Only enable/disable IBRS if the CPU supports it */
> > > > +	if (boot_cpu_has(X86_FEATURE_IBRS)) {
> > > > +		ibrs_enabled = val;
> > > > +		pr_info("Spectre V2 : Spectre v2 mitigation: %s Indirect "    
> > > 
> > > I think the "Spectre V2 : " portion of this message is redundant and is
> > > also not present in upstream's message which could confuse user or
> > > scripts that are looking at log messages. Can it be removed when
> > > applying the patch?  
> > 
> > Again, this is how upstream reported it until just recently.
> > 
> > 
> >    
> > > > +			"Branch Restricted Speculation%s\n",
> > > > +			ibrs_enabled ? "Enabling" : "Disabling",
> > > > +			ibrs_enabled == 2 ? " (user space)" : "");
> > > > +
> > > > +		if (ibrs_enabled == 0) {
> > > > +			/* Always disable IBRS */
> > > > +			u64 val = x86_spec_ctrl_base;
> > > > +
> > > > +			for_each_online_cpu(cpu) {
> > > > +				wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, val);
> > > > +			}
> > > > +		} else if (ibrs_enabled == 2) {
> > > > +			/* Always enable IBRS, even in user space */
> > > > +			u64 val = x86_spec_ctrl_base | SPEC_CTRL_IBRS;
> > > > +
> > > > +			for_each_online_cpu(cpu) {
> > > > +				wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, val);
> > > > +			}
> > > > +		}
> > > > +	} else {
> > > > +		ibrs_enabled = 0;
> > > > +		if (val) {
> > > > +			/* IBRS is not supported but we try to turn it on */
> > > > +			error = -EINVAL;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	/* Update the shadow variable */
> > > > +	__ibrs_enabled = ibrs_enabled;
> > > > +
> > > > +	mutex_unlock(&spec_ctrl_mutex);
> > > > +
> > > > +	return error;
> > > > +}
> > > > +
> > > > +static int ibrs_enabled_handler(struct ctl_table *table, int write,
> > > > +                               void __user *buffer, size_t *lenp,
> > > > +                               loff_t *ppos)
> > > > +{
> > > > +	int error;
> > > > +
> > > > +	error = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> > > > +	if (error)
> > > > +		return error;
> > > > +
> > > > +	return set_ibrs_enabled(__ibrs_enabled);
> > > > +}
> > > >  #endif    
> > > 
> > > IBRS Enhanced cannot be disabled with this sysctl since ibrs_enabled is
> > > already 0 and the corresponding bit is set in x86_spec_ctrl_base. I
> > > personally don't feel like this is a problem but I wanted to mention it.  
> > 
> > As mentioned above, with enhanced IBRS we can still fiddle with the IBRS bit
> > in the MSR via the runtime controls. We probably don't want that.  
>
> Now I'm thinking that it makes sense to be able to disable and enable
> Enhanced IBRS with the sysctl. Why wouldn't we want to support that?

Because it introduces another ubuntu-only-impossible-to-test-hard-to-maintain
feature with questionable benefit. Until there's a user request for it
I'm leaning towards disabling the runtime controls in Enhanced IBRS mode.

...Juerg

> Tyler

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20181205/00a728b1/attachment.sig>