Re: [PATCH v2] lib/crypto: Add SHA3-224, SHA3-256, SHA3-384, SHA-512, SHAKE128, SHAKE256

[David Howells <dhowells@redhat.com>]

Eric Biggers <ebiggers@kernel.org> wrote:

> This should be based on libcrypto-next.

This?

https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git libcrypto-next

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

Sure.  But I'll wait a bit to see if there are any other comments first.

> 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.

I don't really have time at the moment.  Nor am I particularly familiar with
the optimised asm instructions for this on any arch.

> In kerneldoc comments, please make it clear that lengths are measured in
> bytes,

Sure.  The hash algos do love talking in terms of bits.  Possibly because
bytes aren't necessarily 8 bits in size on all arches?

> and that the functions can be called in any context.

"Context" as in?

> 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.

Yes, I'm aware of that.  The hash-test-template template is rather rigid and
not always correct in its assertions (for instance requiring the final
function to have zeroed the context - I had to modify my API to work around
the testsuite).

> 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 disagree.  It should have at least a single self-test.  If we fail to load
any modules because the hash is broken on a particular CPU, it would be useful
to have a note in dmesg.  Loading kunit test modules becomes tricky in such a
case.

> 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).

Yes, but I believe you wanted me to use hash-test-template.  The problem is
that it hard-encodes by macroisation of the #include's file various parameters
including the hash size.

> 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.

I could do that.  It would at least exercise the common engine.

Possibly all 5 different block sizes employed (128, 224, 256, 384 and 512)
need to be so tested - but that only affects how much of the state array is
modified directly sha3_update() and how much can be extracted from by
sha3_squeeze().  The actual keccak mixing function doesn't care.

Other than that, the only differences between the algorithms are the padding
char and how much digest is extracted by default.

On top of that, you can have a variety of different usage sequences: different
sequences of updating and squeezing with different amounts of data added and
extracted.  I wonder if a small sampling is sufficient or whether we need some
loopy thing that tries to exhaustively test a portion of the sample space.

David