Re: [v6 1/1] x86/bugs: Implement mitigation for Predictive Store Forwarding

From: Josh Poimboeuf
Date: Thu Aug 12 2021 - 19:44:49 EST


On Mon, May 17, 2021 at 05:00:58PM -0500, Ramakrishna Saripalli wrote:
> From: Ramakrishna Saripalli <rk.saripalli@xxxxxxx>
> 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