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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

> @@ -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.
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();
}

Jan