Skip to contentSkip to conversation

lib/crypto: mldsa: Address some minor nits

PATCH In review Eric Biggers 2 participants 6 messages Oct 29

Subsystems (2)
lore
Share & tools
lore

Eric Biggers

author maintainer
#0 lore raw

cover lib/crypto: mldsa: Address some minor nits

A few pedantic fixes for issues found by LLMs.

Eric Biggers (3):
lib/crypto: mldsa: Check pk_len before doing pointer arithmetic lib/crypto: mldsa: Explicitly check that 'alg' is a valid value lib/crypto: mldsa: Add __must_check to mldsa_verify()

 include/crypto/mldsa.h         |  6 ++++--
 lib/crypto/mldsa.c             | 15 +++++++++++----
 lib/crypto/tests/mldsa_kunit.c |  9 +++++++++
 3 files changed, 24 insertions(+), 6 deletions(-)

base-commit: d135ab453a7400a34930bdda1ef4bdc8acfac80e

Signature · 1 line
2.56.0
author maintainer
#1 lore raw

PATCH 1/3 lib/crypto: mldsa: Check pk_len before doing pointer arithmetic

Though common in the kernel and benign in practice (not even triggering UBSAN warnings), taking an address past the end of an array is undefined behavior according to the C specification.

Therefore, compute &pk[RHO_LEN] only after pk_len has been validated, since pk_len could be less than RHO_LEN if an incorrectly sized public key is provided.

(Also fix a typo that isn't worth its own commit.)

Fixes 64edccea594c ("lib/crypto: Add ML-DSA verification support")
---
 lib/crypto/mldsa.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
lib/crypto/mldsa.c +3 -2
index c96fddc4e7dcf..68a1b432315b1 100644
@@ -306,7 +306,7 @@ static const u8 *decode_t1_elem(struct mldsa_ring_elem *out,
  * Reference: FIPS 204 Algorithm 27, sigDecode.
  *
  * This also validates that the coefficients of z are in range, corresponding
- * the infinity norm check at the end of Algorithm 8, ML-DSA.Verify_internal.
+ * to the infinity norm check at the end of Algorithm 8, ML-DSA.Verify_internal.
  *
  * Finally, this also converts z to NTT form.
  */
@@ -533,7 +533,7 @@ int mldsa_verify(enum mldsa_alg alg, const u8 *sig, size_t sig_len,
 	/* For now this just does pure ML-DSA with an empty context string. */
 	static const u8 msg_prefix[2] = { /* dom_sep= */ 0, /* ctx_len= */ 0 };
 	const u8 *ctilde; /* The signer's commitment hash */
-	const u8 *t1_encoded = &pk[RHO_LEN]; /* Next encoded element of t_1 */
+	const u8 *t1_encoded; /* Next encoded element of t_1 */
 	u8 *h; /* The signer's hint vector, length k * N */
 	size_t w1_enc_len;
 
@@ -587,6 +587,7 @@ int mldsa_verify(enum mldsa_alg alg, const u8 *sig, size_t sig_len,
 	 * The computation is the same for each of the k rows.  Just do each row
 	 * before moving on to the next, resulting in only one loop over k.
 	 */
+	t1_encoded = &pk[RHO_LEN];
 	for (int i = 0; i < k; i++) {
 		/*
 		 * tmp = NTT(A) * NTT(z) * 2^-32
Signature · 1 line
2.56.0
author maintainer
#2 lore raw

PATCH 2/3 lib/crypto: mldsa: Explicitly check that 'alg' is a valid value

In reply to Eric Biggers

C doesn't guarantee at the language level that an enum actually has one of the declared values.

For robustness, make mldsa_verify() handle invalid values of 'alg'.

No effect on current callers, who always pass a valid value for 'alg'. (Except for the KUnit test case that this commit adds.)

Fixes 64edccea594c ("lib/crypto: Add ML-DSA verification support")
---
 include/crypto/mldsa.h         |  1 +
 lib/crypto/mldsa.c             | 10 ++++++++--
 lib/crypto/tests/mldsa_kunit.c |  9 +++++++++
 3 files changed, 18 insertions(+), 2 deletions(-)
include/crypto/mldsa.h +1 -0
index 3ef2676787c9e..e0754879e7452 100644
@@ -48,6 +48,7 @@ enum mldsa_alg {
  * Return:
  * * 0 if the signature is valid
  * * -EBADMSG if the signature and/or public key is malformed
+ * * -EINVAL if @alg is not a valid value
  * * -EKEYREJECTED if the signature is invalid but otherwise well-formed
  * * -ENOMEM if out of memory so the validity of the signature is unknown
  */
lib/crypto/mldsa.c +8 -2
index 68a1b432315b1..80e0e63b418a7 100644
@@ -528,8 +528,8 @@ static size_t encode_w1(u8 out[MAX_W1_ENCODED_LEN],
 int mldsa_verify(enum mldsa_alg alg, const u8 *sig, size_t sig_len,
 		 const u8 *msg, size_t msg_len, const u8 *pk, size_t pk_len)
 {
-	const struct mldsa_parameter_set *params = &mldsa_parameter_sets[alg];
-	const int k = params->k, l = params->l;
+	const struct mldsa_parameter_set *params;
+	int k, l;
 	/* For now this just does pure ML-DSA with an empty context string. */
 	static const u8 msg_prefix[2] = { /* dom_sep= */ 0, /* ctx_len= */ 0 };
 	const u8 *ctilde; /* The signer's commitment hash */
@@ -537,6 +537,12 @@ int mldsa_verify(enum mldsa_alg alg, const u8 *sig, size_t sig_len,
 	u8 *h; /* The signer's hint vector, length k * N */
 	size_t w1_enc_len;
 
+	if (alg != MLDSA44 && alg != MLDSA65 && alg != MLDSA87)
+		return -EINVAL;
+	params = &mldsa_parameter_sets[alg];
+	k = params->k;
+	l = params->l;
+
 	/* Validate the public key and signature lengths. */
 	if (pk_len != params->pk_len || sig_len != params->sig_len)
 		return -EBADMSG;
lib/crypto/tests/mldsa_kunit.c +9 -0
index ec64e10c97f25..d0ee704a36944 100644
@@ -372,6 +372,14 @@ static void test_mldsa_use_hint(struct kunit *test)
 	}
 }
 
+static void test_mldsa_invalid_alg(struct kunit *test)
+{
+	KUNIT_ASSERT_EQ(test, -EINVAL,
+			mldsa_verify(MLDSA44 - 1, NULL, 0, NULL, 0, NULL, 0));
+	KUNIT_ASSERT_EQ(test, -EINVAL,
+			mldsa_verify(MLDSA87 + 1, NULL, 0, NULL, 0, NULL, 0));
+}
+
 static void benchmark_mldsa(struct kunit *test,
 			    const struct mldsa_testvector *tv)
 {
@@ -414,6 +422,7 @@ static struct kunit_case mldsa_kunit_cases[] = {
 	KUNIT_CASE(test_mldsa65),
 	KUNIT_CASE(test_mldsa87),
 	KUNIT_CASE(test_mldsa_use_hint),
+	KUNIT_CASE(test_mldsa_invalid_alg),
 	KUNIT_CASE(benchmark_mldsa44),
 	KUNIT_CASE(benchmark_mldsa65),
 	KUNIT_CASE(benchmark_mldsa87),
Signature · 1 line
2.56.0
author maintainer
#3 lore raw

PATCH 3/3 lib/crypto: mldsa: Add __must_check to mldsa_verify()

In reply to Eric Biggers

Signature verification status always must be checked. Add __must_check, for consistency with e.g. the AES-GCM and ChaCha20Poly1305 functions.

No effect on current callers, who always check the return value.

Fixes 64edccea594c ("lib/crypto: Add ML-DSA verification support")
---
 include/crypto/mldsa.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
include/crypto/mldsa.h +3 -2
index e0754879e7452..b156f166a1413 100644
@@ -52,8 +52,9 @@ enum mldsa_alg {
  * * -EKEYREJECTED if the signature is invalid but otherwise well-formed
  * * -ENOMEM if out of memory so the validity of the signature is unknown
  */
-int mldsa_verify(enum mldsa_alg alg, const u8 *sig, size_t sig_len,
-		 const u8 *msg, size_t msg_len, const u8 *pk, size_t pk_len);
+int __must_check mldsa_verify(enum mldsa_alg alg, const u8 *sig, size_t sig_len,
+			      const u8 *msg, size_t msg_len, const u8 *pk,
+			      size_t pk_len);
 
 #if IS_ENABLED(CONFIG_CRYPTO_LIB_MLDSA_KUNIT_TEST)
 /* Internal function, exposed only for unit testing */
Signature · 1 line
2.56.0
maintainer Reviewed-by on the v1 series
#4 lore raw
In reply to Eric Biggers
Quoted 7 lines · Eric Biggers

A few pedantic fixes for issues found by LLMs.

Eric Biggers (3):
lib/crypto: mldsa: Check pk_len before doing pointer arithmetic lib/crypto: mldsa: Explicitly check that 'alg' is a valid value lib/crypto: mldsa: Add __must_check to mldsa_verify()

Quoted 6 lines
 include/crypto/mldsa.h         |  6 ++++--
 lib/crypto/mldsa.c             | 15 +++++++++++----
 lib/crypto/tests/mldsa_kunit.c |  9 +++++++++
 3 files changed, 24 insertions(+), 6 deletions(-)
author maintainer Applied
#5 lore raw
Quoted 10 lines · Ard Biesheuvel
Quoted 7 lines · Eric Biggers

A few pedantic fixes for issues found by LLMs.

Eric Biggers (3):
lib/crypto: mldsa: Check pk_len before doing pointer arithmetic lib/crypto: mldsa: Explicitly check that 'alg' is a valid value lib/crypto: mldsa: Add __must_check to mldsa_verify()

Quoted 8 lines
Quoted 6 lines
 include/crypto/mldsa.h         |  6 ++++--
 lib/crypto/mldsa.c             | 15 +++++++++++----
 lib/crypto/tests/mldsa_kunit.c |  9 +++++++++
 3 files changed, 24 insertions(+), 6 deletions(-)

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git/log/?h=libcrypto-fixes

- Eric