Re: [PATCH] powerpc/security: Fix Speculation_Store_Bypass reporting on Power10

[R Nageswara Sastry <rnsastry@linux.ibm.com>]

On 17/05/23 1:19 pm, Michael Ellerman wrote:
> Nageswara reported that /proc/self/status was showing "vulnerable" for
> the Speculation_Store_Bypass feature on Power10, eg:
> 
>    $ grep Speculation_Store_Bypass: /proc/self/status
>    Speculation_Store_Bypass:       vulnerable
> 
> But at the same time the sysfs files, and lscpu, were showing "Not
> affected".
> 
> This turns out to simply be a bug in the reporting of the
> Speculation_Store_Bypass, aka. PR_SPEC_STORE_BYPASS, case.
> 
> When SEC_FTR_STF_BARRIER was added, so that firmware could communicate
> the vulnerability was not present, the code in ssb_prctl_get() was not
> updated to check the new flag.
> 
> So add the check for SEC_FTR_STF_BARRIER being disabled. Rather than
> adding the new check to the existing if block and expanding the comment
> to cover both cases, rewrite the three cases to be separate so they can
> be commented separately for clarity.
> 
> Fixes: 84ed26fd00c5 ("powerpc/security: Add a security feature for STF barrier")
> Cc: stable@vger.kernel.org # v5.14+
> Reported-by: Nageswara R Sastry <rnsastry@linux.ibm.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

Thanks for the patch. Adding tested-by tag along with test results.

With out patch:
# grep Speculation_Store_Bypass: /proc/self/status
Speculation_Store_Bypass:	vulnerable
# uname -r
6.4.0-rc2

With patch:
# grep Speculation_Store_Bypass: /proc/self/status
Speculation_Store_Bypass:	not vulnerable
# uname -r
6.4.0-rc2

Tested-by: Nageswara R Sastry <rnsastry@linux.ibm.com>


> ---
>   arch/powerpc/kernel/security.c | 37 +++++++++++++++++-----------------
>   1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
> index 206475e3e0b4..4856e1a5161c 100644
> --- a/arch/powerpc/kernel/security.c
> +++ b/arch/powerpc/kernel/security.c
> @@ -364,26 +364,27 @@ ssize_t cpu_show_spec_store_bypass(struct device *dev, struct device_attribute *
>   
>   static int ssb_prctl_get(struct task_struct *task)
>   {
> +	/*
> +	 * The STF_BARRIER feature is on by default, so if it's off that means
> +	 * firmware has explicitly said the CPU is not vulnerable via either
> +	 * the hypercall or device tree.
> +	 */
> +	if (!security_ftr_enabled(SEC_FTR_STF_BARRIER))
> +		return PR_SPEC_NOT_AFFECTED;
> +
> +	/*
> +	 * If the system's CPU has no known barrier (see setup_stf_barrier())
> +	 * then assume that the CPU is not vulnerable.
> +	 */
>   	if (stf_enabled_flush_types == STF_BARRIER_NONE)
> -		/*
> -		 * We don't have an explicit signal from firmware that we're
> -		 * vulnerable or not, we only have certain CPU revisions that
> -		 * are known to be vulnerable.
> -		 *
> -		 * We assume that if we're on another CPU, where the barrier is
> -		 * NONE, then we are not vulnerable.
> -		 */
>   		return PR_SPEC_NOT_AFFECTED;
> -	else
> -		/*
> -		 * If we do have a barrier type then we are vulnerable. The
> -		 * barrier is not a global or per-process mitigation, so the
> -		 * only value we can report here is PR_SPEC_ENABLE, which
> -		 * appears as "vulnerable" in /proc.
> -		 */
> -		return PR_SPEC_ENABLE;
> -
> -	return -EINVAL;
> +
> +	/*
> +	 * Otherwise the CPU is vulnerable. The barrier is not a global or
> +	 * per-process mitigation, so the only value that can be reported here
> +	 * is PR_SPEC_ENABLE, which appears as "vulnerable" in /proc.
> +	 */
> +	return PR_SPEC_ENABLE;
>   }
>   
>   int arch_prctl_spec_ctrl_get(struct task_struct *task, unsigned long which)

-- 
Thanks and Regards
R.Nageswara Sastry