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

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Eric Biggers <ebiggers@kernel.org>
Cc: "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" 
	<linux-crypto@vger.kernel.org>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	David Miller <davem@davemloft.net>,
	"Jason A . Donenfeld" <Jason@zx2c4.com>,
	Samuel Neves <sneves@dei.uc.pt>, Arnd Bergmann <arnd@arndb.de>,
	Andy Lutomirski <luto@kernel.org>,
	Martin Willi <martin@strongswan.org>,
	Rene van Dorst <opensource@vdorst.com>,
	David Sterba <dsterba@suse.com>
Subject: Re: [PATCH v4 22/35] crypto: BLAKE2s - generic C library implementation and selftest
Date: Wed, 6 Nov 2019 17:41:37 +0100	[thread overview]
Message-ID: <CAKv+Gu_9rY6OBEZ9iZQ53CWRQbd+k64JL4Tuazn3BO3ohF2g6w@mail.gmail.com> (raw)
In-Reply-To: <20191023045157.GB361298@sol.localdomain>

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