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