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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 20/06/2024 9:39 am, Jan Beulich wrote:
> On 19.06.2024 21:10, Andrew Cooper wrote:
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -2390,7 +2390,7 @@ By default SSBD will be mitigated at runtime (i.e 
>> `ssbd=runtime`).
>>  >              {ibrs,ibpb,ssbd,psfd,
>>  >              eager-fpu,l1d-flush,branch-harden,srb-lock,
>>  >              unpriv-mmio,gds-mit,div-scrub,lock-harden,
>> ->              bhi-dis-s}=<bool> ]`
>> +>              bhi-dis-s,bp-spec-reduce}=<bool> ]`
>>  
>>  Controls for speculative execution sidechannel mitigations.  By default, Xen
>>  will pick the most appropriate mitigations based on compiled in support,
>> @@ -2539,6 +2539,13 @@ 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.
> Upon my request to add "... against HVM guests" here you replied "Ok.", yet
> then you didn't make the change? Even a changelog entry says you supposedly
> added this, so perhaps simply lost in a refresh?

Yes, and later in the thread you (correctly) pointed out that it's not
only for HVM guests.

The PV aspect, discussed in the following sentence, is very relevant and
makes it not specific to HVM guests.

> It also didn't really become clear to me whether the entirety of your reply
> to my request for adding "AMD" early in the sentence wasn't boiling down
> more to a "yes, perhaps".

It was a "no I'm not going to introduce an inconsistency with the way
this document has been written for the past 7 years".


>
>> @@ -605,6 +606,24 @@ static void __init calculate_pv_max_policy(void)
>>          __clear_bit(X86_FEATURE_IBRS, fs);
>>      }
>>  
>> +    /*
>> +     * SRSO_U/S_NO means that the CPU is not vulnerable to SRSO attacks 
>> across
>> +     * the User (CPL3)/Supervisor (CPL<3) boundary.  However the PV64
>> +     * user/kernel boundary is CPL3 on both sides, so it won't convey the
>> +     * meaning that a PV kernel expects.
>> +     *
>> +     * PV32 guests are explicitly unsupported WRT speculative safety, so are
>> +     * ignored to avoid complicating the logic.
>> +     *
>> +     * After discussions with AMD, it is believed to be safe to offer
>> +     * SRSO_US_NO to PV guests when BP_SPEC_REDUCE is active.
> IOW that specific behavior is not tied to #VMEXIT / VMRUN, and also isn't
> going to in future hardware?

This is the best I can say in public.

I am satisfied with the explanation.
>> --- a/xen/arch/x86/cpu/amd.c
>> +++ b/xen/arch/x86/cpu/amd.c
>> @@ -1009,16 +1009,33 @@ 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, activate BP_SPEC_REDUCE by
>> +     * default.  This lets us do two things:
>> +         *
>> +         * 1) Avoid IBPB-on-entry to mitigate SRSO attacks from HVM guests.
>> +         * 2) Lets us advertise SRSO_US_NO to PV guests.
>> +     */
>> +    if (boot_cpu_has(X86_FEATURE_SRSO_MSR_FIX) && opt_bp_spec_reduce)
>> +            new |= BP_CFG_SPEC_REDUCE;
>> +
>> +    /* 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;
> You explained that you want to avoid making this more complex, upon my
> question towards tweaking this to also deal with us possibly clearing
> flags. I'm okay with that, but I did ask that you add at least half a
> sentence to actually clarify this to future readers (which might include
> myself).

"I wrote it this way because it is sufficient and simple, but you can
change it in the future if you really need to" is line noise wherever
it's written.

It literally goes without saying, for every line the entire codebase.

>
>> @@ -1145,22 +1151,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.
>>           */
>>          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;
> To my PV32 related comment here you said "..., we might as well do our best".
> Yet nothing has changed here?

Correct.

First, because the AMD Whitepaper actually says CPL0 and CPL3, not
User/Supervisor (the latter is only implied from the name AMD gave it).

Second, because I thought though what it would actually take to make
this work for PV32 which includes...

>  Yet then, thinking about it again, trying to
> help PV32 would apparently mean splitting def_ibpb_entry_pv and hence, via
> opt_ibpb_entry_pv, X86_FEATURE_IBPB_ENTRY_PV (and perhaps yet more items). I
> guess the resulting complexity then simply isn't worth it.

... the fact that Xen doesn't know if a PV guest is going to be PV32 or
PV64 until after the toolstack has partially constructed the domain,
including choosing it's "default" policy.

This ends up in a bizarre case where PV32 is more featureful than PV64,
(SRSO_US_NO does not depend on BP_SPEC_REDUCE), and nothing in the CPUID
hanlding logic can cope nicely with this.

Here, you end up in the weird situation where it's safe to always pass
the hardware SRSO_U/S_NO bit through into PV32, yet it's not

> However, as an unrelated aspect: According to the respective part of the
> comment you add to  calculate_pv_max_policy(), do we need the IBPB when
> BP_SPEC_REDUCE is active?

To what are you referring?

SRSO is about Return predictions becoming poisoned by other
predictions.  The best way to mount an SRSO attack is with forged
near-branch predictions.

For SRSO safety, we use IBPB to flush the Branch *Type* information from
the BTB.  Fam17h happened to have this property, but Fam19h needed it
retrofitting in a microcode update, with the prior "Indirect Branch
Targets only, explicitly retaining the Branch Type information" being
retroactively named SBPB.

This does not interact with the use of IBPB for it's
architecturally-given purpose.

>> +            /*
>> +             * SRSO_MSR_FIX enumerates that we can use 
>> MSR_BP_CFG.SPEC_REDUCE
>> +             * to mitigate SRSO across the host/guest boundary.  We only 
>> need
>> +             * to use IBPB-on-entry for HVM guests if we haven't enabled 
>> this
>> +             * control.
>> +             */
>> +            if ( !boot_cpu_has(X86_FEATURE_SRSO_MSR_FIX) || 
>> !opt_bp_spec_reduce )
>> +                def_ibpb_entry_hvm = true;
> Here and elsewhere, wouldn't conditionals become simpler if you (early on)
> cleared opt_bp_spec_reduce when !boot_cpu_has(X86_FEATURE_SRSO_MSR_FIX)?

I don't think so, no.  The uses are all at different phases of
initialisation.

>
>> --- a/xen/include/public/arch-x86/cpufeatureset.h
>> +++ b/xen/include/public/arch-x86/cpufeatureset.h
>> @@ -312,7 +312,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 */
> Nit: Elsewhere we have ! first, and I think that's preferable, to avoid
> confusion with | (which normally comes last).

Ok.

~Andrew