Re: [PATCH 8/8] x86/spec-ctrl: Mitigate the Zen1 DIV leakge

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/09/2023 11:52 am, Jan Beulich wrote:
> On 13.09.2023 22:27, Andrew Cooper wrote:
>> @@ -378,6 +392,8 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
>>      verw STACK_CPUINFO_FIELD(verw_sel)(%r14)
>>  .L\@_verw_skip:
>>  
>> +    ALTERNATIVE "", DO_SPEC_CTRL_DIV, X86_FEATURE_SC_DIV
>> +
>>  .L\@_skip_ist_exit:
>>  .endm
> While we did talk about using alternatives patching here, I'm now in
> doubt again, in particular because the rest of the macro uses
> conditionals anyway, and the code here is bypassed for non-IST exits. If
> you're sure you want to stick to this, then I think some justification
> wants adding to the description.

Patching IST paths is safe - we drop into NMI context, and rewrite
before starting APs.

VERW needs to remain a conditional because of the FB_CLEAR/MMIO paths. 
MSR_SPEC_CTRL is going to turn back into being an alternative when I
make eIBRS work sensibly.

>> @@ -955,6 +960,40 @@ static void __init srso_calculations(bool 
>> hw_smt_enabled)
>>          setup_force_cpu_cap(X86_FEATURE_SRSO_NO);
>>  }
>>  
>> +/*
>> + * Div leakage is specific to the AMD Zen1 microarchitecure.  Use STIBP as a
>> + * heuristic to select between Zen1 and Zen2 uarches.
>> + */
>> +static bool __init has_div_vuln(void)
>> +{
>> +    if ( !(boot_cpu_data.x86_vendor &
>> +           (X86_VENDOR_AMD | X86_VENDOR_HYGON)) )
>> +        return false;
>> +
>> +    if ( (boot_cpu_data.x86 != 0x17 && boot_cpu_data.x86 != 0x18) ||
>> +         !boot_cpu_has(X86_FEATURE_AMD_STIBP) )
>> +        return false;
>> +
>> +    return true;
>> +}
>> +
>> +static void __init div_calculations(bool hw_smt_enabled)
>> +{
>> +    bool cpu_bug_div = has_div_vuln();
>> +
>> +    if ( opt_div_scrub == -1 )
>> +        opt_div_scrub = cpu_bug_div;
>> +
>> +    if ( opt_div_scrub )
>> +        setup_force_cpu_cap(X86_FEATURE_SC_DIV);
> Isn't this only lowering performance (even if just slightly) when SMT is
> enabled, without gaining us very much?

It is consistent with how MDS/L1TF/others work, which is important.

And it does protect against a single-threaded attacker, for what that's
worth in practice.

>
>> +    if ( opt_smt == -1 && cpu_bug_div && hw_smt_enabled )
>> +        warning_add(
>> +            "Booted on leaky-DIV hardware with SMT/Hyperthreading\n"
>> +            "enabled.  Please assess your configuration and choose an\n"
>> +            "explicit 'smt=<bool>' setting.  See XSA-439.\n");
>> +}
> What about us running virtualized? The topology we see may not be that
> of the underlying hardware. Maybe check_smt_enabled() should return
> true when cpu_has_hypervisor is true? (In-guest decisions would
> similarly need to assume that they may be running on SMT-enabled
> hardware, despite not themselves seeing this to be the case.)
>
> Since we can't know for sure when running virtualized, that's a case
> where I would consider it useful to enable the workaround nevertheless
> (perhaps accompanied by a warning that whether this helps depends on
> the next level hypervisor).

Honestly, SMT's not relevant.  If you're virtualised, you've lost
irrespective.

There is no FOO_NO bit, so there's no possible way a VM can figure out
if it is current on, or may subsequently move to, an impacted processor.

~Andrew