|SUCCESS| [2/2] test/crypto: fix prehash in MLDSA session

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164485

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-27

# Review of DPDK Crypto Test Patches

git sha: 20260527085257.1933-1-gmuthukrishn at marvell.com (patch 1/2)
git sha: 20260527085257.1933-2-gmuthukrishn at marvell.com (patch 2/2)

Author: Gowrishankar Muthukrishnan <gmuthukrishn at marvell.com>

Subject: [PATCH 1/2] test/crypto: validate ML crypto keys
Subject: [PATCH 2/2] test/crypto: fix prehash in MLDSA session

These patches add validation for ML-KEM and ML-DSA cryptographic keys
and fix prehash flag handling in ML-DSA sessions.

## Patch 1/2: test/crypto: validate ML crypto keys

> diff --git a/app/test/test_cryptodev_asym.c b/app/test/test_cryptodev_asym.c
> index bf1a1fc417..73a8198e53 100644
> --- a/app/test/test_cryptodev_asym.c
> +++ b/app/test/test_cryptodev_asym.c
> @@ -3949,19 +3949,15 @@ mlkem_keygen(const void *test_data)
>  		rte_crypto_ml_kem_pubkey_size[vector->type],
>  		"Incorrect Encapsulation key length\n");
>  
> -	/* If the seed is all zero, keys are deterministic */
> -	if (memcmp(vector->d.data, (uint8_t [32]) {0},
> -			vector->d.length) == 0) {

Is the compound literal `(uint8_t [32]) {0}` properly scoped? Can this
construct leak memory or cause undefined behavior when used within
memcmp()? The coding style guide indicates compound literals should be
used carefully.

> -		TEST_ASSERT_BUFFERS_ARE_EQUAL(vector->dk.data,
> -			self->result_op->asym->mlkem.keygen.dk.data,
> -			self->result_op->asym->mlkem.keygen.dk.length,
> -			"Incorrect Decapsulation key\n");
> -		TEST_ASSERT_BUFFERS_ARE_EQUAL(vector->ek.data,
> -			self->result_op->asym->mlkem.keygen.ek.data,
> -			self->result_op->asym->mlkem.keygen.ek.length,
> -			"Incorrect Encapsulation key\n");
> -		RTE_LOG(DEBUG, USER1, "Deterministic keygen test passed\n");
> -	}
> +	TEST_ASSERT_BUFFERS_ARE_EQUAL(vector->dk.data,
> +		self->result_op->asym->mlkem.keygen.dk.data,
> +		self->result_op->asym->mlkem.keygen.dk.length,
> +		"Incorrect Decapsulation key\n");
> +	TEST_ASSERT_BUFFERS_ARE_EQUAL(vector->ek.data,
> +		self->result_op->asym->mlkem.keygen.ek.data,
> +		self->result_op->asym->mlkem.keygen.ek.length,
> +		"Incorrect Encapsulation key\n");
> +	RTE_LOG(DEBUG, USER1, "Deterministic keygen test passed\n");

Does removing the conditional check affect test coverage? Should there
be validation that the test is actually using deterministic test vectors
before asserting that "Deterministic keygen test passed"?

[ ... ]

> @@ -4071,7 +4067,7 @@ mlkem_encap(const void *test_data)
>  	self->op->asym->mlkem.decap.sk.data = sk;
>  	self->op->asym->mlkem.decap.sk.length = 32;
>  
> -	TEST_ASSERT_SUCCESS(send_one(),
> +	TEST_ASSERT_SUCCESS(send_one_no_status_check(),
>  		"Failed to process crypto op (ML-KEM Decap)");

Why is the negative test in mlkem_encap() changed from send_one() to
send_one_no_status_check()? Does this hide error conditions that should
be caught?

[ ... ]

> @@ -4149,7 +4145,7 @@ mlkem_decap(const void *test_data)
>  	self->op->asym->mlkem.decap.sk.data = sk;
>  	self->op->asym->mlkem.decap.sk.length = 32;
>  
> -	TEST_ASSERT_SUCCESS(send_one(),
> +	TEST_ASSERT_SUCCESS(send_one_no_status_check(),
>  		"Failed to process crypto op (ML-KEM Decap)");

Same question for mlkem_decap() - does using send_one_no_status_check()
mask failures that should be reported?

[ ... ]

> @@ -4214,19 +4210,15 @@ mldsa_keygen(const void *test_data)
>  		rte_crypto_ml_dsa_pubkey_size[vector->type],
>  		"Incorrect Public key length\n");
>  
> -	/* If the seed is all zero, keys are deterministic */
> -	if (memcmp(vector->seed.data, (uint8_t [32]) {0},
> -			vector->seed.length) == 0) {

Same concern about the compound literal usage here. Can this cause
memory issues?

[ ... ]

> diff --git a/app/test/test_cryptodev_ml_dsa_test_vectors.h b/app/test/test_cryptodev_ml_dsa_test_vectors.h
> index f2453b9f34..675a6fad04 100644
> --- a/app/test/test_cryptodev_ml_dsa_test_vectors.h
> +++ b/app/test/test_cryptodev_ml_dsa_test_vectors.h
> @@ -1653,6 +1653,14 @@ struct crypto_testsuite_mldsa_params mldsa_keygen_test_vectors[] = {
>  			.data = mldsa_44_seed,
>  			.length = sizeof(mldsa_44_seed),
>  		},
> +		.privkey = {
> +			.data = mldsa_44_privkey,
> +			.length = sizeof(mldsa_44_privkey),
> +		},
> +		.pubkey = {
> +			.data = mldsa_44_pubkey,
> +			.length = sizeof(mldsa_44_pubkey),
> +		},

This adds expected key validation data. Are the mldsa_44_privkey and
mldsa_44_pubkey arrays defined elsewhere in the file? If not, does this
cause undefined reference errors?

[ ... ]

> diff --git a/app/test/test_cryptodev_ml_kem_test_vectors.h b/app/test/test_cryptodev_ml_kem_test_vectors.h
> index 8294acc29e..a1d727b969 100644
> --- a/app/test/test_cryptodev_ml_kem_test_vectors.h
> +++ b/app/test/test_cryptodev_ml_kem_test_vectors.h
> @@ -785,6 +785,14 @@ struct crypto_testsuite_mlkem_params mlkem_keygen_test_vectors[] = {
>  			.data = mlkem_512_z,
>  			.length = sizeof(mlkem_512_z),
>  		},
> +		.dk = {
> +			.data = mlkem_512_dk,
> +			.length = sizeof(mlkem_512_dk),
> +		},
> +		.ek = {
> +			.data = mlkem_512_ek,
> +			.length = sizeof(mlkem_512_ek),
> +		}

Missing comma after the closing brace. Does this cause compilation
issues, or is it intentional for this being the last member?

## Patch 2/2: test/crypto: fix prehash in MLDSA session

> diff --git a/app/test/test_cryptodev_asym.c b/app/test/test_cryptodev_asym.c
> index 73a8198e53..90af49e485 100644
> --- a/app/test/test_cryptodev_asym.c
> +++ b/app/test/test_cryptodev_asym.c
> @@ -4236,6 +4236,7 @@ mldsa_sign(const void *test_data)
>  
>  	xform.mldsa.type = vector->type;
>  	xform.mldsa.sign_deterministic = vector->sign_deterministic;
> +	xform.mldsa.sign_prehash = !!vector->hash;

The double negation operator `!!` converts vector->hash to boolean.
While functional, does this comply with the DPDK coding style which
prefers explicit comparisons? Should this be
`vector->hash != RTE_CRYPTO_AUTH_NULL` or a similar explicit check?

[ ... ]

> @@ -4310,6 +4311,7 @@ mldsa_sign(const void *test_data)
>  	self->op->asym->mldsa.sigver.pubkey.length = vector->pubkey.length;
>  	self->op->asym->mldsa.sigver.sign.data = sign;
>  	self->op->asym->mldsa.sigver.sign.length = sign_len;
> +	self->op->asym->mldsa.sigver.hash = vector->hash;

[ ... ]

> @@ -4352,12 +4355,26 @@ mldsa_verify(const void *test_data)
>  	const struct crypto_testsuite_mldsa_params *vector = test_data;
>  	const uint8_t dev_id = params->valid_devs[0];
>  	struct rte_crypto_asym_xform xform = {0};
> +	struct rte_cryptodev_info dev_info;
>  	uint8_t sign[TEST_DATA_SIZE] = {0};
>  	size_t sign_len;
>  
>  	xform.mldsa.type = vector->type;
>  	xform.mldsa.sign_deterministic = vector->sign_deterministic;
> +	xform.mldsa.sign_prehash = !!vector->hash;
>  	xform.xform_type = RTE_CRYPTO_ASYM_XFORM_ML_DSA;
> +
> +	rte_cryptodev_info_get(dev_id, &dev_info);
> +
> +	/* Check if prehash is supported */
> +	if (vector->hash) {

Should this use an explicit comparison instead of an implicit boolean
check per DPDK coding standards?

> +		if (!(dev_info.feature_flags & RTE_CRYPTODEV_FF_MLDSA_SIGN_PREHASH)) {

This conditional should be split for readability. Consider:
```
bool prehash_supported = dev_info.feature_flags &
                         RTE_CRYPTODEV_FF_MLDSA_SIGN_PREHASH;
if (!prehash_supported) {
```

[ ... ]

> @@ -4379,6 +4396,7 @@ mldsa_verify(const void *test_data)
>  	self->op->asym->mldsa.sigver.pubkey.length = vector->pubkey.length;
>  	self->op->asym->mldsa.sigver.sign.data = vector->sign.data;
>  	self->op->asym->mldsa.sigver.sign.length = vector->sign.length;
> +	self->op->asym->mldsa.sigver.hash = vector->hash;