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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

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

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

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

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

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?

> +            /*
> +             * 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)?

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

Jan