Re: [PATCH v4 22/35] crypto: BLAKE2s - generic C library implementation and selftest

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Wed, 6 Nov 2019 at 17:41, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Wed, 23 Oct 2019 at 06:51, Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Thu, Oct 17, 2019 at 09:09:19PM +0200, Ard Biesheuvel wrote:
> > > diff --git a/lib/crypto/blake2s-selftest.c b/lib/crypto/blake2s-selftest.c
> > > new file mode 100644
> > > index 000000000000..7ba00fcc6b60
> > > --- /dev/null
> > > +++ b/lib/crypto/blake2s-selftest.c
> > > @@ -0,0 +1,2093 @@
> > > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > > +/*
> > > + * Copyright (C) 2015-2019 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
> > > + */
> > > +
> > > +#include <crypto/blake2s.h>
> > > +#include <linux/string.h>
> > > +
> > > +static const u8 blake2s_testvecs[][BLAKE2S_HASH_SIZE] __initconst = {
> > [...]
> > > +bool __init blake2s_selftest(void)
> > > +{
> > > +     u8 key[BLAKE2S_KEY_SIZE];
> > > +     u8 buf[ARRAY_SIZE(blake2s_testvecs)];
> > > +     u8 hash[BLAKE2S_HASH_SIZE];
> > > +     size_t i;
> > > +     bool success = true;
> > > +
> > > +     for (i = 0; i < BLAKE2S_KEY_SIZE; ++i)
> > > +             key[i] = (u8)i;
> > > +
> > > +     for (i = 0; i < ARRAY_SIZE(blake2s_testvecs); ++i)
> > > +             buf[i] = (u8)i;
> > > +
> > > +     for (i = 0; i < ARRAY_SIZE(blake2s_keyed_testvecs); ++i) {
> > > +             blake2s(hash, buf, key, BLAKE2S_HASH_SIZE, i, BLAKE2S_KEY_SIZE);
> > > +             if (memcmp(hash, blake2s_keyed_testvecs[i], BLAKE2S_HASH_SIZE)) {
> > > +                     pr_err("blake2s keyed self-test %zu: FAIL\n", i + 1);
> > > +                     success = false;
> > > +             }
> > > +     }
> > > +
> > > +     for (i = 0; i < ARRAY_SIZE(blake2s_testvecs); ++i) {
> > > +             blake2s(hash, buf, NULL, BLAKE2S_HASH_SIZE, i, 0);
> > > +             if (memcmp(hash, blake2s_testvecs[i], BLAKE2S_HASH_SIZE)) {
> > > +                     pr_err("blake2s unkeyed self-test %zu: FAIL\n", i + i);
> > > +                     success = false;
> > > +             }
> > > +     }
> > > +     return success;
> > > +}
> >
> > The only tests here are for blake2s(), with 0 and 32-byte keys.  There's no
> > tests that incremental blake2s_update()s work correctly, nor any other key
> > sizes.  And these don't get tested properly by the blake2s-generic shash tests
> > either, because blake2s-generic has a separate implementation of the boilerplate
> > and calls blake2s_compress_generic() directly.  Did you consider implementing
> > blake2s-generic on top of blake2s_init/update/final instead?
> >
>
> That would make blake2s-generic use the accelerated implementations as
> well, which I tried to avoid.
>
> I will add some test cases here instead.
>
> > Also, blake2s_hmac() needs tests.
> >
>
> Ack

I cooked up another set of test cases for keyed and plain blake2s
which use arbitrary combinations of supported key and digest lengths.

The delta patch is at the top of this branch:
https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=wireguard-crypto-library-api-v5

The blake2s_hmac() code is actually broken: it ignores the 'outlen'
parameter except for the memcpy() at the end, which means it runs both
blake2s with the full length digest twice and then simply truncates it
at the end.

Given that blake2s hmac seems to be a WireGuard invention, which only
uses the full digest length, would anyone mind if I change this to
blake2s256_hmac() and drop the outlen parameter? Then, we at least
have parity with openssl HMAC() and EVP_blake2s256().