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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPcyv4iEZMkU-K14vTcFSjAy=12QHPJjMGc8Uu7xUVakHfqOyQ@mail.gmail.com>
Date:   Fri, 19 Jan 2018 10:12:47 -0800
From:   Dan Williams <dan.j.williams@...el.com>
To:     Adam Sampson <ats@...og.org>
Cc:     Jann Horn <jannh@...gle.com>,
        kernel list <linux-kernel@...r.kernel.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        Kernel Hardening <kernel-hardening@...ts.openwall.com>,
        Catalin Marinas <catalin.marinas@....com>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Will Deacon <will.deacon@....com>,
        Russell King <linux@...linux.org.uk>,
        Ingo Molnar <mingo@...hat.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Alan Cox <alan@...ux.intel.com>,
        Alexei Starovoitov <ast@...nel.org>
Subject: Re: [kernel-hardening] [PATCH v4 02/10] asm/nospec, array_ptr:
 sanitize speculative array de-references

[ adding Alexei back to the cc ]

On Fri, Jan 19, 2018 at 9:48 AM, Adam Sampson <ats@...og.org> wrote:
> Jann Horn <jannh@...gle.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?

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.