Re: [kernel-hardening] [PATCH v4 02/10] asm/nospec, array_ptr: sanitize speculative array de-references

[Will Deacon]

On Fri, Jan 19, 2018 at 10:12:47AM -0800, Dan Williams wrote:
> [ adding Alexei back to the cc ]
> 
> On Fri, Jan 19, 2018 at 9:48 AM, Adam Sampson <ats@xxxxxxxxx> wrote:
> > Jann Horn <jannh@xxxxxxxxxx> 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?
> 
> We're operating on the assumption that compilers will not try to
> introduce branches where they don't exist in the code, so if this is
> producing identical assembly I think we should go with it and drop the
> x86 array_ptr_mask.

Branches, perhaps, but this could easily be compiled to a conditional
select (CSEL) instruction on arm64 and that wouldn't be safe without a
CSDB. Of course, we can do our own thing in assembly to prevent that, but
it would mean that the generic C implementation would not be robust for us.

Will