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

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]

Date:   Wed, 3 Jan 2018 17:37:47 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Ard Biesheuvel <ard.biesheuvel@...aro.org>
Cc:     PrasannaKumar Muralidharan <prasannatsmkumar@...il.com>,
        Jakub Jelinek <jakub@...hat.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        James Morris <james.l.morris@...cle.com>,
        Richard Biener <rguenther@...e.de>,
        Jakub Jelinek <jakub@....gnu.org>,
        "David S. Miller" <davem@...emloft.net>,
        "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" 
        <linux-crypto@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] [RFT] crypto: aes-generic - turn off -ftree-pre and -ftree-sra

On Fri, Dec 22, 2017 at 4:47 PM, Ard Biesheuvel
<ard.biesheuvel@...aro.org> wrote:
> On 21 December 2017 at 13:47, PrasannaKumar Muralidharan <prasannatsmkumar@...il.com> wrote:
>> On 21 December 2017 at 17:52, Ard Biesheuvel <ard.biesheuvel@...aro.org> wrote:
>>> On 21 December 2017 at 10:20, Arnd Bergmann <arnd@...db.de> 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