Re: [Xen-devel] [PATCH 1/4] x86emul: avoid speculative out of bounds accesses

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.01.19 at 17:14, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 31/01/2019 15:50, Jan Beulich wrote:
>>>>> On 31.01.19 at 15:54, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 31/01/2019 14:25, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>>> @@ -2207,10 +2207,7 @@ static void *_decode_gpr(
>>>>  
>>>>      ASSERT(modrm_reg < ARRAY_SIZE(byte_reg_offsets));
>>>>  
>>>> -    /* For safety in release builds.  Debug builds will hit the ASSERT() 
>>>> */
>>>> -    modrm_reg &= ARRAY_SIZE(byte_reg_offsets) - 1;
>>>> -
>>>> -    return (void *)regs + byte_reg_offsets[modrm_reg];
>>>> +    return (void *)regs + array_access_nospec(byte_reg_offsets, 
>>>> modrm_reg);
>>>>  }
>>> Actually, the &= here wasn't by accident.  When the array size is an
>>> power of two and known to the compiler, it is a rather lower overhead
>>> alternative to array_access_nospec(), as it avoids the cmp/sbb dance in
>>> the asm volatile statement.
>>>
>>> I wonder if there is a sensible way cope with this in
>>> array_access_nospec().  Perhaps something like:
>>>
>>> #define array_access_nospec(array, index)
>>> ({
>>>     size_t _s = ARRAY_SIZE(array);
>>>
>>>     if ( !(_s & (_s - 1)) )
>>>     {
>>>         typeof(index) _i = index & (_s - 1);
>>>         OPTIMIZER_HIDE_VAR(_i);
>>>         (array)[_i];
>>>     }
>>>     else
>>>         (array)[array_index_nospec(index, ARRAY_SIZE(array))];
>>> })
>>>
>>> As _s is known at compile time, only one half of the if condition will
>>> be emitted by the compiler.
>> Except that this won't work as an lvalue anymore, yet we want
>> to use it as such in some cases. I can't seem to immediately think
>> of a way to overcome this.
> 
> Does the lvalue use result in safe asm?

Hmm, I'm a little puzzled - why would it not? Could you perhaps be
a little more specific about what worries you in that case?

>  Irrespective, this macro can
> probably be expressed as a ternary operation and retain its lvalue-ness,
> albeit at the expense of readability.

Oh, you're right. Not even overly difficult. The readability aspect
could perhaps be addressed by using another helper macro. But
for the purposes here I'll go the comment adjustment route. We
can always add this improvement to the macro later on.

Jan



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