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

[Norbert Manthey <nmanthey@xxxxxxxxx>]

On 1/31/19 18:05, Jan Beulich wrote:
>>>> 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.
Will fix.
>
>> +#if defined(CONFIG_X86) && defined(CONFIG_HVM)
>> +static inline bool arch_barrier_nospec_true(void) {
> The brace belongs on its own line.
Will fix.
>
>> +    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.
I will move the #if inside.
>
>> +#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?
You are right, the two defines should be equal.
>
>> +#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.

Will add them.

Best,
Norbert

>
> Jan
>
>




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

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