linux-kernel - Re: [PATCH 1/4] lib/crypto: Add ML-DSA verification support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2624664.1763646918@warthog.procyon.org.uk>
Date: Thu, 20 Nov 2025 13:55:18 +0000
From: David Howells <dhowells@...hat.com>
To: Eric Biggers <ebiggers@...nel.org>
Cc: dhowells@...hat.com, linux-crypto@...r.kernel.org,
    Herbert Xu <herbert@...dor.apana.org.au>,
    Luis Chamberlain <mcgrof@...nel.org>,
    Petr Pavlu <petr.pavlu@...e.com>, Daniel Gomez <da.gomez@...nel.org>,
    Sami Tolvanen <samitolvanen@...gle.com>,
    "Jason A . Donenfeld" <Jason@...c4.com>,
    Ard Biesheuvel <ardb@...nel.org>,
    Stephan Mueller <smueller@...onox.de>,
    Lukas Wunner <lukas@...ner.de>,
    Ignat Korchagin <ignat@...udflare.com>, keyrings@...r.kernel.org,
    linux-modules@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] lib/crypto: Add ML-DSA verification support

Eric Biggers <ebiggers@...nel.org> wrote:

> +	/* Compute d = (c mod 2^32) * (q^-1 mod 2^32). */
> +	s32 d = (s32)c * QINV_MOD_R;

Hmmm...  is "(s32)c" actually "(c mod 2^32)"?  Should that be:

	u32 d = (u32)c * QINV_MOD_R;

This is followed up by casting 'd' to "s64".  I don't think that should
sign-extend it, but...

> +	for (int m = 0, len = 128; len >= 1; len /= 2) {

Can you put "int m = 0" outside of the for-statement?  I know putting it
inside saves a line or two, but 'm' is not the loop counter - which it seems
like it should be by virtue of being listed first.

> +	for (int m = 256, len = 1; len < 256; len *= 2) {

Ditto.

> +static const u8 *decode_t1_elem(struct mldsa_ring_elem *out,
> +				const u8 *t1_encoded)

I think this is (more or less) pkDecode()?  Can you put something like:

  * Decode the vector 't1' from the public key.
  * Reference: FIPS 204 Algorithm 23, sigDecode.

in the comment before it?

> +/*
> + * Use @seed to generate a ring element @c with coefficients in {-1, 0, 1},
> + * exactly @tau of them nonzero.  Reference: FIPS 204 Algorithm 29, SampleInBall
> + */
> +static void sample_in_ball(struct mldsa_ring_elem *c, const u8 *seed,
> +			   size_t seed_len, int tau, struct shake_ctx *shake)

Should "seed" actually be labelled "rho"?  I know a seed is what it is, but
the algo description has a different label - and the caller passes it ctilde,
not rho:-/.

> +	u8 (*h)[N]; /* The signer's hint vector, length k */
> +	h = (u8 (*)[N])&ws->z[l];

C is weird sometimes.

> +		/* Reduce to [0, q), then tmp = w'_1 = UseHint(h, w'_Approx) */

Bracket mismatch.  "[0, q]"

> +		/* w1Encode(w'_1) */
> +		w1_pos = 0;
> ...

Given you put the decode functions into helpers, don't you want to do that
with this?

> +	if (memcmp(ws->ctildeprime, ctilde, params->ctilde_len) != 0)
> +		return -EBADMSG;

Actually, this should return -EKEYREJECTED, not -EBADMSG.

I guess you don't need to use crypto_memneq() as timing doesn't matter.

The maths look okay, I think.  You can add:

	Reviewed-by: David Howells <dhowells@...hat.com>

David