Re: [RFC][PATCH 06/17] x86/cpu: Add SRSO untrain to retbleed=

[Josh Poimboeuf <jpoimboe@kernel.org>]

On Wed, Aug 09, 2023 at 09:12:24AM +0200, Peter Zijlstra wrote:
>  static enum retbleed_mitigation retbleed_mitigation __ro_after_init =
> @@ -796,6 +802,10 @@ static int __init retbleed_parse_cmdline
>  			retbleed_cmd = RETBLEED_CMD_AUTO;
>  		} else if (!strcmp(str, "unret")) {
>  			retbleed_cmd = RETBLEED_CMD_UNRET;
> +		} else if (!strcmp(str, "srso")) {
> +			retbleed_cmd = RETBLEED_CMD_UNRET_SRSO;
> +		} else if (!strcmp(str, "srso_alias")) {
> +			retbleed_cmd = RETBLEED_CMD_UNRET_SRSO_ALIAS;

It doesn't make sense for "srso_alias" to be a separate cmdline option,
as that option is a model-dependent variant of the SRSO mitigation.

> +	if (boot_cpu_has_bug(X86_BUG_SRSO)) {
> +		has_microcode = boot_cpu_has(X86_FEATURE_IBPB_BRTYPE) || cpu_has_ibpb_brtype_microcode();
> +		if (!has_microcode) {
> +			pr_warn("IBPB-extending microcode not applied!\n");
> +			pr_warn(RETBLEED_SRSO_NOTICE);
> +		} else {
> +			/*
> +			 * Enable the synthetic (even if in a real CPUID leaf)
> +			 * flags for guests.
> +			 */
> +			setup_force_cpu_cap(X86_FEATURE_IBPB_BRTYPE);
> +			setup_force_cpu_cap(X86_FEATURE_SBPB);
> +
> +			/*
> +			 * Zen1/2 with SMT off aren't vulnerable after the right
> +			 * IBPB microcode has been applied.
> +			 */
> +			if ((boot_cpu_data.x86 < 0x19) &&
> +			    (cpu_smt_control == CPU_SMT_DISABLED))
> +				setup_force_cpu_cap(X86_FEATURE_SRSO_NO);

The rumor I heard was that SMT had to be disabled specifically by BIOS
for this condition to be true.  Can somebody from AMD confirm?

The above check doesn't even remotely do that, in fact it does the
opposite.  Regardless, at the very least it should be checking
cpu_smt_possible().  I guess that would be a separate patch as it's a
bug fix.

> @@ -870,8 +915,17 @@ static void __init retbleed_select_mitig
>  	default:
>  		if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
>  		    boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
> -			if (IS_ENABLED(CONFIG_CPU_UNRET_ENTRY))
> -				retbleed_mitigation = RETBLEED_MITIGATION_UNRET;
> +			if (IS_ENABLED(CONFIG_CPU_UNRET_ENTRY)) {
> +				if (boot_cpu_has_bug(X86_BUG_RETBLEED))
> +					retbleed_mitigation = RETBLEED_MITIGATION_UNRET;
> +
> +				if (boot_cpu_has_bug(X86_BUG_SRSO) && !boot_cpu_has(X86_FEATURE_SRSO_NO)) {
> +					if (boot_cpu_data.x86 == 0x19)
> +						retbleed_mitigation = RETBLEED_MITIGATION_UNRET_SRSO_ALIAS;
> +					else
> +						retbleed_mitigation = RETBLEED_MITIGATION_UNRET_SRSO;

It would be great to get confirmation from somebody at AMD that the SRSO
mitigations supersede the Retbleed one, i.e., that the SRSO mitigations
also fix Retbleed.

Yes, the original patches might imply that, but they also seem confused
since they do the double untraining for both Retbleed and SRSO.  And I
was given conflicting information.

Even better, we could really use some official AMD documentation for
this mitigation, since right now it all feels very unofficial and
ubsubstantiated.

> +				}
> +			}
>  			else if (IS_ENABLED(CONFIG_CPU_IBPB_ENTRY) && boot_cpu_has(X86_FEATURE_IBPB))
>  				retbleed_mitigation = RETBLEED_MITIGATION_IBPB;

Here should have the microcode check too:

				if (boot_cpu_has_bug(X86_BUG_SRSO) && !has_microcode)
					pr_err("IBPB-extending microcode not applied; SRSO NOT mitigated\n");


-- 
Josh