Re: [PATCH v4 for-4.18?] x86: support data operand independent timing mode

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Oct 20, 2023 at 07:53:51AM +0200, Jan Beulich wrote:
> [1] specifies a long list of instructions which are intended to exhibit
> timing behavior independent of the data they operate on. On certain
> hardware this independence is optional, controlled by a bit in a new
> MSR. Provide a command line option to control the mode Xen and its
> guests are to operate in, with a build time control over the default.
> Longer term we may want to allow guests to control this.

Couldn't we just expose DOITM in MSR_ARCH_CAPS (by setting the A flag
in the feature enumeration) and handle accesses to
MSR_UARCH_MISC_CTRL?

The complications would be with the leveling of the feature across a
pool, as AFAICT the bit needs to be OR'ed rather than AND'ed across
hosts.

It would also then need some special handling in order to allow
reporting DOITM on hardware that doesn't have the feature (and writes
to MSR_UARCH_MISC_CTRL won't be forwarded to hardware in that case).

>From an implementation PoV we might want to treat this as SSBD, and
allow Xen to run with the guest selection.

Anyway, likely much more than what you want to do

> Since Arm64 supposedly also has such a control, put command line option
> and Kconfig control in common files.
> 
> [1] 
> https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/best-practices/data-operand-independent-timing-isa-guidance.html
> 
> Requested-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> Release-acked-by: Henry Wang <Henry.Wang@xxxxxxx>

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

The only concern I have is with the Kconfig and command line options
being named DIT instead of DOIT, which would be what I would search
for given the documentation from Intel.  Maybe that's for unification
with Arm?

> ---
> This may be viewed as a new feature, and hence be too late for 4.18. It
> may, however, also be viewed as security relevant, which is why I'd like
> to propose to at least consider it. Note that already for 4.17 it had a
> release ack to go in late; just the necessary normal ack continues to be
> missing.
> 
> Slightly RFC, in particular for whether the Kconfig option should
> default to Y or N.
> 
> I would have wanted to invoke setup_doitm() from cpu_init(), but that
> works only on the BSP. On APs cpu_init() runs before ucode loading.
> Plus recheck_cpu_features() invoking identify_cpu() takes care of the
> BSP during S3 resume.

As I wrote below, we do not seem to be very consistent with where are
such setup functions called.  Would be good if we could unify all the
CPU initialization in a single function, as that could avoid
confusion, and mistakes with forgetting initialization in the resume
path.

> ---
> v4: Re-base.
> v3: Extend command line doc. Add changelog entry.
> v2: Introduce and use cpu_has_doitm. Add comment "borrowed" from the
>     XenServer patch queue patch providing similar functionality.
>     Re-base.
> 
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -33,6 +33,8 @@ The format is based on [Keep a Changelog
>     nodes using a device tree overlay binary (.dtbo).
>   - Introduce two new hypercalls to map the vCPU runstate and time areas by
>     physical rather than linear/virtual addresses.
> + - On x86, support for enforcing system-wide operation in Data Operand
> +   Independent Timing Mode.
>  
>  ### Removed
>   - On x86, the "pku" command line option has been removed.  It has never
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -788,6 +788,16 @@ Specify the size of the console debug tr
>  additionally a trace buffer of the specified size is allocated per cpu.
>  The debug trace feature is only enabled in debugging builds of Xen.
>  
> +### dit (x86/Intel)

Why DIT and not DOIT?  That seems to be the acronym used in Intel
docs, and what I would use to grep for in the command line document.

> +> `= <boolean>`
> +
> +> Default: `CONFIG_DIT_DEFAULT`
> +
> +Specify whether Xen and guests should operate in Data Independent Timing
> +mode. Note that enabling this option cannot guarantee anything beyond what
> +underlying hardware guarantees (with, where available and known to Xen,
> +respective tweaks applied).
> +
>  ### dma_bits
>  > `= <integer>`
>  
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -15,6 +15,7 @@ config X86
>       select HAS_ALTERNATIVE
>       select HAS_COMPAT
>       select HAS_CPUFREQ
> +     select HAS_DIT
>       select HAS_EHCI
>       select HAS_EX_TABLE
>       select HAS_FAST_MULTIPLY
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -204,6 +204,28 @@ void ctxt_switch_levelling(const struct
>               alternative_vcall(ctxt_switch_masking, next);
>  }
>  
> +static void setup_doitm(void)
> +{
> +    uint64_t msr;
> +
> +    if ( !cpu_has_doitm )
> +        return;
> +
> +    /*
> +     * We don't currently enumerate DOITM to guests.  As a conseqeuence, 
> guest
> +     * kernels will believe they're safe even when they are not.
> +     *
> +     * For now, set it unilaterally.  This prevents otherwise-correct crypto
> +     * code from becoming vulnerable to timing sidechannels.
> +     */
> +
> +    rdmsrl(MSR_UARCH_MISC_CTRL, msr);
> +    msr |= UARCH_CTRL_DOITM;
> +    if ( !opt_dit )
> +        msr &= ~UARCH_CTRL_DOITM;
> +    wrmsrl(MSR_UARCH_MISC_CTRL, msr);
> +}
> +
>  bool opt_cpu_info;
>  boolean_param("cpuinfo", opt_cpu_info);
>  
> @@ -599,6 +621,8 @@ void identify_cpu(struct cpuinfo_x86 *c)
>  
>               mtrr_bp_init();
>       }
> +
> +     setup_doitm();

We do not seem to to be very consistent about where features are
enabled, we do seem to have a tail of features that could also likely
be part of identify_cpu()? tsx_init(), update_mcu_opt_ctrl(),
MSR_SPEC_CTRL.

>  }
>  
>  /* leaf 0xb SMT level */
> --- a/xen/arch/x86/include/asm/cpufeature.h
> +++ b/xen/arch/x86/include/asm/cpufeature.h
> @@ -202,6 +202,7 @@ static inline bool boot_cpu_has(unsigned
>  #define cpu_has_tsx_ctrl        boot_cpu_has(X86_FEATURE_TSX_CTRL)
>  #define cpu_has_taa_no          boot_cpu_has(X86_FEATURE_TAA_NO)
>  #define cpu_has_mcu_ctrl        boot_cpu_has(X86_FEATURE_MCU_CTRL)
> +#define cpu_has_doitm           boot_cpu_has(X86_FEATURE_DOITM)
>  #define cpu_has_fb_clear        boot_cpu_has(X86_FEATURE_FB_CLEAR)
>  #define cpu_has_rrsba           boot_cpu_has(X86_FEATURE_RRSBA)
>  #define cpu_has_gds_ctrl        boot_cpu_has(X86_FEATURE_GDS_CTRL)
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -56,6 +56,9 @@ config HAS_COMPAT
>  config HAS_DEVICE_TREE
>       bool
>  
> +config HAS_DIT # Data Independent Timing
> +     bool
> +
>  config HAS_EX_TABLE
>       bool
>  
> @@ -187,6 +190,18 @@ config SPECULATIVE_HARDEN_GUEST_ACCESS
>  
>  endmenu
>  
> +config DIT_DEFAULT
> +     bool "Data Independent Timing default"
> +     depends on HAS_DIT
> +     help
> +       Hardware often surfaces instructions the timing of which is dependent
> +       on the data they process.  Some of these instructions may be used in
> +       timing sensitive environments, e.g. cryptography.  When such
> +       instructions exist, hardware may further surface a control allowing
> +       to make the behavior of such instructions independent of the data
> +       they act upon.  Choose the default here for when no "dit" command line
> +       option is present.

I would word the last sentence as `Note the build time value can be
changed at runtime using the "dit" command line option.`

Thanks, Roger.