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;
Date:   Fri, 11 Oct 2019 10:57:40 -0700
From:   Eric Biggers <ebiggers@kernel.org>
To:     David Sterba <dsterba@suse.com>
Cc:     linux-crypto@vger.kernel.org, ard.biesheuvel@linaro.org
Subject: Re: [PATCH v4 0/5] BLAKE2b generic implementation
Message-ID: <20191011175739.GA235973@gmail.com>
Mail-Followup-To: David Sterba <dsterba@suse.com>,
        linux-crypto@vger.kernel.org, ard.biesheuvel@linaro.org
References: <cover.1570812094.git.dsterba@suse.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <cover.1570812094.git.dsterba@suse.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

On Fri, Oct 11, 2019 at 06:52:03PM +0200, David Sterba wrote:
> The patchset adds blake2b refrerence implementation and test vectors.
> 
> V4:
> 
> Code changes:
> 
> - removed .finup
> - removed .cra_init
> - dropped redundant sanity checks (key length, output size length)
> - switch blake2b_param from a 1 element array to plain struct
> - direct assignment in blake2b_init, instead of put_unaligned*
> - removed blake2b_is_lastblock
> - removed useless error cases in the blake * helpers
> - replace digest_desc_ctx with blake2b_state
> - use __le32 in blake2b_param
> 
> Added testmgr vectors:
> 
> - all digests covered: 160, 256, 384, 512
> - 4 different keys:
>   - empty
>   - short (1 byte, 'B', 0x42)
>   - half of the default key (32 bytes, sequence 00..1f)
>   - default key (64 bytes, sequence 00..3f)
> - plaintext values:
>   - subsequences of 0..15 and 247..255
>   - the full range 0..255 add up to 4MiB of .h, for all digests and key
>     sizes, so this is not very practical for the in-kernel testsuite
> - official blake2 provided test vectors are only for empty and default key for
>   digest size 512
> - the remaining combinations were obtained from b2sum utility (enhanced to
>   accept a key)

The choice of data lengths seems a bit unusual, as they include every length in
two ranges but nothing in between.  Also, none of the lengths except 0 is a
multiple of the blake2b block size.  Instead, maybe use something like
[0, 1, 7, 15, 64, 247, 256]?

Also, since the 4 variants share nearly all their code, it seems the tests would
be just as effective in practice if we cut the test vectors down by 4x by
distributing the key lengths among each variant.  Like:

          blake2b-160  blake2b-256  blake2b-384  blake2b-512
         ---------------------------------------------------
len=0   | klen=0       klen=1       klen=16      klen=32
len=1   | klen=16      klen=32      klen=0       klen=1
len=7   | klen=32      klen=0       klen=1       klen=16
len=15  | klen=1       klen=16      klen=32      klen=0
len=64  | klen=0       klen=1       klen=16      klen=32
len=247 | klen=16      klen=32      klen=0       klen=1
len=256 | klen=32      klen=0       klen=1       klen=16

> 
> Testing performed:
> 
> - compiled with SLUB_DEBUG and KASAN, plus crypto selftests
>   CONFIG_CRYPTO_MANAGER2=y
>   CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=n
>   CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y
> - module loaded, no errors reported from the tessuite
> - (un)intentionally broken test values were detected
> 
> The test values were produced by b2sum, compiled from the reference
> implementation. The generated values were cross-checked by pyblake2
> based script (ie. not the same sources, built by distro).
> 
> The .h portion of testmgr is completely generated, so in case somebody feels
> like reducing it in size, adding more keys, changing the formatting, it's easy
> to do.

> 
> In case the patches don't make it to the mailinglist, it's in git
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git dev/blake2b-v4

Can you please rebase this onto cryptodev/master?

- Eric