Messages in this thread | | | Date | Thu, 12 Aug 2021 16:44:40 -0700 | From | Josh Poimboeuf <> | Subject | Re: [v6 1/1] x86/bugs: Implement mitigation for Predictive Store Forwarding |
| |
On Mon, May 17, 2021 at 05:00:58PM -0500, Ramakrishna Saripalli wrote: > From: Ramakrishna Saripalli <rk.saripalli@amd.com> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index 04545725f187..a5f694dccb24 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -3940,6 +3940,11 @@ > Format: {"off"} > Disable Hardware Transactional Memory > > + predictive_store_fwd_disable= [X86] This option controls PSF. > + off - Turns on PSF. > + on - Turns off PSF. > + default : off. > +
This needs a lot more text.
> +static const char * const psf_strings[] = { > + [PREDICTIVE_STORE_FORWARD_NONE] = "Vulnerable", > + [PREDICTIVE_STORE_FORWARD_DISABLE] = "Mitigation: Predictive Store Forward disabled",
This defaults to "Vulnerable", which is problematic for at least a few reasons.
1) I'm fairly sure this would be the first mitigation designed to default to "Vulnerable". Aside from whether that's a good idea, many users will be alarmed to see "Vulnerable" in sysfs.
2) If the system has the default per-process SSB mitigation (prctl/seccomp) then PSF will be automatically mitigated in the same way. In that case "Vulnerable" isn't an accurate description. (More on that below.)
> +static const struct { > + const char *option; > + enum psf_mitigation_cmd cmd; > +} psf_mitigation_options[] __initconst = { > + { "on", PREDICTIVE_STORE_FORWARD_CMD_ON }, /* Disable Speculative Store Bypass */ > + { "off", PREDICTIVE_STORE_FORWARD_CMD_NONE }, /* Don't touch Speculative Store Bypass */
Copy/paste error in the comments: "Speculative Store Bypass" -> "Predictive Store Forwarding"
I'd also recommend an "auto" option:
{ "auto", PREDICTIVE_STORE_FORWARD_CMD_AUTO }, /* Platform decides */
which would be the default. For now it would function the same as "off", but would give room for tweaking defaults later.
> +static enum psf_mitigation __init __psf_select_mitigation(void) > +{ > + enum psf_mitigation mode = PREDICTIVE_STORE_FORWARD_NONE; > + enum psf_mitigation_cmd cmd; > + > + if (!boot_cpu_has(X86_FEATURE_PSFD)) > + return mode; > + > + cmd = psf_parse_cmdline(); > + > + switch (cmd) { > + case PREDICTIVE_STORE_FORWARD_CMD_ON: > + mode = PREDICTIVE_STORE_FORWARD_DISABLE; > + break; > + default: > + mode = PREDICTIVE_STORE_FORWARD_NONE; > + break; > + } > + > + x86_spec_ctrl_mask |= SPEC_CTRL_PSFD;
A comment would help for this last line. I assume it's virt-related.
> + > + if (ssb_mode == SPEC_STORE_BYPASS_DISABLE) > + mode = PREDICTIVE_STORE_FORWARD_DISABLE; > + > + if (mode == PREDICTIVE_STORE_FORWARD_DISABLE) { > + setup_force_cpu_cap(X86_FEATURE_PSFD); > + x86_spec_ctrl_base |= SPEC_CTRL_PSFD; > + wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base); > + }
The PSF mitigation is (to some extent) dependent on the SSB mitigation, since turning off SSB implicitly turns off PSF. That should be reflected properly in sysfs for the prctl/seccomp cases. Here I'd propose adding something like:
} else if (ssb_mode == SPEC_STORE_BYPASS_PRCTL) { mode = PREDICTIVE_STORE_FORWARD_SSB_PRCTL; } else if (ssb_mode == SPEC_STORE_BYPASS_SECCOMP) { mode = PREDICTIVE_STORE_FORWARD_SSB_SECCOMP; }
And of course you'd need additional strings for those:
[PREDICTIVE_STORE_FORWARD_SSB_PRCTL] = "Mitigation: Predictive Store Forward disabled via SSB prctl", [PREDICTIVE_STORE_FORWARD_SSB_SECCOMP] = "Mitigation: Predictive Store Forward disabled via SSB seccomp",
-- Josh
| |