Messages in this thread | | | From | Adam Sampson <> | Subject | Re: [kernel-hardening] [PATCH v4 02/10] asm/nospec, array_ptr: sanitize speculative array de-references | Date | Fri, 19 Jan 2018 17:48:04 +0000 |
| |
Jann Horn <jannh@google.com> writes:
>> +/* >> + * If idx is negative or if idx > size then bit 63 is set in the mask, >> + * and the value of ~(-1L) is zero. When the mask is zero, bounds check >> + * failed, array_ptr will return NULL. >> + */ >> +#ifndef array_ptr_mask >> +static inline unsigned long array_ptr_mask(unsigned long idx, >> unsigned long sz) >> +{ >> + return ~(long)(idx | (sz - 1 - idx)) >> (BITS_PER_LONG - 1); >> +} >> +#endif > > Nit: Maybe add a comment saying that this is equivalent to > "return ((long)idx >= 0 && idx < sz) ? ULONG_MAX : 0"?
That's only true when sz < LONG_MAX, which is documented below but not here; it's also different from the asm version, which doesn't do the idx <= LONG_MAX check. So making the constraint explicit would be a good idea.
From a bit of experimentation, when the top bit of sz is set, this expression, the C version and the assembler version all have different behaviour. For example, with 32-bit unsigned long:
index=00000000 size=80000001: expr=ffffffff c=00000000 asm=ffffffff index=80000000 size=80000001: expr=00000000 c=00000000 asm=ffffffff index=00000000 size=a0000000: expr=ffffffff c=00000000 asm=ffffffff index=00000001 size=a0000000: expr=ffffffff c=00000000 asm=ffffffff index=fffffffe size=ffffffff: expr=00000000 c=00000000 asm=ffffffff
It may be worth noting that:
return 0 - ((long) (idx < sz));
causes GCC, on ia32 and amd64, to generate exactly the same cmp/sbb sequence as in Linus's asm. Are there architectures where this form would allow speculation?
Thanks,
-- Adam Sampson <ats@offog.org> <http://offog.org/>
| |