[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/4] x86emul: avoid speculative out of bounds accesses
>>> 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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |