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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 29/01/2019 14:43, Norbert Manthey wrote:
> Since the L1TF vulnerability of Intel CPUs, loading hypervisor data into
> L1 cache is problemetic, because when hyperthreading is used as well, a
s/problemetic/problematic/

> 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
s/evalauations/evaluations/

> 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.
This sentence is a bit misleading because lfence instruction does not exist on 
Arm. A better wording would be:
"As the L1TF vulnerability is only present on the x86 architecture, there are no 
need to add protection for other architectures."
> 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.
s/alterantive/alternative/

> 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>
> ---
>   xen/include/xen/nospec.h | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
>
> diff --git a/xen/include/xen/nospec.h b/xen/include/xen/nospec.h
> --- a/xen/include/xen/nospec.h
> +++ b/xen/include/xen/nospec.h
> @@ -7,6 +7,7 @@
>   #ifndef XEN_NOSPEC_H
>   #define XEN_NOSPEC_H
>   
> +#include <asm/alternative.h>
I don't want asm/alternative.h to be included here when it is not necessary on 
Arm. This is one of the reason why I suggested to have arch specific code in 
arch specific header rather than in common headers.
>   #include <asm/system.h>
>   
>   /**
> @@ -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
> + */
> +#if defined(CONFIG_X86) && defined(CONFIG_HVM)
I am not an x86 expert, however I think you should explain in the commit message 
why this is only built for HVM.
> +static inline bool arch_barrier_nospec_true(void) {
> +    alternative("", "lfence", X86_FEATURE_SC_L1TF_VULN);
> +    return true;
> +}
> +#else
> +static inline bool arch_barrier_nospec_true(void) { return true; }
> +#endif
> +
> +/*
> + * Allow to protect evaluation of conditional with respect to speculation on 
> x86
> + */
> +#ifndef CONFIG_X86
> +#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()
> +
>   #endif /* XEN_NOSPEC_H */
>   
>   /*
Cheers,

--
Julien Grall

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