Re: [PATCH] x86/spec-ctrl: Support for SRSO_US_NO and SRSO_MSR_FIX

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 26/03/2024 9:13 am, Jan Beulich wrote:
> On 25.03.2024 19:18, Andrew Cooper wrote:
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -2377,7 +2377,8 @@ By default SSBD will be mitigated at runtime (i.e `ssbd=runtime`).
>>  >              {msr-sc,rsb,verw,ibpb-entry}=<bool>|{pv,hvm}=<bool>,
>>  >              bti-thunk=retpoline|lfence|jmp, {ibrs,ibpb,ssbd,psfd,
>>  >              eager-fpu,l1d-flush,branch-harden,srb-lock,
>> ->              unpriv-mmio,gds-mit,div-scrub,lock-harden}=<bool> ]`
>> +>              unpriv-mmio,gds-mit,div-scrub,lock-harden,
>> +>              bp-spec-reduce}=<bool> ]`
>>  
>>  Controls for speculative execution sidechannel mitigations.  By default, Xen
>>  will pick the most appropriate mitigations based on compiled in support,
>> @@ -2509,6 +2510,12 @@ boolean can be used to force or prevent Xen from using speculation barriers to
>>  protect lock critical regions.  This mitigation won't be engaged by default,
>>  and needs to be explicitly enabled on the command line.
>>  
>> +On hardware supporting SRSO_MSR_FIX, the `bp-spec-reduce=` option can be used
>> +to force or prevent Xen from using MSR_BP_CFG.BP_SPEC_REDUCE to mitigate the
>> +SRSO (Speculative Return Stack Overflow) vulnerability.
> "... against HVM guests" to avoid things being left ambiguous, and to also ...
>
>>  By default, Xen will
>> +use bp-spec-reduce when available, as it preferable to using `ibpb-entry=hvm`
>> +to mitigate SRSO.
> ... correlate with the `ibpb-entry=hvm` here?

Ok.

> Maybe at the start of the paragraph also add "AMD"?

The rest of the text specifically doesn't mention vendors, in an attempt
to prevent it going stale.

Although now I re-read it, it makes the statements about microcode drops
ambiguous.

>
>> --- a/xen/arch/x86/cpu/amd.c
>> +++ b/xen/arch/x86/cpu/amd.c
>> @@ -1009,16 +1009,31 @@ static void cf_check fam17_disable_c6(void *arg)
>>  	wrmsrl(MSR_AMD_CSTATE_CFG, val & mask);
>>  }
>>  
>> -static void amd_check_erratum_1485(void)
>> +static void amd_check_bp_cfg(void)
>>  {
>> -	uint64_t val, chickenbit = (1 << 5);
>> +	uint64_t val, new = 0;
>>  
>> -	if (cpu_has_hypervisor || boot_cpu_data.x86 != 0x19 || !is_zen4_uarch())
>> +	/*
>> +	 * AMD Erratum #1485.  Set bit 5, as instructed.
>> +	 */
>> +	if (!cpu_has_hypervisor && boot_cpu_data.x86 == 0x19 && is_zen4_uarch())
>> +		new |= (1 << 5);
>> +
>> +	/*
>> +	 * On hardware supporting SRSO_MSR_FIX, we prefer BP_SPEC_REDUCE to
>> +	 * IBPB-on-entry to mitigate SRSO for HVM guests.
>> +	 */
>> +	if (IS_ENABLED(CONFIG_HVM) && boot_cpu_has(X86_FEATURE_SRSO_US_NO) &&
>> +            opt_bp_spec_reduce)
> Nit: Indentation is odd here (wants to be a tab followed by a few spaces).
>
>> +		new |= BP_CFG_SPEC_REDUCE;
> I take it that this goes from the assumption that it is deemed pretty unlikely
> that nowadays people would only run PV guests on a host? Otherwise, assuming
> that - like almost any such mitigation - its use costs performance, enabling
> the mitigation only as long as there are any HVM guests around might be better.

I have no idea what the perf cost is.  Native systems are expected not
to use this.

However, I have no care to try and make this dynamic based on having HVM
guests active.  For one, it would need a real spinlock to avoid the MSR
race, or a stop machine context, and anyone running PV guests on this
hardware really oughtn't to be.

[Edit, See later, but we need this for PV too]

>
>> +	/* Avoid reading BP_CFG if we don't intend to change anything. */
>> +	if (!new)
>>  		return;
>>  
>>  	rdmsrl(MSR_AMD64_BP_CFG, val);
>>  
>> -	if (val & chickenbit)
>> +	if ((val & new) == new)
>>  		return;
> Since bits may also need turning off:
>
> 	if (!((val ^ new) & (BP_CFG_SPEC_REDUCE | (1 << 5))))
> 		return;
>
> and the !new early-out dropped, too? Looks like this wasn't quite right
> before, either. 

That's adding unnecessary complexity.  It's unlikely that we'll ever
need to clear bits like this.

>
>> @@ -1078,22 +1082,41 @@ static void __init ibpb_calculations(void)
>>           * Confusion.  Mitigate with IBPB-on-entry.
>>           */
>>          if ( !boot_cpu_has(X86_FEATURE_BTC_NO) )
>> -            def_ibpb_entry = true;
>> +            def_ibpb_entry_pv = def_ibpb_entry_hvm = true;
>>  
>>          /*
>> -         * Further to BTC, Zen3/4 CPUs suffer from Speculative Return Stack
>> -         * Overflow in most configurations.  Mitigate with IBPB-on-entry if we
>> -         * have the microcode that makes this an effective option.
>> +         * Further to BTC, Zen3 and later CPUs suffer from Speculative Return
>> +         * Stack Overflow in most configurations.  Mitigate with IBPB-on-entry
>> +         * if we have the microcode that makes this an effective option,
>> +         * except where there are other mitigating factors available.
>>           */
> Hmm, is "Zen3 and later" really appropriate?

Yes.

SRSO isn't fixed until SRSO_NO is enumerated.

> Isn't your "speculative coding"
> remark related to perhaps both of the new bits becoming available on Zen5
> (meaning that the vulnerability would be limited to the guest/host boundary,
> as long as the MSR-based mitigation isn't used)?

Both of these are interim fixes.
>>          if ( !boot_cpu_has(X86_FEATURE_SRSO_NO) &&
>>               boot_cpu_has(X86_FEATURE_IBPB_BRTYPE) )
>> -            def_ibpb_entry = true;
>> +        {
>> +            /*
>> +             * SRSO_U/S_NO is a subset of SRSO_NO, identifying that SRSO isn't
>> +             * possible across the user/supervisor boundary.  We only need to
>> +             * use IBPB-on-entry for PV guests on hardware which doesn't
>> +             * enumerate SRSO_US_NO.
>> +             */
>> +            if ( !boot_cpu_has(X86_FEATURE_SRSO_US_NO) )
>> +                def_ibpb_entry_pv = true;
> opt_rsb_pv continues to take opt_pv32 into account, despite us having
> removed security support for 32-bit PV guests (by wording that's sadly a
> little ambiguous). Shouldn't that be done here too then, seeing that the
> flag only covers transitions from ring3?

Hmm, probably.

In practice, all these systems have CET so PV32 will be off-by-default,
but if people really want to run PV32 guests, we might as well do our best.

>
>> --- a/xen/include/public/arch-x86/cpufeatureset.h
>> +++ b/xen/include/public/arch-x86/cpufeatureset.h
>> @@ -304,7 +304,9 @@ XEN_CPUFEATURE(FSRSC,              11*32+19) /*A  Fast Short REP SCASB */
>>  XEN_CPUFEATURE(AMD_PREFETCHI,      11*32+20) /*A  PREFETCHIT{0,1} Instructions */
>>  XEN_CPUFEATURE(SBPB,               11*32+27) /*A  Selective Branch Predictor Barrier */
>>  XEN_CPUFEATURE(IBPB_BRTYPE,        11*32+28) /*A  IBPB flushes Branch Type predictions too */
>> -XEN_CPUFEATURE(SRSO_NO,            11*32+29) /*A  Hardware not vulenrable to Speculative Return Stack Overflow */
>> +XEN_CPUFEATURE(SRSO_NO,            11*32+29) /*A  Hardware not vulnerable to Speculative Return Stack Overflow */
>> +XEN_CPUFEATURE(SRSO_US_NO,         11*32+30) /*A  Hardware not vulnerable to SRSO across the User/Supervisor boundary */
> Can we validly expose this to 64-bit PV guests, where there's no CPL
> boundary? Or else isn't my "x86/PV: issue branch prediction barrier
> when switching 64-bit guest to kernel mode" needed as a prereq?

Based on uarch details, if BP-SPEC-REDUCE is active, we can advertise
SRSO_US_NO to PV guests.

But I doubt I'll be able to persuade AMD to put the safety details
behind this in writing.


I also missed the hunks adding SRSO_US_NO and SRSO_MSR_FIX to
print_details().  Fixed locally.

~Andrew