Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 12 Aug 2021 16:44:40 -0700
From:   Josh Poimboeuf <jpoimboe@redhat.com>
To:     Ramakrishna Saripalli <rsaripal@amd.com>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org, tglx@linutronix.de,
        mingo@redhat.com, bp@alien8.de, hpa@zytor.com,
        Jonathan Corbet <corbet@lwn.net>, bsd@redhat.com
Subject: Re: [v6 1/1] x86/bugs: Implement mitigation for Predictive Store
 Forwarding
Message-ID: <20210812234440.tcssf2iqs435bgdo@treble>
References: <20210517220059.6452-1-rsaripal@amd.com>
 <20210517220059.6452-2-rsaripal@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20210517220059.6452-2-rsaripal@amd.com>
Precedence: bulk

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