Re: [PATCH v3 18/35] x86/bugs: Restructure srso mitigation

[Josh Poimboeuf]

On Wed, Jan 08, 2025 at 02:24:58PM -0600, David Kaplan wrote:
> @@ -2749,98 +2741,98 @@ static void __init srso_select_mitigation(void)
>	if (has_microcode) {
>		/*
>		 * Zen1/2 with SMT off aren't vulnerable after the right
>		 * IBPB microcode has been applied.
>		 *
>		 * Zen1/2 don't have SBPB, no need to try to enable it here.
>		 */

This second paragraph no longer applies here since enablement isn't done
in this function anyway.

>		if (boot_cpu_data.x86 < 0x19 && !cpu_smt_possible()) {
>			setup_force_cpu_cap(X86_FEATURE_SRSO_NO);
>			return;
>		}

This should also set 'srso_mitigation = SRSO_MITIGATION_NONE', otherwise
it will end up applying the mitigation.


> +	switch (srso_mitigation) {
> +	case SRSO_MITIGATION_MICROCODE:
>  		break;

The switch statement has a default case so this one isn't needed.

>  
> +	case SRSO_MITIGATION_SAFE_RET:
> +	case SRSO_MITIGATION_SAFE_RET_UCODE_NEEDED:
> +		if (!IS_ENABLED(CONFIG_MITIGATION_SRSO))
>  			pr_err("WARNING: kernel not compiled with MITIGATION_SRSO.\n");
> -		}
> +		else if (boot_cpu_has(X86_FEATURE_SRSO_USER_KERNEL_NO))
> +			srso_mitigation = SRSO_MITIGATION_IBPB_ON_VMEXIT;

This misses the below SRSO_MITIGATION_IBPB_ON_VMEXIT check for
CONFIG_MITIGATION_SRSO.

Though, that doesn't make any sense.  What they really need to be
checking for is CONFIG_MITIGATION_IBPB_ENTRY.

> +	case SRSO_MITIGATION_IBPB_ON_VMEXIT:
> +		if (!IS_ENABLED(CONFIG_MITIGATION_SRSO))
>  			pr_err("WARNING: kernel not compiled with MITIGATION_SRSO.\n");
> -                }
> +		break;

This is an existing bug, but as mentioned above this should be checking
for CONFIG_MITIGATION_IBPB_ENTRY instead of CONFIG_MITIGATION_SRSO.

> +static void __init srso_update_mitigation(void)
> +{
> +	/* If retbleed is using IBPB, that works for SRSO as well */
> +	if (retbleed_mitigation == RETBLEED_MITIGATION_IBPB)
> +		srso_mitigation = SRSO_MITIGATION_IBPB;

Another dependency on retbleed_update_mitigation().

> +	if (srso_mitigation != SRSO_MITIGATION_NONE)
> +		pr_info("%s\n", srso_strings[srso_mitigation]);
> +}

For consistency with others this should probably be something like

	if (boot_cpu_has_bug(X86_BUG_SRSO) && !cpu_migitations_off())
		pr_info("%s\n", srso_strings[srso_mitigation]);

> +		setup_clear_cpu_cap(X86_FEATURE_RSB_VMEXIT);
>  		break;
>  	default:
>  		break;
>  	}
>  
> -out:
> -	pr_info("%s\n", srso_strings[srso_mitigation]);
>  }

Extra whitespace.

-- 
Josh