Received-SPF: pass (google.com: best guess record for domain of linux-crypto-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190925161255.1871-1-ard.biesheuvel@linaro.org>
 <20190925161255.1871-19-ard.biesheuvel@linaro.org> <CAHk-=wjYsbxSiV_XKWV3BwGvau_hUvQiQHLOoc7vLUZt0Wqzfw@mail.gmail.com>
 <CAKv+Gu_YOqvqJ4YC=ixBh-v4fiFTFNpEagHiTRU7Oq4PrhJPkw@mail.gmail.com>
In-Reply-To: <CAKv+Gu_YOqvqJ4YC=ixBh-v4fiFTFNpEagHiTRU7Oq4PrhJPkw@mail.gmail.com>
From:   Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date:   Thu, 26 Sep 2019 14:34:56 +0200
Message-ID: <CAKv+Gu8JxmU_8byEun17B2d9LjQpR0wRJktUfr-Oax0q4if6Dg@mail.gmail.com>
Subject: Re: [RFC PATCH 18/18] net: wireguard - switch to crypto API for
 packet encryption
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        David Miller <davem@davemloft.net>,
        Greg KH <gregkh@linuxfoundation.org>,
        "Jason A . Donenfeld" <Jason@zx2c4.com>,
        Samuel Neves <sneves@dei.uc.pt>,
        Dan Carpenter <dan.carpenter@oracle.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Eric Biggers <ebiggers@google.com>,
        Andy Lutomirski <luto@kernel.org>,
        Will Deacon <will@kernel.org>, Marc Zyngier <maz@kernel.org>,
        Catalin Marinas <catalin.marinas@arm.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

On Thu, 26 Sep 2019 at 13:06, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Thu, 26 Sep 2019 at 00:15, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Wed, Sep 25, 2019 at 9:14 AM Ard Biesheuvel
> > <ard.biesheuvel@linaro.org> wrote:
> > >
> > > Replace the chacha20poly1305() library calls with invocations of the
> > > RFC7539 AEAD, as implemented by the generic chacha20poly1305 template.
> >
> > Honestly, the other patches look fine to me from what I've seen (with
> > the small note I had in a separate email for 11/18), but this one I
> > consider just nasty, and a prime example of why people hate those
> > crypto lookup routines.
> >
> > Some of it is just the fundamental and pointless silly indirection,
> > that just makes things harder to read, less efficient, and less
> > straightforward.
> >
> > That's exemplified by this part of the patch:
> >
> > >  struct noise_symmetric_key {
> > > -       u8 key[NOISE_SYMMETRIC_KEY_LEN];
> > > +       struct crypto_aead *tfm;
> >
> > which is just one of those "we know what we want and we just want to
> > use it directly" things, and then the crypto indirection comes along
> > and makes that simple inline allocation of a small constant size
> > (afaik it is CHACHA20POLY1305_KEY_SIZE, which is 32) be another
> > allocation entirely.
> >
> > And it's some random odd non-typed thing too, so then you have that
> > silly and stupid dynamic allocation using a name lookup:
> >
> >    crypto_alloc_aead("rfc7539(chacha20,poly1305)", 0, CRYPTO_ALG_ASYNC);
> >
> > to create what used to be (and should be) a simple allocation that was
> > has a static type and was just part of the code.
> >
>
> That crypto_alloc_aead() call does a lot of things under the hood:
> - use an existing instantiation of rfc7539(chacha20,poly1305) if available,
> - look for modules that implement the whole transformation directly,
> - if none are found, instantiate the rfc7539 template, which will
> essentially do the above for chacha20 and poly1305, potentially using
> per-arch accelerated implementations if available (for either), or
> otherwise, fall back to the generic versions.
>
> What *I* see as the issue here is not that we need to do this at all,
> but that we have to do it for each value of the key. IMO, it would be
> much better to instantiate this thing only once, and have a way of
> passing a per-request key into it, permitting us to hide the whole
> thing behind the existing library interface.
>

Note that we don't have to do the whole dance for each new value of
the key: subsequent invocations will all succeed at step #1, and grab
the existing instantiation, but allocate a new TFM structure that
refers to it. It is this step that we should be able to omit as well
if the API is changed to allow per-request keys to be passed in via
the request structure.