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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

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.

Jan



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