Re: [PATCH] powerpc/security: Fix spectre_v2 reporting

[Michael Neuling <mikey@neuling.org>]

On Thu, 2019-03-21 at 15:24 +1100, Michael Ellerman wrote:
> When I updated the spectre_v2 reporting to handle software count cache
> flush I got the logic wrong when there's no software count cache
> enabled at all.
> 
> The result is that on systems with the software count cache flush
> disabled we print:
> 
>   Mitigation: Indirect branch cache disabled, Software count cache flush
> 
> Which correctly indicates that the count cache is disabled, but
> incorrectly says the software count cache flush is enabled.
> 
> The root of the problem is that we are trying to handle all
> combinations of options. But we know now that we only expect to see
> the software count cache flush enabled if the other options are false.
> 
> So split the two cases, which simplifies the logic and fixes the bug.
> We were also missing a space before "(hardware accelerated)".
> 
> The result is we see one of:
> 
>   Mitigation: Indirect branch serialisation (kernel only)
>   Mitigation: Indirect branch cache disabled
>   Mitigation: Software count cache flush
>   Mitigation: Software count cache flush (hardware accelerated)
> 
> Fixes: ee13cb249fab ("powerpc/64s: Add support for software count cache flush")
> Cc: stable@vger.kernel.org # v4.19+
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

LGTM

Reviewed-by: Michael Neuling <mikey@neuling.org>

> ---
>  arch/powerpc/kernel/security.c | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
> index 9b8631533e02..b33bafb8fcea 100644
> --- a/arch/powerpc/kernel/security.c
> +++ b/arch/powerpc/kernel/security.c
> @@ -190,29 +190,22 @@ ssize_t cpu_show_spectre_v2(struct device *dev, struct device_attribute *attr, c
>  	bcs = security_ftr_enabled(SEC_FTR_BCCTRL_SERIALISED);
>  	ccd = security_ftr_enabled(SEC_FTR_COUNT_CACHE_DISABLED);
>  
> -	if (bcs || ccd || count_cache_flush_type != COUNT_CACHE_FLUSH_NONE) {
> -		bool comma = false;
> +	if (bcs || ccd) {
>  		seq_buf_printf(&s, "Mitigation: ");
>  
> -		if (bcs) {
> +		if (bcs)
>  			seq_buf_printf(&s, "Indirect branch serialisation (kernel only)");
> -			comma = true;
> -		}
>  
> -		if (ccd) {
> -			if (comma)
> -				seq_buf_printf(&s, ", ");
> -			seq_buf_printf(&s, "Indirect branch cache disabled");
> -			comma = true;
> -		}
> -
> -		if (comma)
> +		if (bcs && ccd)
>  			seq_buf_printf(&s, ", ");
>  
> -		seq_buf_printf(&s, "Software count cache flush");
> +		if (ccd)
> +			seq_buf_printf(&s, "Indirect branch cache disabled");
> +	} else if (count_cache_flush_type != COUNT_CACHE_FLUSH_NONE) {
> +		seq_buf_printf(&s, "Mitigation: Software count cache flush");
>  
>  		if (count_cache_flush_type == COUNT_CACHE_FLUSH_HW)
> -			seq_buf_printf(&s, "(hardware accelerated)");
> +			seq_buf_printf(&s, " (hardware accelerated)");
>  	} else if (btb_flush_enabled) {
>  		seq_buf_printf(&s, "Mitigation: Branch predictor state flush");
>  	} else {