Re: [PATCH SpectreV1+L1TF v6 4/9] spec: add l1tf-barrier

["Jan Beulich" <JBeulich@suse.com>]

>>> On 08.02.19 at 14:44, <nmanthey@amazon.de> wrote:
> To control the runtime behavior on L1TF vulnerable platforms better, the
> command line option l1tf-barrier is introduced. This option controls
> whether on vulnerable x86 platforms the lfence instruction is used to
> prevent speculative execution from bypassing the evaluation of
> conditionals that are protected with the evaluate_nospec macro.
> 
> By now, Xen is capable of identifying L1TF vulnerable hardware. However,
> this information cannot be used for alternative patching, as a CPU feature
> is required. To control alternative patching with the command line option,
> a new x86 feature "X86_FEATURE_SC_L1TF_VULN" is introduced. This feature
> is used to patch the lfence instruction into the arch_barrier_nospec_true
> function. The feature is enabled only if L1TF vulnerable hardware is
> detected and the command line option does not prevent using this feature.
> 
> The status of hyperthreading is not considered when automatically enabling
> adding the lfence instruction, because platforms without hyperthreading
> can still be vulnerable to L1TF in case the L1 cache is not flushed
> properly.
> 
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
> 
> ---
> 
> Notes:
>   v6: Move disabling l1tf-barrier into spec-ctrl=no
>       Use gap in existing flags
>       Force barrier based on commandline, independently of L1TF detection
> 
>  docs/misc/xen-command-line.pandoc | 14 ++++++++++----
>  xen/arch/x86/spec_ctrl.c          | 17 +++++++++++++++--
>  xen/include/asm-x86/cpufeatures.h |  1 +
>  xen/include/asm-x86/spec_ctrl.h   |  1 +
>  4 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -483,9 +483,9 @@ accounting for hardware capabilities as enumerated via 
> CPUID.
>  
>  Currently accepted:
>  
> -The Speculation Control hardware features `ibrsb`, `stibp`, `ibpb`,
> -`l1d-flush` and `ssbd` are used by default if available and applicable.  They 
> can
> -be ignored, e.g. `no-ibrsb`, at which point Xen won't use them itself, and
> +The Speculation Control hardware features `ibrsb`, `stibp`, `ibpb`, 
> `l1d-flush`,
> +`l1tf-barrier` and `ssbd` are used by default if available and applicable.  
> They
> +can be ignored, e.g. `no-ibrsb`, at which point Xen won't use them itself, 
> and
>  won't offer them to guests.
>  
>  ### cpuid_mask_cpu
> @@ -1896,7 +1896,7 @@ By default SSBD will be mitigated at runtime (i.e 
> `ssbd=runtime`).
>  ### spec-ctrl (x86)
>  > `= List of [ <bool>, xen=<bool>, {pv,hvm,msr-sc,rsb}=<bool>,
>  >              bti-thunk=retpoline|lfence|jmp, {ibrs,ibpb,ssbd,eager-fpu,
> ->              l1d-flush}=<bool> ]`
> +>              l1d-flush,l1tf-barrier}=<bool> ]`
>  
>  Controls for speculative execution sidechannel mitigations.  By default, 
> Xen
>  will pick the most appropriate mitigations based on compiled in support,
> @@ -1962,6 +1962,12 @@ Irrespective of Xen's setting, the feature is 
> virtualised for HVM guests to
>  use.  By default, Xen will enable this mitigation on hardware believed to 
> be
>  vulnerable to L1TF.
>  
> +On hardware vulnerable to L1TF, the `l1tf-barrier=` option can be used to 
> force
> +or prevent Xen from protecting evaluations inside the hypervisor with a 
> barrier
> +instruction to not load potentially secret information into L1 cache.  By
> +default, Xen will enable this mitigation on hardware believed to be 
> vulnerable
> +to L1TF.
> +
>  ### sync_console
>  > `= <boolean>`
>  
> diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
> --- a/xen/arch/x86/spec_ctrl.c
> +++ b/xen/arch/x86/spec_ctrl.c
> @@ -21,6 +21,7 @@
>  #include <xen/lib.h>
>  #include <xen/warning.h>
>  
> +#include <asm/cpuid.h>
>  #include <asm/microcode.h>
>  #include <asm/msr.h>
>  #include <asm/processor.h>
> @@ -50,6 +51,7 @@ bool __read_mostly opt_ibpb = true;
>  bool __read_mostly opt_ssbd = false;
>  int8_t __read_mostly opt_eager_fpu = -1;
>  int8_t __read_mostly opt_l1d_flush = -1;
> +int8_t __read_mostly opt_l1tf_barrier = -1;
>  
>  bool __initdata bsp_delay_spec_ctrl;
>  uint8_t __read_mostly default_xen_spec_ctrl;
> @@ -91,6 +93,8 @@ static int __init parse_spec_ctrl(const char *s)
>              if ( opt_pv_l1tf_domu < 0 )
>                  opt_pv_l1tf_domu = 0;
>  
> +            opt_l1tf_barrier = 0;
> +
>          disable_common:
>              opt_rsb_pv = false;
>              opt_rsb_hvm = false;
> @@ -157,6 +161,8 @@ static int __init parse_spec_ctrl(const char *s)
>              opt_eager_fpu = val;
>          else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 )
>              opt_l1d_flush = val;
> +        else if ( (val = parse_boolean("l1tf-barrier", s, ss)) >= 0 )
> +            opt_l1tf_barrier = val;
>          else
>              rc = -EINVAL;
>  
> @@ -248,7 +254,7 @@ static void __init print_details(enum ind_thunk thunk, 
> uint64_t caps)
>                 "\n");
>  
>      /* Settings for Xen's protection, irrespective of guests. */
> -    printk("  Xen settings: BTI-Thunk %s, SPEC_CTRL: %s%s, Other:%s%s\n",
> +    printk("  Xen settings: BTI-Thunk %s, SPEC_CTRL: %s%s, Other:%s%s%s\n",
>             thunk == THUNK_NONE      ? "N/A" :
>             thunk == THUNK_RETPOLINE ? "RETPOLINE" :
>             thunk == THUNK_LFENCE    ? "LFENCE" :
> @@ -258,7 +264,8 @@ static void __init print_details(enum ind_thunk thunk, 
> uint64_t caps)
>             !boot_cpu_has(X86_FEATURE_SSBD)           ? "" :
>             (default_xen_spec_ctrl & SPEC_CTRL_SSBD)  ? " SSBD+" : " SSBD-",
>             opt_ibpb                                  ? " IBPB"  : "",
> -           opt_l1d_flush                             ? " L1D_FLUSH" : "");
> +           opt_l1d_flush                             ? " L1D_FLUSH" : "",
> +           opt_l1tf_barrier                          ? " L1TF_BARRIER" : 
> "");
>  
>      /* L1TF diagnostics, printed if vulnerable or PV shadowing is in use. 
> */
>      if ( cpu_has_bug_l1tf || opt_pv_l1tf_hwdom || opt_pv_l1tf_domu )
> @@ -842,6 +849,12 @@ void __init init_speculation_mitigations(void)
>      else if ( opt_l1d_flush == -1 )
>          opt_l1d_flush = cpu_has_bug_l1tf && !(caps & ARCH_CAPS_SKIP_L1DFL);
>  
> +    /* By default, enable L1TF_VULN on L1TF-vulnerable hardware */
> +    if ( opt_l1tf_barrier == -1 )
> +        opt_l1tf_barrier = cpu_has_bug_l1tf;
> +    if ( opt_l1tf_barrier > 0)
> +        setup_force_cpu_cap(X86_FEATURE_SC_L1TF_VULN);

Did we end with a misunderstanding in the v5 discussion? I didn't
answer your question regarding whether to also consider L1D
flush availability (on top of my request to consider SMT) with a
straight "yes", but I think it was still clear that my more extensive
response boiled down to a "yes". Oh, I see now - the same topic
was discussed in two places, and for the second I then said that
with the "for now" aspect properly stated (which you now do)
I'd be fine.

So let me put it this way: Is taking into consideration at least
opt_smt and opt_l1d_flush (but putting on the side the "too
early" aspect of the determination here) very difficult to do,
or would that leave un-addressed concerns of yours? If not,
may I ask that you go at least that little step further? As said
before - we'd like to avoid penalizing configurations as well as
setups which aren't affected. In particular it would seem
pretty bad of us to further penalize people who have set
"smt=0" and who use up-to-date microcode.

Also in the second if() there's yet again a missing blank.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel