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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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?  Irrespective, this macro can
probably be expressed as a ternary operation and retain its lvalue-ness,
albeit at the expense of readability.

>
> As just said on the call, in the re-based AVX512F gather patch
> the respective hunk is now
>
> --- unstable.orig/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ unstable/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -662,9 +662,10 @@ static inline unsigned long *decode_gpr(
>      BUILD_BUG_ON(ARRAY_SIZE(cpu_user_regs_gpr_offsets) &
>                   (ARRAY_SIZE(cpu_user_regs_gpr_offsets) - 1));
>  
> -    ASSERT(modrm < ARRAY_SIZE(cpu_user_regs_gpr_offsets));
> +    /* Note that this also acts as array_access_nospec() stand-in. */
> +    modrm &= ARRAY_SIZE(cpu_user_regs_gpr_offsets) - 1;
>  
> -    return (void *)regs + array_access_nospec(cpu_user_regs_gpr_offsets, 
> modrm);
> +    return (void *)regs + cpu_user_regs_gpr_offsets[modrm];
>  }
>  
>  /* Unhandleable read, write or instruction fetch */
>
> I could obviously make the patch here simply insert that comment
> instead of adding actual uses of the macro.

Ah ok - this looks fine, however it ends up looking.

~Andrew

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