Eric Biggers <[email protected]> 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 <[email protected]>
David
