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

[Arnd Bergmann]

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.

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).

       Arnd