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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/09/2023 12:15 pm, Jan Beulich wrote:
> On 15.09.2023 17:00, Andrew Cooper wrote:
>> In the Zen1 microarchitecure, there is one divider in the pipeline which
>> services uops from both threads.  In the case of #DE, the latched result from
>> the previous DIV to execute will be forwarded speculatively.
>>
>> This is an interesting covert channel that allows two threads to communicate
>> without any system calls.  In also allows userspace to obtain the result of
>> the most recent DIV instruction executed (even speculatively) in the core,
>> which can be from a higher privilege context.
>>
>> Scrub the result from the divider by executing a non-faulting divide.  This
>> needs performing on the exit-to-guest paths, and ist_exit-to-Xen.
>>
>> This is XSA-439 / CVE-2023-20588.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> Nevertheless I would have hoped you add at least a sentence on the 
> alternatives
> patching of the IST path. Hitting #MC while patching is possible, after all
> (yes, you will tell me that #MC is almost certainly fatal to the system 
> anyway,
> but still).

I'll see what I can do.

>
>> @@ -955,6 +960,46 @@ static void __init srso_calculations(bool 
>> hw_smt_enabled)
>>          setup_force_cpu_cap(X86_FEATURE_SRSO_NO);
>>  }
>>  
>> +/*
>> + * The Div leakage issue is specific to the AMD Zen1 microarchitecure.
>> + *
>> + * However, there's no $FOO_NO bit defined, so if we're virtualised we have 
>> no
>> + * hope of spotting the case where we might move to vulnerable hardware.  We
>> + * also can't make any useful conclusion about SMT-ness.
>> + *
>> + * Don't check the hypervisor bit, so at least we do the safe thing when
>> + * booting on something that looks like a Zen1 CPU.
>> + */
>> +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) ||
>> +         !is_zen1_uarch() )
>> +        return false;
>> +
>> +    return true;
>> +}
> Just to mention it - personally I consider
>
>     ...
>     if ( ... )
>         return true;
>
>     return false;
> }
>
> a minor anti-pattern, as a sole return imo makes more clear what's going on.

Well yes, here is an area where we disagree.

It's the same as trailing commas on lists, or "| 0)" for bitmaps for
making a smaller delta for future changes.

> In a case like this, where you intentionally split return paths anyway, I'd
> then go with
>
> 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 )
>         return false;
>
>     return is_zen1_uarch();
> }

I'll swap to this because there is no realistic chance of the logic
chain needing to expand.

~Andrew