Message-ID: <20250919190413.GA2249@quark>
Date: Fri, 19 Sep 2025 14:04:13 -0500
From: Eric Biggers <ebiggers@...nel.org>
To: David Howells <dhowells@...hat.com>
Cc: "Jason A. Donenfeld" <Jason@...c4.com>,
	Ard Biesheuvel <ardb@...nel.org>,
	Harald Freudenberger <freude@...ux.ibm.com>,
	Holger Dengler <dengler@...ux.ibm.com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	Stephan Mueller <smueller@...onox.de>, Simo Sorce <simo@...hat.com>,
	linux-crypto@...r.kernel.org, linux-s390@...r.kernel.org,
	keyrings@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] lib/crypto: Add SHA3-224, SHA3-256, SHA3-384,
 SHA-512, SHAKE128, SHAKE256

On Fri, Sep 19, 2025 at 05:31:59PM +0100, David Howells wrote:
> Add SHA3, providing SHA3-224, SHA3-256, SHA3-384, SHA-512, SHAKE128 and
> SHAKE256 to lib/crypto.
> 
> The state array handling is simplified from what's in crypto/sha3_generic.c
> by keeping the state array (a u64[25]) in LE form and byteswapping all the
> entries before and after applying the keccak function on a BE system.  This
> means no byteswapping is required when XOR'ing data into the state array or
> when extracting the digest.  Further, this is a no-op on LE systems.
> 
> Also:
> 
>  - Perform a multistage shake256 hash check in the module initialisation.
> 
>  - Add kunit tests for each algorithm based on the gen-hash-testvecs.
> 
>  - The conflicting static s390x crypto function names are renamed to have
>    an s390_ prefix.
> 
>  - gen-hash-testvecs.py had to be modified to be able to generate SHAKE
>    hashes because Python's hashlib requires the output digest size
>    supplying for those two algorithms as they produce arbitrary length
>    digests.
> 
> Notes:
> 
>  (1) I've left hooks in sha3.c for asm-optimised variants, but as I don't
>      entirely know what those might look like, not having implemented any,
>      the hooks' usability is uncertain.
> 
>  (2) The SHAKE algorithms will be required for ML-DSA.
> 
> Signed-off-by: David Howells <dhowells@...hat.com>
> cc: Eric Biggers <ebiggers@...nel.org>
> cc: Jason A. Donenfeld <Jason@...c4.com>
> cc: Ard Biesheuvel <ardb@...nel.org>
> cc: Harald Freudenberger <freude@...ux.ibm.com>
> cc: Holger Dengler <dengler@...ux.ibm.com>
> cc: Herbert Xu <herbert@...dor.apana.org.au>
> cc: Stephan Mueller <smueller@...onox.de>
> cc: linux-crypto@...r.kernel.org
> cc: linux-s390@...r.kernel.org
> ---    
>  Changes
>  =======
>  v2)
>   - Simplify the endianness handling.
>  
>   - Rename sha3_final() to sha3_squeeze() and don't clear the context at the
>     end as it's permitted to continue calling sha3_final() to extract
>     continuations of the digest (needed by ML-DSA).
>  
>   - Don't reapply the end marker to the hash state in continuation
>     sha3_squeeze() unless sha3_update() gets called again (needed by
>     ML-DSA).
>  
>   - Give sha3_squeeze() the amount of digest to produce as a parameter
>     rather than using ctx->digest_size and don't return the amount digested.
>  
>   - Reimplement sha3_final() as a wrapper around sha3_squeeze() that
>     extracts ctx->digest_size amount of digest and then zeroes out the
>     context.  The latter is necessary to avoid upsetting
>     hash-test-template.h.
>  
>   - Provide a sha3_reinit() function to clear the state, but to leave the
>     parameters that indicate the hash properties unaffected, allowing for
>     reuse.
>  
>   - Provide a sha3_set_digestsize() function to change the size of the
>     digest to be extracted by sha3_final().  sha3_squeeze() takes a
>     parameter for this instead.
>  
>   - Don't pass the digest size as a parameter to shake128/256_init() but
>     rather default to 128/256 bits as per the function name.
>  
>   - Provide a sha3_clear() function to zero out the context.
> 
>  arch/s390/crypto/sha3_256_s390.c          |   26 -
>  include/crypto/sha3.h                     |  160 +++++++-
>  lib/crypto/Kconfig                        |    7 
>  lib/crypto/Makefile                       |    6 
>  lib/crypto/sha3.c                         |  597 ++++++++++++++++++++++++++++++
>  lib/crypto/tests/Kconfig                  |   12 
>  lib/crypto/tests/Makefile                 |    7 
>  lib/crypto/tests/sha3_224_kunit.c         |   32 +
>  lib/crypto/tests/sha3_224_testvecs.h      |  231 +++++++++++
>  lib/crypto/tests/sha3_256_kunit.c         |   32 +
>  lib/crypto/tests/sha3_256_testvecs.h      |  231 +++++++++++
>  lib/crypto/tests/sha3_384_kunit.c         |   32 +
>  lib/crypto/tests/sha3_384_testvecs.h      |  281 ++++++++++++++
>  lib/crypto/tests/sha3_512_kunit.c         |   32 +
>  lib/crypto/tests/sha3_512_testvecs.h      |  331 ++++++++++++++++
>  lib/crypto/tests/sha3_shake128_kunit.c    |   37 +
>  lib/crypto/tests/sha3_shake128_testvecs.h |  181 +++++++++
>  lib/crypto/tests/sha3_shake256_kunit.c    |   37 +
>  lib/crypto/tests/sha3_shake256_testvecs.h |  231 +++++++++++
>  scripts/crypto/gen-hash-testvecs.py       |    8 
>  20 files changed, 2495 insertions(+), 16 deletions(-)

Thanks for working on this!  Some preliminary comments (it will take a
few days for me to find time to fully review this):

This should be based on libcrypto-next.  And as with any kernel patch,
it should include a base-commit so that people know where it applies to.

This should be split into three patches: (1) the arch/s390/ changes, (2)
adding the library functions themselves, and (3) adding the tests.

We'll also need to integrate the existing arch-optimized SHA-3 code, and
reimplement the SHA-3 crypto_shash algorithms on top of the library.
Let me know whether you're planning to do that to.  If not, I can do it.

In kerneldoc comments, please make it clear that lengths are measured in
bytes, and that the functions can be called in any context.

The testing situation looks odd.  This patch adds six KUnit test suites:
one for each of the SHA-3 algorithms.  But they only include the
hash-test-template.h test cases, and they don't test the unique behavior
of SHAKE.  The KUnit tests need to fully test the library.

I see you also have a test in sha3_mod_init(), which doesn't make sense.
The tests should be in the KUnit test suite(s).  If you intended for the
sha3_mod_init() test to be a FIPS pre-operational self-test, then (1) it
would first need to be confirmed with the people doing FIPS
certifications that a FIPS pre-operational self-test is actually
necessary here, (2) it would need to be fixed to actually fulfill the
requirements for that type of test such as panicing the kernel on
failure, and (3) it would need to come in its own patch with its own
explanation.  But, unless you are sure you actually need the FIPS test,
just omit it out for now and focus on the real tests.

I also think that splitting the SHA-3 tests into six KUnit test suites
is awkward.  I know I did something similar for SHA-2, but it made more
sense for SHA-2 because (1) there are only four SHA-2 variants, (2)
SHA-256 and SHA-512 don't share any code, and (3) there wasn't anything
more to add on top of hash-test-template.h.  In contrast, SHA-3 has six
variants, which all share most of their code, and there will need to be
SHA-3 specific tests (for the XOFs).

I think what I'd recommend is creating a single sha3_kunit test suite.
Make it instantiate hash-test-template.h once to test one of the
algorithms, maybe SHA3-256.  Then add test cases (that is, additional
KUnit test cases in the same KUnit test suite) that cover the code
specific to the other variants, including the XOFs.

- Eric

Powered by blists - more mailing lists