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]

Message-ID: <CAKv+Gu_+nQA15NuOEA6QLbEFRJkztJ7YcPvFuX5p5j8dkiJrAg@mail.gmail.com>
Date:   Wed, 3 Jan 2018 17:36:09 +0000
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     Arnd Bergmann <arnd@...db.de>
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 3 January 2018 at 16:37, Arnd Bergmann <arnd@...db.de> wrote:
> 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.
>

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