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;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Ramakrishna Saripalli <rsaripal@amd.com>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org, mingo@redhat.com,
        bp@alien8.de, hpa@zytor.com, Jonathan Corbet <corbet@lwn.net>,
        bsd@redhat.com, rsaripal@amd.com,
        Josh Poimboeuf <jpoimboe@redhat.com>
Subject: Re: [PATCH v5 1/1] x86/cpufeatures: Implement Predictive Store Forwarding control.
In-Reply-To: <20210505190923.276051-2-rsaripal@amd.com>
References: <20210505190923.276051-1-rsaripal@amd.com> <20210505190923.276051-2-rsaripal@amd.com>
Date:   Fri, 07 May 2021 17:13:33 +0200
Message-ID: <87wnsamvaa.ffs@nanos.tec.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Wed, May 05 2021 at 14:09, Ramakrishna Saripalli wrote:
> From: Ramakrishna Saripalli <rk.saripalli@amd.com>
>
> Certain AMD processors feature a new technology called Predictive Store
> Forwarding (PSF).
>
> PSF is a micro-architectural optimization designed to improve the
> performance of code execution by predicting dependencies between
> loads and stores.
>
> Incorrect PSF predictions can occur due to two reasons.
>
> - It is possible that the load/store pair may have had dependency for
>   a while but the dependency has stopped because the address in the
>   load/store pair has changed.
>
> - Second source of incorrect PSF prediction can occur because of an alias
>   in the PSF predictor structure stored in the microarchitectural state.
>   PSF predictor tracks load/store pair based on portions of instruction
>   pointer. It is possible that a load/store pair which does have a
>   dependency may be aliased by another load/store pair which does not have
>   the same dependency. This can result in incorrect speculation.
>
>   Software may be able to detect this aliasing and perform side-channel
>   attacks.

So this is the new post spectre/meltdown/.../ world order.

What would have been considered a potential speculative side channel bug
two years ago is now sold a feature which is by default enabled.

Just to be clear. From a security POV this is just yet another
default enabled speculative vulnerability. The difference to the others
is that this is communicated upfront and comes with a knob to turn it
off right away.

There is also interaction with SSB and the SSB mitigation which is
described in the cover letter, but not in the changelog and is not
detectable from user space.

I know that you had it implemented that way in your first attempt, but I
was busy with other things and missed the discussion which resulted in
this being treated as a feature.

TBH, I'm not really happy about this because that's inconsistent with
how we treat the other speculation related issues and there is no way
for user space to actually check this like the other one via /sys/....

> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -1170,3 +1170,22 @@ void set_dr_addr_mask(unsigned long mask, int dr)
>  		break;
>  	}
>  }
> +
> +static int __init psf_cmdline(char *str)
> +{
> +	if (!boot_cpu_has(X86_FEATURE_PSFD))
> +		return 0;
> +
> +	if (!str)
> +		return -EINVAL;
> +
> +	if (!strcmp(str, "off")) {
> +		set_cpu_cap(&boot_cpu_data, X86_FEATURE_MSR_SPEC_CTRL);

What? Why is this setting this feature here and why is this not done in
init_speculation_control() as for all the other speculation misfeatures?

> +		x86_spec_ctrl_base |= SPEC_CTRL_PSFD;

What? See below.

> +		msr_set_bit(MSR_IA32_SPEC_CTRL, SPEC_CTRL_PSFD_SHIFT);
> +	}
> +
> +	return 0;

So any parameter is treated as valid here. That's interesting at best.

> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -78,6 +78,8 @@ EXPORT_SYMBOL_GPL(mds_idle_clear);
>  
>  void __init check_bugs(void)
>  {
> +	u64 tmp = 0;
> +
>  	identify_boot_cpu();
>  
>  	/*
> @@ -97,7 +99,9 @@ void __init check_bugs(void)
>  	 * init code as it is not enumerated and depends on the family.
>  	 */
>  	if (boot_cpu_has(X86_FEATURE_MSR_SPEC_CTRL))
> -		rdmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
> +		rdmsrl(MSR_IA32_SPEC_CTRL, tmp);
> +
> +	x86_spec_ctrl_base |= tmp;

How is anyone supposed to understand that logic?

Just because x86_spec_ctrl_base is a global variable does not justify
this hackery at all. It's just a matter of time that someone reads this:

u64 x86_spec_ctrl_base;

void __init check_bugs(void)
{
	u64 tmp = 0;

        ...

  	if (boot_cpu_has(X86_FEATURE_MSR_SPEC_CTRL))
		rdmsrl(MSR_IA32_SPEC_CTRL, tmp);

	x86_spec_ctrl_base |= tmp;

and figures that this is a pointless exercise and reverts that hunk.

What's wrong with just treating this in the same way in which we treat
all other speculative vulnerabilities and provide a consistent picture
to the user?

Something like the below. You get the idea.

Thanks,

        tglx
---
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -111,6 +111,7 @@ void __init check_bugs(void)
 	mds_select_mitigation();
 	taa_select_mitigation();
 	srbds_select_mitigation();
+	psf_select_mitigation();
 
 	/*
 	 * As MDS and TAA mitigations are inter-related, print MDS
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -891,6 +891,9 @@ static void init_speculation_control(str
 		set_cpu_cap(c, X86_FEATURE_MSR_SPEC_CTRL);
 		clear_cpu_cap(c, X86_FEATURE_VIRT_SSBD);
 	}
+
+	if (!boot_cpu_has(X86_FEATURE_PSFD))
+		set_cpu_cap(c, X86_FEATURE_MSR_SPEC_CTRL);
 }
 
 void get_cpu_cap(struct cpuinfo_x86 *c)