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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.06.2024 22:20, Andrew Cooper wrote:
> 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.

Hmm, yes, sorry, I apparently lost track of that again.

>>> --- 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.

Well. Elsewhere we go to the lengths of trying to cover for unexpected
states. So to me this goes beyond "sufficient and simple". But anyway.

>> 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?

The fact that for HVM the change to ibpb_calculations() results in no
entry-IBPB when we have SRSO_MSR_FIX and opt_bp_spec_reduce is true.
To me the comment you add to calculate_pv_max_policy() suggests a
sufficient similarity. IOW ...

> 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.

... why is there an interaction for HVM but not for PV? (Sorry, I'm
apparently lost here to a certain degree.)

Jan