Message-ID: <20251020171838.GB1644@sol>
Date: Mon, 20 Oct 2025 10:18:38 -0700
From: Eric Biggers <ebiggers@...nel.org>
To: David Howells <dhowells@...hat.com>
Cc: linux-crypto@...r.kernel.org, Ard Biesheuvel <ardb@...nel.org>,
	"Jason A . Donenfeld" <Jason@...c4.com>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-s390@...r.kernel.org
Subject: Re: [PATCH 11/17] lib/crypto: sha3: Simplify the API

On Mon, Oct 20, 2025 at 11:33:36AM +0100, David Howells wrote:
> Eric Biggers <ebiggers@...nel.org> wrote:
> 
> > Instead of having separate types and functions for each of the six SHA-3
> > algorithms, instead divide them into two groups: the digests and the
> > XOFs.  The digests use sha3_ctx and the XOFs use shake_ctx.  The
> > internal context is now called __sha3_ctx.
> 
> Please roll changes into the original patches rather than posting them with a
> set of "fixes" and add a Co-developed-by tag for yourself.

Sure, that sounds good to me.  I'll do that on the next version.  For
this version I wanted to make it clear what I had changed.  Also, some
people don't like it when their patches are changed, so I wanted to make
sure you were okay with it first.

> > +/** Context for SHA3-224, SHA3-256, SHA3-384, or SHA3-512 */
> > +struct sha3_ctx {
> > +	struct __sha3_ctx	ctx;
> > +	u8			digest_size;	/* Digest size in bytes */
> > +};
> 
> Don't do that.  That expands the context by an extra word when there's spare
> space in __sha3_ctx.  If you go with the separate types, then this field is
> redundant.

Right, packing the struct tightly is nice.  On the other hand, having
the digest_size be in the digest-specific struct prevents it from
accidentally be used during XOF operations.

Consider that the arm64 SHA-3 assembly function took a digest_size
argument.  It was tempting to just use __sha3_ctx::digest_size, but of
course that would have been wrong.

But it looks like we could tighten sha3_absorb_blocks() to take a
(sha3_state, block_size) pair.  That would reduce the number of places
in which __sha3_ctx is used.  We should do that anyway.  So I'll
tentatively plan to do that and also move the digest_size to __sha3_ctx.

> Actually, I lean slightly towards passing in the desired digest length
> to sha3_*final() and doing a WARN if it doesn't match.

That would be redundant though.  It would make the API more difficult to
use, especially in the case where the caller is supporting multiple
SHA-3 variants, e.g. crypto/sha3.c.

Note that BLAKE2s has a variable-length digest too, and the BLAKE2s API
only requires that the digest size be passed to init.

I prefer the simpler version that's consistent with the BLAKE2s API.

> > +static inline void sha3_zeroize_ctx(struct sha3_ctx *ctx)
> 
> sha3_zero_ctx() please if you don't like "sha3_clear_ctx".  "zero" is a
> perfectly usable as verb in itself.

In cryptography it's normally zeroize.  See
https://en.wikipedia.org/wiki/Zeroisation

And also try 'git grep zeroize include/crypto/':

    include/crypto/acompress.h: * acomp_request_free() -- zeroize and free asynchronous (de)compression
    include/crypto/aead.h: * crypto_free_aead() - zeroize and free aead handle
    include/crypto/aead.h: * aead_request_free() - zeroize and free request data structure
    include/crypto/akcipher.h: * akcipher_request_free() - zeroize and free public key request
    include/crypto/chacha.h:static inline void chacha_zeroize_state(struct chacha_state *state)
    include/crypto/hash.h: * crypto_free_ahash() - zeroize and free the ahash handle
    include/crypto/hash.h: * ahash_request_free() - zeroize and free the request data structure
    include/crypto/hash.h: * crypto_free_shash() - zeroize and free the message digest handle
    include/crypto/internal/cipher.h: * crypto_free_cipher() - zeroize and free the single block cipher handle
    include/crypto/kpp.h: * kpp_request_free() - zeroize and free kpp request
    include/crypto/md5.h: * After finishing, this zeroizes @ctx.  So the caller does not need to do it.
    include/crypto/md5.h: * After finishing, this zeroizes @ctx.  So the caller does not need to do it.
    include/crypto/rng.h: * crypto_free_rng() - zeroize and free RNG handle
    include/crypto/sha1.h: * After finishing, this zeroizes @ctx.  So the caller does not need to do it.
    include/crypto/sha1.h: * After finishing, this zeroizes @ctx.  So the caller does not need to do it.
    include/crypto/sha2.h: * After finishing, this zeroizes @ctx.  So the caller does not need to do it.
    include/crypto/sha2.h: * After finishing, this zeroizes @ctx.  So the caller does not need to do it.
    include/crypto/sha2.h: * After finishing, this zeroizes @ctx.  So the caller does not need to do it.
    include/crypto/sha2.h: * After finishing, this zeroizes @ctx.  So the caller does not need to do it.
    include/crypto/sha2.h: * After finishing, this zeroizes @ctx.  So the caller does not need to do it.
    include/crypto/sha2.h: * After finishing, this zeroizes @ctx.  So the caller does not need to do it.
    include/crypto/sha2.h: * After finishing, this zeroizes @ctx.  So the caller does not need to do it.
    include/crypto/sha2.h: * After finishing, this zeroizes @ctx.  So the caller does not need to do it.
    include/crypto/skcipher.h: * crypto_free_skcipher() - zeroize and free cipher handle
    include/crypto/skcipher.h: * crypto_free_lskcipher() - zeroize and free cipher handle
    include/crypto/skcipher.h: * skcipher_request_free() - zeroize and free request data structure

Point is, we should prefer standard terminology that is already used in
the code and the wider problem domain, instead of coming up with new
terminology each time a new contributor hops in.

- Eric

Powered by blists - more mailing lists