Message-ID: <20250926195958.GA2163@sol>
Date: Fri, 26 Sep 2025 12:59:58 -0700
From: Eric Biggers <ebiggers@...nel.org>
To: David Howells <dhowells@...hat.com>
Cc: "Jason A . Donenfeld" <Jason@...c4.com>,
	Ard Biesheuvel <ardb@...nel.org>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	Stephan Mueller <smueller@...onox.de>, linux-crypto@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 0/8] crypto, lib/crypto: Add SHAKE128/256 support and
 move SHA3 to lib/crypto

Hi David,

On Fri, Sep 26, 2025 at 03:19:43PM +0100, David Howells wrote:
> I have done what Eric required and made a separate wrapper struct and set
> of wrapper functions for each algorithm, though I think this is excessively
> bureaucratic as this multiplies the API load by 7 (and maybe 9 in the
> future[*]).

I don't think I "required" that it be implemented in exactly this way.
Sorry if I wasn't clear.  Let me quote what I wrote:

    First, this patch's proposed API is error-prone due to the weak
    typing that allows mixing steps of different algorithms together.
    For example, users could initialize a sha3_ctx with sha3_256_init()
    and then squeeze an arbitrary amount from it, incorrectly treating
    it as a XOF.  It would be worth considering separating the APIs for
    the different algorithms that are part of SHA-3, similar to what I
    did with SHA-224 and SHA-256.  (They would of course still share
    code internally, just like SHA-2.)

So I asked that to prevent usage errors such as treating a digest as a
XOF, you consider separating the APIs.  There is more than one way to do
that, and I was hoping that you'd consider different ways.  One way is
separate functions and types for all six SHA-3 algorithms.

However, if that is not scaling well, then we could instead just
separate the SHA-3 algorithms into two groups, the digests and the XOFs:

    void sha3_224_init(struct sha3_ctx *ctx);
    void sha3_256_init(struct sha3_ctx *ctx);
    void sha3_384_init(struct sha3_ctx *ctx);
    void sha3_512_init(struct sha3_ctx *ctx);
    void sha3_update(struct sha3_ctx *ctx, const u8 *data, size_t data_len);
    void sha3_final(struct sha3_ctx *ctx, u8 *out);

    void shake128_init(struct shake_ctx *ctx);
    void shake256_init(struct shake_ctx *ctx);
    void shake_update(struct shake_ctx *ctx, const u8 *data, size_t data_len);
    void shake_squeeze(struct shake_ctx *ctx, u8 *out, size_t out_len);
    void shake_clear(struct shake_ctx *ctx);

(With "sha3_ctx" being used for the digests specifically, the internal
context struct would then have to have a third name, like "__sha3_ctx".)

The *_init() functions would store the correct information in the
context so that the other functions would know what to do.  This would
be similar to how blake2s_init() saves the 'outlen' for blake2s_final().

That would be sufficient to prevent misuse errors where steps of
different algorithms are mixed together, right?

Keep in mind that for SHA-2 we have to have completely different code
and underlying state for the 32-bit hashes (SHA-224 and SHA-256) and
64-bit hashes (SHA-384 and SHA-512) anyway.  We also traditionally
haven't kept any information in the SHA-2 context about which SHA-2
algorithm is being executed.  So that led us more down the road of the
separate functions and types for each SHA-2 algorithm.  With SHA-3,
where e.g. the 224, 256, 384, and 512-bit digests all use the same
underlying state, a slightly more unified API might be appropriate.

All I'm really requesting is that we don't create footguns, like the
following that the API in the v2 patch permitted:

    1. sha3_init() + sha3_update()
        [infinite loop]

    2. sha3_256_init() + sha3_update() + sha3_squeeze()
        [not valid, treats SHA3-256 as a XOF]

    3. sha3_update() + sha3_squeeze() + sha3_update() + sha3_squeeze()
        [not valid, as discussed]

(1) is prevented just by not having the internal function sha3_init() as
a public function.

Splitting the context into two types, one for the digests and one for
the XOFs, is sufficient to prevent (2), as long as there's still one
init function per algorithm.  We don't necessarily need six types.

(3) isn't preventable via the type system, but it's detectable by a
run-time check, which you've done by adding a WARN_ON_ONCE() to
sha3_update().

So, I think we'd be in a good position with just the digests and XOFs
separated out into different functions + types.

> This does, however, cause a problem for what I need to do as the ML-DSA
> prehash is dynamically selectable by certificate OID, so I have to add
> SHAKE128/256 support to the crypto shash API too - though hopefully it will
> only require an output of 16 or 32 bytes respectively for the prehash case
> and won't require multiple squeezing.

When there's only a small number of supported algorithms, just doing the
dispatch in the calling code tends to be simpler than using
crypto_shash.  For example, see the recent conversion of fs/verity/ to
use the SHA-2 library API instead of crypto_shash.

- Eric

Powered by blists - more mailing lists