linux-kernel - RE: [PATCH crypto v2] crypto: blake2s

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: <ffa404b7427043fda4b9f4a20ea0f068@AcuMS.aculab.com>
Date:   Fri, 27 May 2022 12:36:07 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     "'Jason A. Donenfeld'" <Jason@...c4.com>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>
CC:     gaochao <gaochao49@...wei.com>, Eric Biggers <ebiggers@...nel.org>,
        "Ard Biesheuvel" <ardb@...nel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [PATCH crypto v2] crypto: blake2s - remove shash module

From: Jason A. Donenfeld
> Sent: 27 May 2022 09:11
> 
> BLAKE2s has no use as an shash, with no users of it. Just remove all of
> this unnecessary plumbing. Removing this shash was something we talked
> about back when we were making BLAKE2s a built-in, but I simply never
> got around to doing it. So this completes that project.
...
> diff --git a/lib/crypto/blake2s.c b/lib/crypto/blake2s.c
> index c71c09621c09..716da32cf4dc 100644
> --- a/lib/crypto/blake2s.c
> +++ b/lib/crypto/blake2s.c
> @@ -16,16 +16,43 @@
>  #include <linux/init.h>
>  #include <linux/bug.h>
> 
> +static inline void blake2s_set_lastblock(struct blake2s_state *state)
> +{
> +	state->f[0] = -1;
> +}
> +
>  void blake2s_update(struct blake2s_state *state, const u8 *in, size_t inlen)
>  {
> -	__blake2s_update(state, in, inlen, false);
> +	const size_t fill = BLAKE2S_BLOCK_SIZE - state->buflen;
> +
> +	if (unlikely(!inlen))
> +		return;

Does this happen often enough to optimise for?
The zero length memcpy() should be fine.
(though pedants might worry about in == NULL)

> +	if (inlen > fill) {

Testing inlen >= fill will be better.
You also don't need the code below in the (probably) likely
case that state->buflen == 0.

> +		memcpy(state->buf + state->buflen, in, fill);
> +		blake2s_compress(state, state->buf, 1, BLAKE2S_BLOCK_SIZE);
> +		state->buflen = 0;
> +		in += fill;
> +		inlen -= fill;

an 'if (!inlen) return' check here may be a cheap optimisation.

> +	}
> +	if (inlen > BLAKE2S_BLOCK_SIZE) {

This test only needs to be inside the earlier inlen > fill condition.
The compiler may not be able to assume so.

> +		const size_t nblocks = DIV_ROUND_UP(inlen, BLAKE2S_BLOCK_SIZE);

Why not inlen/BLAKE2S_BLOCK_SIZE and remove all the '- 1'.
Looping inside blakes2s_compress() has to be better than
doing an extra call when processing the next data block.

> +		blake2s_compress(state, in, nblocks - 1, BLAKE2S_BLOCK_SIZE);
> +		in += BLAKE2S_BLOCK_SIZE * (nblocks - 1);
> +		inlen -= BLAKE2S_BLOCK_SIZE * (nblocks - 1);
> +	}
> +	memcpy(state->buf + state->buflen, in, inlen);
> +	state->buflen += inlen;
>  }
>  EXPORT_SYMBOL(blake2s_update);
> 
>  void blake2s_final(struct blake2s_state *state, u8 *out)
>  {
>  	WARN_ON(IS_ENABLED(DEBUG) && !out);
> -	__blake2s_final(state, out, false);
> +	blake2s_set_lastblock(state);
> +	memset(state->buf + state->buflen, 0, BLAKE2S_BLOCK_SIZE - state->buflen); /* Padding */
> +	blake2s_compress(state, state->buf, 1, state->buflen);
> +	cpu_to_le32_array(state->h, ARRAY_SIZE(state->h));
> +	memcpy(out, state->h, state->outlen);
>  	memzero_explicit(state, sizeof(*state));
>  }
>  EXPORT_SYMBOL(blake2s_final);
> @@ -38,12 +65,7 @@ static int __init blake2s_mod_init(void)
>  	return 0;
>  }

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)