Re: [PATCH v4 2/2] lib/crypto: tests: Add KUnit tests for ML-DSA verification

[Geert Uytterhoeven]

Hi Eric,

CC kunit

On Sun, 14 Dec 2025 at 19:18, Eric Biggers <ebig...@kernel.org> wrote:
> Add a KUnit test suite for ML-DSA verification, including the following
> for each ML-DSA parameter set (ML-DSA-44, ML-DSA-65, and ML-DSA-87):
>
> - Positive test (valid signature), using vector imported from leancrypto
> - Various negative tests:
> - Wrong length for signature, message, or public key
> - Out-of-range coefficients in z vector
> - Invalid encoded hint vector
> - Any bit flipped in signature, message, or public key
> - Unit test for the internal function use_hint()
> - A benchmark
>
> ML-DSA inputs and outputs are very large. To keep the size of the tests
> down, use just one valid test vector per parameter set, and generate the
> negative tests at runtime by mutating the valid test vector.
>
> I also considered importing the test vectors from Wycheproof. I've
> tested that mldsa_verify() indeed passes all of Wycheproof's ML-DSA test
> vectors that use an empty context string. However, importing these
> permanently would add over 6 MB of source. That's too much to be a
> reasonable addition to the Linux kernel tree for one algorithm. It also
> wouldn't actually provide much better test coverage than this commit.
> Another potential issue is that Wycheproof uses the Apache license.
>
> Similarly, this also differs from the earlier proposal to import a long
> list of test vectors from leancrypto. I retained only one valid
> signature for each algorithm, and I also added (runtime-generated)
> negative tests which were missing. I think this is a better tradeoff.
>
> Reviewed-by: David Howells <dhow...@redhat.com>
> Tested-by: David Howells <dhow...@redhat.com>
> Signed-off-by: Eric Biggers <ebig...@kernel.org>

Thanks for your patch, which is now commit ed894faccb8de55c
("lib/crypto: tests: Add KUnit tests for ML-DSA verification")
in v7.0-rc1.

> --- a/lib/crypto/tests/Kconfig
> +++ b/lib/crypto/tests/Kconfig
> @@ -36,10 +36,19 @@ config CRYPTO_LIB_MD5_KUNIT_TEST
> select CRYPTO_LIB_MD5
> help
> KUnit tests for the MD5 cryptographic hash function and its
> corresponding HMAC.
>
> +config CRYPTO_LIB_MLDSA_KUNIT_TEST
> + tristate "KUnit tests for ML-DSA" if !KUNIT_ALL_TESTS
> + depends on KUNIT
> + default KUNIT_ALL_TESTS || CRYPTO_SELFTESTS
> + select CRYPTO_LIB_BENCHMARK_VISIBLE
> + select CRYPTO_LIB_MLDSA

These two selects mean that enabling KUNIT_ALL_TESTS also enables
extra functionality, which may not be desirable in a production system.
Fortunately CRYPTO_LIB_MLDSA is tristate, so in the modular case
the extra functionality is a module, too, and not part of the running system
by default. Unfortunately CRYPTO_LIB_MLDSA is invisible, so this cannot
just be changed from "select" to "depends on". But as CRYPTO_MLDSA
also selects it, perhaps the test can be made dependent on CRYPTO_MLDSA?

> + help
> + KUnit tests for the ML-DSA digital signature algorithm.
> +
> config CRYPTO_LIB_POLY1305_KUNIT_TEST
> tristate "KUnit tests for Poly1305" if !KUNIT_ALL_TESTS
> depends on KUNIT
> default KUNIT_ALL_TESTS || CRYPTO_SELFTESTS
> select CRYPTO_LIB_BENCHMARK_VISIBLE

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

[Eric Biggers]

"depends on CRYPTO_MLDSA" doesn't make sense, since the test is for the
code in CRYPTO_LIB_MLDSA, not CRYPTO_MLDSA. CRYPTO_MLDSA just happens
to be one of the users of CRYPTO_LIB_MLDSA. In this case the names
happen to be similar, but consider e.g. CRYPTO_LIB_AESGCM which is
selected by AMD_MEM_ENCRYPT. If we added a test for CRYPTO_LIB_AESGCM,
it clearly shouldn't use "depends on AMD_MEM_ENCRYPT".

So, "depends on CRYPTO_LIB_MLDSA" would be the correct way to switch it
from a selection to a dependency.

It's just a bit annoying to do this for hidden symbols, given that it
makes it so that anyone who wants to unconditionally enable the test,
like what I do to test all the crypto library code, has to find and
enable some other random kconfig symbol that enables the code.

Also, the series that originally added CRYPTO_LIB_MLDSA and its test
(https://lore.kernel.org/linux-crypto/20251214181712....@kernel.org/)
didn't add any user of CRYPTO_LIB_MLDSA besides the test, as the real
user came a bit later. So if I had used "depends on CRYPTO_LIB_MLDSA",
my series wouldn't have received any build bot coverage, and I'd have
needed to temporarily carry local patches to build and test the code.

But if this is really the convention for KUnit, as it seems to be, I
will follow it and work around it for my own testing. So I'll plan to
change the crypto library and CRC tests to use "depends on".

But any thoughts from the KUnit maintainers would also be appreciated.
Is it indeed intended that the tests for library modules depend on those
modules rather than selecting them, despite their symbols being hidden?

- Eric

[Eric Biggers]

I sent the following patch to consider:
https://lore.kernel.org/linux-crypto/20260226191749....@kernel.org/

- Eric