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

[Tyler Hicks]

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.

> 
> 
> > >  	/*
> > >  	 * 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. 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?

Tyler
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20181204/5092b6bd/attachment.sig>