Re: [PATCH] [RFT] crypto: aes-generic - turn off -ftree-pre and -ftree-sra

[Ard Biesheuvel]

On 3 January 2018 at 16:37, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Fri, Dec 22, 2017 at 4:47 PM, Ard Biesheuvel
> <ard.biesheuvel@xxxxxxxxxx> wrote:
>> On 21 December 2017 at 13:47, PrasannaKumar Muralidharan <prasannatsmkumar@xxxxxxxxx> wrote:
>>> On 21 December 2017 at 17:52, Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote:
>>>> On 21 December 2017 at 10:20, Arnd Bergmann <arnd@xxxxxxxx> wrote:
>>>>
>>>> So my vote is to disable UBSAN for all such cipher implementations:
>>>> aes_generic, but also aes_ti, which has a similar 256 byte lookup
>>>> table [although it does not seem to be affected by the same issue as
>>>> aes_generic], and possibly others as well.
>>>>
>>>> Perhaps it makes sense to move core cipher code into a separate
>>>> sub-directory, and disable UBSAN at the directory level?
>>>>
>>>> It would involve the following files
>>>>
>>>> crypto/aes_generic.c
>>>> crypto/aes_ti.c
>>>> crypto/anubis.c
>>>> crypto/arc4.c
>>>> crypto/blowfish_generic.c
>>>> crypto/camellia_generic.c
>>>> crypto/cast5_generic.c
>>>> crypto/cast6_generic.c
>>>> crypto/des_generic.c
>>>> crypto/fcrypt.c
>>>> crypto/khazad.c
>>>> crypto/seed.c
>>>> crypto/serpent_generic.c
>>>> crypto/tea.c
>>>> crypto/twofish_generic.c
>>>
>>> As *SAN is enabled only on developer setup, is such a change required?
>>> Looks like I am missing something here. Can you explain what value it
>>> provides?
>>>
>>
>> Well, in this particular case, the value it provides is that the
>> kernel can still boot and invoke the AES code without overflowing the
>> kernel stack. Of course, this is a compiler issue that hopefully gets
>> fixed, but I think it may be reasonable to exclude some C code from
>> UBSAN by default.
>
> Any idea how to proceed here? I've retested with the latest gcc snapshot
> and verified that the problem is still there. No idea what the chance of
> getting it fixed before the 7.3 release is. From the performance tests
> I've done, the patch I posted is pretty much useless, it causes significant
> performance regressions on most other compiler versions.
>
> A minimal patch would be to disable UBSAN specifically for aes-generic.c
> for gcc-7.2+ but not gcc-8 to avoid the potential stack overflow. We could
> also force building with -Os on gcc-7, and leave UBSAN enabled,
> this would improve performance some 3-5% on x86 with gcc-7 (both
> 7.1 and 7.2.1) and avoid the stack overflow.
>

Can't we just disable UBSAN for that file for all GCC versions and be
done with it? It is not a production feature, and that code is
unlikely to change in ways where UBSAN would make a difference anyway,
nor is it ever executed on 99.9% of systems running Linux.

> For the performance regression in gcc-7.2.1 on this file, I've opened
> a separate gcc PR now, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83651
> I've also tested the libressl version of their generic AES code, with
> mixed results (it's appears to be much slower than the kernel version
> to start with, and while it has further performance regressions with recent
> compilers, those are with a different set of versions compared to the
> kernel implementation, and it does not suffer from the high stack usage).
>