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

[Juerg Haefliger]

> > +#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);


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


> > +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.

...Juerg
-------------- 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/20181203/01957c88/attachment-0001.sig>