Re: [PATCH 2/2] x86: Activate Data Operand Invariant Timing Mode by default

[Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Oct 04, 2022 at 05:08:10PM +0100, Andrew Cooper wrote:
> Intel IceLake and later CPUs have microarchitectural behaviours which cause
> data-dependent timing behaviour.  This is not an issue for 99% of software,
> but it is a problem for cryptography routines.  On these CPUs, a new
> architectural feature, DOITM, was retrofitted in microcode.
> 
> For now, Xen can't enumerate DOITM to guest kernels; getting this working is
> still in progress.  The consequence is that guest kernels will incorrectly
> conclude that they are safe.
> 
> To maintain the safety of current software, activate DOITM unilaterally.  This
> will be relaxed in the future when we can enumerate the feature properly to
> guests.
> 
> As an emergency stopgap, this behaviour can be disabled by specifying
> `cpuid=no-doitm` on Xen's command line, but is not guaranteed ABI moving
> forward.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Henry Wang <Henry.Wang@xxxxxxx>
> CC: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> CC: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
> ---
>  xen/arch/x86/cpu/common.c            | 29 +++++++++++++++++++++++++++++
>  xen/arch/x86/cpuid.c                 |  5 +++++
>  xen/arch/x86/include/asm/processor.h |  2 ++
>  xen/tools/gen-cpuid.py               |  2 ++
>  4 files changed, 38 insertions(+)
> 
> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
> index 0412dbc915e5..8c46a4db430a 100644
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -209,6 +209,34 @@ void ctxt_switch_levelling(const struct vcpu *next)
>               alternative_vcall(ctxt_switch_masking, next);
>  }
>  
> +bool __ro_after_init opt_doitm = true;
> +
> +static void doitm_init(void)
> +{
> +    uint64_t val;
> +
> +    if ( !opt_doitm || !cpu_has_arch_caps )
> +        return;
> +
> +    rdmsrl(MSR_ARCH_CAPABILITIES, val);
> +    if ( !(val & ARCH_CAPS_DOITM) )
> +        return;
> +
> +    /*
> +     * We are currently unable to enumerate MSR_ARCH_CAPS to guest.  As a
> +     * consequence, guest kernels will believe they're safe even when they 
> are
> +     * not.
> +     *
> +     * Until we can enumerate DOITM properly for guests, set it unilaterally.
> +     * This prevents otherwise-correct crypto from becoming vulnerable to
> +     * timing sidechannels.
> +     */
> +
> +    rdmsrl(MSR_UARCH_MISC_CTRL, val);
> +    val |= UARCH_CTRL_DOITM;
> +    wrmsrl(MSR_UARCH_MISC_CTRL, val);
> +}
> +
>  bool_t opt_cpu_info;
>  boolean_param("cpuinfo", opt_cpu_info);
>  
> @@ -532,6 +560,7 @@ void identify_cpu(struct cpuinfo_x86 *c)
>       /* Now the feature flags better reflect actual CPU features! */
>  
>       xstate_init(c);
> +     doitm_init();
>  
>  #ifdef NOISY_CAPS
>       printk(KERN_DEBUG "CPU: After all inits, caps:");
> diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
> index 112ee63a9449..09c1ee18fd95 100644
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -106,7 +106,12 @@ static void __init cf_check _parse_xen_cpuid(
>      const char *name, unsigned int feat, bool val)
>  {
>      if ( unlikely(feat == ~0u) )
> +    {
> +        if ( strcmp(name, "doitm") == 0 )
> +            opt_doitm = val;
> +
>          return;
> +    }
>  
>      if ( !val )
>          setup_clear_cpu_cap(feat);
> diff --git a/xen/arch/x86/include/asm/processor.h 
> b/xen/arch/x86/include/asm/processor.h
> index 8e2816fae9b9..2978416e6c5b 100644
> --- a/xen/arch/x86/include/asm/processor.h
> +++ b/xen/arch/x86/include/asm/processor.h
> @@ -637,6 +637,8 @@ enum ap_boot_method {
>  };
>  extern enum ap_boot_method ap_boot_method;
>  
> +extern bool opt_doitm;
> +
>  #endif /* !__ASSEMBLY__ */
>  
>  #endif /* __ASM_X86_PROCESSOR_H */
> diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
> index f3045b3bfd36..78a3a5c1941f 100755
> --- a/xen/tools/gen-cpuid.py
> +++ b/xen/tools/gen-cpuid.py
> @@ -303,6 +303,8 @@ def crunch_numbers(state):
>      # specially
>      #
>      pseduo_names = (
> +        # Data Operand Invariant Timing Mode.  Lives in MSR_ARCH_CAPS
> +        "doitm",
>      )
>  
>      for n in pseduo_names:
> -- 
> 2.11.0

I can’t review the actual implementation, but the idea looks good to me.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature