Re: [Xen-devel] [PATCH SpectreV1+L1TF v5 5/9] nospec: introduce evaluate_nospec

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.01.19 at 15:43, <nmanthey@xxxxxxxxx> wrote:
> Since the L1TF vulnerability of Intel CPUs, loading hypervisor data into
> L1 cache is problemetic, because when hyperthreading is used as well, a
> guest running on the sibling core can leak this potentially secret data.
> 
> To prevent these speculative accesses, we block speculation after
> accessing the domain property field by adding lfence instructions. This
> way, the CPU continues executing and loading data only once the condition
> is actually evaluated.
> 
> As the macros are typically used in if statements, the lfence has to come
> in a compatible way. Therefore, a function that returns true after an
> lfence instruction is introduced. To protect both branches after a
> conditional, an lfence instruction has to be added for the two branches.
> To be able to block speculation after several evalauations, the generic
> barrier macro block_speculation is also introduced.
> 
> As the L1TF vulnerability is only present on the x86 architecture, the
> macros will not use the lfence instruction on other architectures and the
> protection is disabled during compilation. By default, the lfence
> instruction is not present either. Only when a L1TF vulnerable platform
> is detected, the lfence instruction is patched in via alterantive patching.
> 
> Introducing the lfence instructions catches a lot of potential leaks with
> a simple unintrusive code change. During performance testing, we did not
> notice performance effects.
> 
> Signed-off-by: Norbert Manthey <nmanthey@xxxxxxxxx>

Looks okay to me now, but I'm going to wait with giving an ack
until perhaps others have given comments, as some of this
was not entirely uncontroversial. There are a few cosmetic
issues left though:

> @@ -64,6 +65,33 @@ static inline unsigned long 
> array_index_mask_nospec(unsigned long index,
>  #define array_access_nospec(array, index)                               \
>      (array)[array_index_nospec(index, ARRAY_SIZE(array))]
>  
> +/*
> + * Allow to insert a read memory barrier into conditionals
> + */

Here and below, please make single line comments really be
single lines.

> +#if defined(CONFIG_X86) && defined(CONFIG_HVM)
> +static inline bool arch_barrier_nospec_true(void) {

The brace belongs on its own line.

> +    alternative("", "lfence", X86_FEATURE_SC_L1TF_VULN);
> +    return true;
> +}
> +#else
> +static inline bool arch_barrier_nospec_true(void) { return true; }

This could be avoided if you placed the #if inside the
function body.

> +#endif
> +
> +/*
> + * Allow to protect evaluation of conditional with respect to speculation on 
> x86
> + */
> +#ifndef CONFIG_X86

Why is this conditional different from the one above?

> +#define evaluate_nospec(condition) (condition)
> +#else
> +#define evaluate_nospec(condition)                                         \
> +    ((condition) ? arch_barrier_nospec_true() : !arch_barrier_nospec_true())
> +#endif
> +
> +/*
> + * Allow to block speculative execution in generic code
> + */
> +#define block_speculation() (void)arch_barrier_nospec_true()

Missing an outer pair of parentheses.

Jan



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