linux-kernel - Re: [PATCH net-next v5 02/20] zinc: introduce minimal cryptography library

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: <CAK8P3a3j0eqAPc-Vkp7YFxT56R2-LrqL_EcULQFam5Bm93gPyw@mail.gmail.com>
Date:   Tue, 25 Sep 2018 09:18:05 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Eric Biggers <ebiggers@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" 
        <linux-crypto@...r.kernel.org>, David Miller <davem@...emloft.net>,
        gregkh <gregkh@...uxfoundation.org>,
        Samuel Neves <sneves@....uc.pt>,
        Andy Lutomirski <luto@...nel.org>,
        Jean-Philippe Aumasson <jeanphilippe.aumasson@...il.com>
Subject: Re: [PATCH net-next v5 02/20] zinc: introduce minimal cryptography library

On Sat, Sep 22, 2018 at 6:11 PM Arnd Bergmann <arnd@...db.de> wrote:
>
> On Thu, Sep 20, 2018 at 5:12 PM Jason A. Donenfeld <Jason@...c4.com> wrote:
> >
> > Hey Arnd,
> >
> > On Thu, Sep 20, 2018 at 6:02 PM Arnd Bergmann <arnd@...db.de> wrote:
> > > Right, if you hit a stack requirement like this, it's usually the compiler
> > > doing something bad, not just using too much stack but also generating
> > > rather slow object code in the process. It's better to fix the bug by
> > > optimizing the code to not spill registers to the stack.
> > >
> > > In the long run, I'd like to reduce the stack frame size further, so
> > > best assume that anything over 1024 bytes (on 32-bit) or 1280 bytes
> > > (on 64-bit) is a bug in the code, and stay below that.
> > >
> > > For prototyping, you can just mark the broken functions individually
> > > by setting the warning limit for a specific function that is known to
> > > be misoptimized by the compiler (with a comment about which compiler
> > > and architectures are affected), but not override the limit for the
> > > entire file.
> >
> > Thanks for the explanation. Fortunately in my case, the issues were
> > trivially fixable to get it under 1024/1280
>
> A lot of these bugs are not trivial, but we still need a full analysis of what
> failed and what the possible mititgations are. Can you describe more
> specifically what causes it?

I think I misread your earlier sentence and thought you had said the
exact opposite.

For confirmation, I've downloaded your git tree and built it with my
collection of compilers (gcc-4.6 through 8.1) and tried building it
in various configurations. Nothing alarming stood out, the only
thing that I think would might warrant some investigation is this one:

lib/zinc/curve25519/curve25519-hacl64.h: In function 'curve25519_generic':
lib/zinc/curve25519/curve25519-hacl64.h:785:1: warning: the frame size
of 1536 bytes is larger than 500 bytes [-Wframe-larger-than=]

Without KASAN, this takes 832 bytes, which is still more than it should
use from a look at the source code.

I first suspected some misoptimization around the get/put_unaligned_le64()
calls, but playing around with it some more led me to this patch:

diff --git a/lib/zinc/curve25519/curve25519-hacl64.h
b/lib/zinc/curve25519/curve25519-hacl64.h
index c7b2924a68c2..1f6eb5708a0e 100644
--- a/lib/zinc/curve25519/curve25519-hacl64.h
+++ b/lib/zinc/curve25519/curve25519-hacl64.h
@@ -182,8 +182,7 @@ static __always_inline void
fmul_mul_shift_reduce_(u128 *output, u64 *input,

 static __always_inline void fmul_fmul(u64 *output, u64 *input, u64 *input21)
 {
-       u64 tmp[5];
-       memcpy(tmp, input, 5 * sizeof(*input));
+       u64 tmp[5] = { input[0], input[1], input[2], input[3], input[4] };
        {
                u128 b4;
                u128 b0;

That change gets it down to

lib/zinc/curve25519/curve25519-hacl64.h: In function 'curve25519_generic':
lib/zinc/curve25519/curve25519-hacl64.h:788:1: warning: the frame size
of 640 bytes is larger than 500 bytes [-Wframe-larger-than=]

with KASAN, or 496 bytes without it. This indicates that there might
be something wrong with either gcc-8 or with our fortified memset()
function that requires more investigation. Maybe you can see
something there that I missed.

        Arnd