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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew

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