|SUCCESS| [v2,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/164487

_AI Code Review_

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

I'll review this DPDK patch series against the coding standards and contribution guidelines.

Commit: 76a5877072c0 (v2 1/2)
Author: Gowrishankar Muthukrishnan <gmuthukrishn at marvell.com>
Subject: test/crypto: validate ML crypto keys

This patch validates ML-KEM and ML-DSA keys by enabling key verification
in test cases.

> 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) {
> -		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 this change break existing test cases that rely on non-deterministic
key generation? The removed conditional checked if the seed was all zeros
before validating keys. Now the code unconditionally validates against
vector->dk and vector->ek. What happens when vector->dk.data or
vector->ek.data are NULL or invalid for non-deterministic test vectors?

>  
>  	rte_cryptodev_asym_session_free(dev_id, self->sess);
>  	return TEST_SUCCESS;
> @@ -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(),

Can this mask legitimate errors? The function name suggests status
checking is bypassed. What specific error condition necessitates
ignoring the status in the negative test case?

>  		"Failed to process crypto op (ML-KEM Decap)");
>  
>  	debug_hexdump(stdout, "Shared secret from negative test",
> @@ -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(),

Similarly, does bypassing status checks in mlkem_decap() hide errors
that should be caught?

>  		"Failed to process crypto op (ML-KEM Decap)");
>  
>  	debug_hexdump(stdout, "Shared secret from negative test",
> @@ -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) {
> -		TEST_ASSERT_BUFFERS_ARE_EQUAL(vector->privkey.data,
> -			self->result_op->asym->mldsa.keygen.privkey.data,
> -			self->result_op->asym->mldsa.keygen.privkey.length,
> -			"Incorrect Private key\n");
> -		TEST_ASSERT_BUFFERS_ARE_EQUAL(vector->pubkey.data,
> -			self->result_op->asym->mldsa.keygen.pubkey.data,
> -			self->result_op->asym->mldsa.keygen.pubkey.length,
> -			"Incorrect Public key\n");
> -		RTE_LOG(DEBUG, USER1, "Deterministic keygen test passed\n");
> -	}
> +	TEST_ASSERT_BUFFERS_ARE_EQUAL(vector->privkey.data,
> +		self->result_op->asym->mldsa.keygen.privkey.data,
> +		self->result_op->asym->mldsa.keygen.privkey.length,
> +		"Incorrect Private key\n");
> +	TEST_ASSERT_BUFFERS_ARE_EQUAL(vector->pubkey.data,
> +		self->result_op->asym->mldsa.keygen.pubkey.data,
> +		self->result_op->asym->mldsa.keygen.pubkey.length,
> +		"Incorrect Public key\n");
> +	RTE_LOG(DEBUG, USER1, "Deterministic keygen test passed\n");

Same concern as with mlkem_keygen(): Are vector->privkey.data and
vector->pubkey.data guaranteed to be valid for all test vectors that
will invoke mldsa_keygen()?

[ ... ]

Commit: 76a5877072c0 (v2 2/2)
Author: Gowrishankar Muthukrishnan <gmuthukrishn at marvell.com>
Subject: test/crypto: fix prehash in MLDSA session

This patch sets the prehash flag in MLDSA session based on test
parameters.

> 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 is unnecessary for boolean conversion in C
code. Does vector->hash have a type that requires this explicit
conversion, or can this be simplified to assign vector->hash directly?

>  	xform.xform_type = RTE_CRYPTO_ASYM_XFORM_ML_DSA;
>  
>  	rte_cryptodev_info_get(dev_id, &dev_info);
> @@ -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;
>  
>  	TEST_ASSERT_SUCCESS(send_one(),
>  		"Failed to process crypto op (ML-DSA Verify)");
> @@ -4332,6 +4334,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;
>  
>  	TEST_ASSERT_SUCCESS(send_one_no_status_check(),
>  		"Failed to process crypto op (ML-DSA Verify)");
> @@ -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;

Same unnecessary double negation as mentioned above.

>  	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) {
> +		if (!(dev_info.feature_flags & RTE_CRYPTODEV_FF_MLDSA_SIGN_PREHASH)) {
> +			RTE_LOG(DEBUG, USER1,
> +				"Device doesn't support prehash in ML-DSA verify. Test skipped\n");
> +			return TEST_SKIPPED;
> +		}
> +	}
> +

Does this code properly clean up any resources allocated before the skip
condition? Although xform initialization happens on the stack, is there
any other initialization in the call chain that might need cleanup?

[ ... ]

> @@ -4403,6 +4421,7 @@ mldsa_verify(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;
>  
>  	TEST_ASSERT_SUCCESS(send_one_no_status_check(),
>  		"Failed to process crypto op (ML-DSA Verify)");