Re: [Xen-devel] [PATCH for-4.13 1/2] xen/nospec: Introduce CONFIG_SPECULATIVE_ARRAY_HARDEN

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/10/2019 11:45, Jan Beulich wrote:
> On 30.09.2019 20:24, Andrew Cooper wrote:
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -77,6 +77,27 @@ config HAS_CHECKPOLICY
>>      string
>>      option env="XEN_HAS_CHECKPOLICY"
>>  
>> +menu "Speculative hardening"
>> +
>> +config SPECULATIVE_ARRAY_HARDEN
> Seeing also the new item in patch 2 - wouldn't it be better for them all
> to have (just) a common prefix, rather than common prefix and a common
> suffix. E.g. SPECULATIVE_HARDEN_ARRAYS here and SPECULATIVE_HARDEN_BRANCHES
> there?

Can do.

>
>> --- a/xen/include/xen/nospec.h
>> +++ b/xen/include/xen/nospec.h
>> @@ -33,6 +33,7 @@ static inline unsigned long 
>> array_index_mask_nospec(unsigned long index,
>>  }
>>  #endif
>>  
>> +#ifdef CONFIG_SPECULATIVE_ARRAY_HARDEN
>>  /*
>>   * array_index_nospec - sanitize an array index after a bounds check
>>   *
>> @@ -58,6 +59,17 @@ static inline unsigned long 
>> array_index_mask_nospec(unsigned long index,
>>                                                                          \
>>      (typeof(_i)) (_i & _mask);                                          \
>>  })
>> +#else
>> +/* No index hardening. */
>> +#define array_index_nospec(index, size)                                 \
>> +({                                                                      \
>> +    typeof(index) _i = (index);                                         \
>> +    typeof(size) _s = (size);                                           \
>> +                                                                        \
>> +    (void)_s;                                                           \
>> +    _i;                                                                 \
>> +})
> Why not the simpler
>
> #define array_index_nospec(index, size)                                 \
> ({                                                                      \
>     (void)(size);                                                       \
>     (index);                                                            \
> })
>
> at which point it would seem feasible to avoid the use of compiler
> extensions altogether by making it
>
> #define array_index_nospec(index, size) ((void)(size), (index))

Huh - I tried that first, and GCC was distinctly unhappy.  It turns out
to be the bracketing of size, which when omitted, causes:

/local/xen.git/xen/include/xen/nospec.h:66:42: error: void value not
ignored as it ought to be
 #define array_index_nospec(index, size) ((void)size, (index))
                                          
argo.c:2174:16: note: in expansion of macro ‘array_index_nospec’
         niov = array_index_nospec(arg3, XEN_ARGO_MAXIOV + 1);
                ^~~~~~~~~~~~~~~~~~

I'll switch to this version.

~Andrew

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