|WARNING| [v2,4/4] test/crypto: add asym test suite for AESNI_MB

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/171092

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-12

# DPDK Patch Review

## Patch 1: crypto/ipsec_mb: add support for IPsecMB 3.0

### Errors

**Patch 1 line 117-120 (pmd_aesni_gcm.c wrapper functions):**
```c
#define _fn(bitlen) static void imb_aes##bitlen##_gcm_pre(const void *key, \
		struct gcm_key_data *key_data, IMB_MGR *state) \
{ \
	state->gcm##bitlen##_pre(key, key_data); \
}
```
The wrapper function name declares `IMB_MGR *state` but the library function pointer stored in `state` is being called without checking if `state` is NULL. However, these wrappers are only used when `IMB_VERSION(3, 0, 0) > IMB_VERSION_NUM` (pre-3.0), and in that case `state->gcm##bitlen##_pre` is the function pointer from the old API. The new code in `aesni_gcm_session_configure()` (lines 287-295) and `aesni_gcm_set_ops()` (lines 152-210) does not check `mb_mgr` for NULL before dereferencing. If `aesni_gcm_session_configure()` is called with a NULL `mb_mgr`, line 287 `imb_aes128_gcm_pre(key, &sess->gdata_key, mb_mgr)` would pass NULL to the wrapper, which would then dereference it. The function signature does not document that `mb_mgr` must be non-NULL, and the callers in the tree (via `ipsec_mb_session_configure()`) obtain `mb_mgr` from `qp->mb_mgr` or `alloc_init_mb_mgr()`, but no NULL check is visible in the patch.

**Patch 1 line 152-157 (aesni_gcm_set_ops):**
```c
static void
aesni_gcm_set_ops(struct aesni_gcm_ops *ops, IMB_MGR *mb_mgr)
{
	/* Set IMB_MGR pointers. */
	ops[GCM_KEY_128].mb_mgr = mb_mgr;
	ops[GCM_KEY_192].mb_mgr = mb_mgr;
	ops[GCM_KEY_256].mb_mgr = mb_mgr;
```
No NULL check on `mb_mgr` or `ops` before dereferencing. If `mb_mgr` is NULL, storing it and later dereferencing `ops->mb_mgr` in the crypto fast path will cause a NULL pointer dereference. The calling code (in other files not shown in the patch) must ensure `mb_mgr` is non-NULL, but this function does not document or enforce that precondition.

**Patch 1 line 84 (pmd_aesni_mb.c):**
```c
#define _fn(bitlen) static void imb_aes##bitlen##_gcm_pre(const void *key, \
```
This is a different file from the first finding but the same macro pattern. The issue is the same: no NULL check on the `state` parameter that will be dereferenced.

### Warnings

**Patch 1 line 287-295 (aesni_gcm_session_configure key expansion):**
```c
	switch (key_length) {
	case 16:
		sess->key_length = GCM_KEY_128;
		imb_aes128_gcm_pre(key, &sess->gdata_key, mb_mgr);
		break;
	case 24:
		sess->key_length = GCM_KEY_192;
		imb_aes192_gcm_pre(key, &sess->gdata_key, mb_mgr);
		break;
	case 32:
		sess->key_length = GCM_KEY_256;
		imb_aes256_gcm_pre(key, &sess->gdata_key, mb_mgr);
		break;
```
The `key` pointer (from `xform->aead.key.data`) is not checked for NULL before being passed to the pre-expansion functions. If the application supplies a NULL key pointer (which would be a bug in the application, but defensible), this would pass NULL to the library. The library behavior with NULL is not specified in the patch. However, the `key_length` has already been validated (lines 272-275), so a NULL key with a valid length would be caught by the library or would be an application error. Still, a NULL check here would harden the code.

**Patch 1 line 473-508 (process_gcm_crypto_op encrypt path):**
The code calls `ops->init`, `ops->update_enc`, and `ops->finalize_enc` in a loop over multiple segments. If `ops` or any of the function pointers in `ops` are NULL, this will segfault. The `ops` struct is populated by `aesni_gcm_set_ops()` which assigns all function pointers, so they should not be NULL unless there is memory corruption. However, there is no runtime check. This is acceptable as the function pointers are set at init time and are not expected to be NULL in production, but defensive programming would check `ops` at the start of the function.

**Patch 1 line 504-508 (process_gcm_crypto_op decrypt path):**
Same issue as encrypt: no NULL checks on `ops` or its function pointers before calling them in a loop.

---

## Patch 2: crypto/ipsec_mb: add support for 256-NxA4/5/6 algorithms

### Warnings

**Patch 2 line 238-247 (aesni_mb_set_session_auth_parameters SNOW5G_NIA4):**
```c
	if (xform->auth.algo == RTE_CRYPTO_AUTH_SNOW5G_NIA4) {
		sess->template_job.hash_alg = IMB_AUTH_SNOW5G_NIA4;
		sess->template_job.auth_tag_output_len_in_bytes =
			sess->auth.req_digest_len;
		memcpy(sess->auth.snow5g_auth_key, xform->auth.key.data,
			xform->auth.key.length);
		sess->template_job.u.NIA._key = sess->auth.snow5g_auth_key;
		return 0;
	}
```
No check on `xform->auth.key.length` before using it in `memcpy`. If the key length is invalid (e.g., not 32 bytes for this algorithm), the memcpy could overflow the `sess->auth.snow5g_auth_key` buffer (which is declared as `uint8_t snow5g_auth_key[32]` in patch line 1333). The code should validate the key length against the algorithm's requirements before copying. The same issue exists for AES_NIA5 (lines 248-258) and ZUC_NIA6 (lines 259-267).

**Patch 2 line 248-258 (aesni_mb_set_session_auth_parameters AES_NIA5):**
```c
	} else if (xform->auth.algo == RTE_CRYPTO_AUTH_AES_NIA5) {
		sess->template_job.hash_alg = IMB_AUTH_AES_NIA5;
		sess->template_job.auth_tag_output_len_in_bytes =
			sess->auth.req_digest_len;
		IMB_AES_KEYEXP_256(mb_mgr, xform->auth.key.data,
			sess->cipher.expanded_aes_keys.encode,
			sess->cipher.expanded_aes_keys.decode);
		sess->template_job.u.NIA._key =
			(uint8_t *)sess->cipher.expanded_aes_keys.encode;
		return 0;
	}
```
`IMB_AES_KEYEXP_256` is called with `xform->auth.key.data` which could be NULL if the application misconfigures the xform. The function does not check for NULL before passing to the library. While this is an application bug, defensive programming would add a NULL check.

**Patch 2 line 643-667 (aesni_mb_set_session_cipher_parameters NxAN ciphers):**
```c
	} else if (is_nxan) {
		if (xform->cipher.key.length != 32) {
			IPSEC_MB_LOG(ERR, "Invalid cipher key length");
			return -EINVAL;
		}
		if (xform->cipher.iv.length != 16) {
			IPSEC_MB_LOG(ERR, "Invalid cipher IV length");
			return -EINVAL;
		}
		sess->template_job.key_len_in_bytes = 32;
		if (is_nxan_exp_key) {
			IMB_AES_KEYEXP_256(mb_mgr, xform->cipher.key.data,
					sess->cipher.expanded_aes_keys.encode,
					sess->cipher.expanded_aes_keys.decode);
			sess->template_job.enc_keys = sess->cipher.expanded_aes_keys.encode;
			sess->template_job.dec_keys = sess->cipher.expanded_aes_keys.decode;
		} else {
			memcpy(sess->cipher.nxa_cipher_key, xform->cipher.key.data, 32);
			sess->template_job.enc_keys = sess->cipher.nxa_cipher_key;
			sess->template_job.dec_keys = sess->cipher.nxa_cipher_key;
		}
```
The key length is validated (line 644) but `xform->cipher.key.data` is not checked for NULL before being passed to `IMB_AES_KEYEXP_256` or `memcpy`. If `key.data` is NULL, this will crash.

**Patch 2 line 820-829 (aesni_mb_set_session_aead_parameters SNOW5G_NCA4):**
```c
		if (xform->aead.key.length != 32) {
			IPSEC_MB_LOG(ERR, "Invalid key length");
			return -EINVAL;
		}
		sess->template_job.key_len_in_bytes = 32;
		memcpy(sess->cipher.nxa_cipher_key, xform->aead.key.data, 32);
```
Key length validated but `xform->aead.key.data` not checked for NULL before `memcpy`. Same issue for AES_NCA5 (line 843) and ZUC_NCA6 (line 863).

---

## Patch 3: crypto/ipsec_mb: add support for ML-KEM and ML-DSA

### Errors

**Patch 3 line 1008-1012 (aesni_mb_asym_session_configure ML_KEM keygen):**
```c
		ret = imb_ml_kem_new(mb_mgr, sess->asym.ml_kem.alg, &sess->asym.ml_kem.ctx);
		if (ret) {
			IPSEC_MB_LOG(ERR, "Failed to create ML KEM session");
			return ret;
		}
```
If `imb_ml_kem_new()` fails (returns non-zero), the function returns `ret` (the error code from the library). However, the session memory (`priv_sess`) is not freed or cleared before returning. The caller (`ipsec_mb_asym_session_configure()` in `ipsec_mb_ops.c`, line 987-1001) does not show a cleanup path on error, so the session memory is leaked. The same issue exists for ML_DSA on line 1033.

**Patch 3 line 2089-2092 (aesni_mb_ml_kem_ek_sz and related size functions):**
```c
static inline size_t aesni_mb_ml_kem_ek_sz(const IMB_ML_KEM_ALG alg)
{
		switch (alg) {
		case IMB_ML_KEM_512:
				return IMB_ML_KEM_512_PUBKEY_BYTES;
		case IMB_ML_KEM_768:
				return IMB_ML_KEM_768_PUBKEY_BYTES;
		case IMB_ML_KEM_1024:
				return IMB_ML_KEM_1024_PUBKEY_BYTES;
		}
		return 0;
}
```
The function returns 0 if `alg` is not one of the three valid values. The callers (lines 2190, 2202, 2213) use the return value to set lengths (`keygen->ek.length = aesni_mb_ml_kem_ek_sz(...)`) without checking if it is 0. If an invalid `alg` value is passed (which would be a bug in `aesni_mb_asym_session_configure()` or memory corruption), the length is set to 0 and the subsequent call to `imb_ml_kem_keypair()` (line 2193) would receive a 0-length buffer, leading to undefined behavior or silent data corruption. The same issue exists for `aesni_mb_ml_kem_dk_sz`, `aesni_mb_ml_kem_ct_sz`, `aesni_mb_ml_dsa_pk_sz`, `aesni_mb_ml_dsa_sk_sz`, and `aesni_mb_ml_dsa_sig_sz`.

**Patch 3 line 2185-2199 (aesni_mb_asym_process ML_KEM keygen):**
```c
		case RTE_CRYPTO_ML_KEM_OP_KEYGEN:
		{
			uint8_t seed_d_z[64];
			struct rte_crypto_ml_kem_keygen_op *keygen = &mlkem->keygen;
			memcpy(seed_d_z,      keygen->d.data, 32);
			memcpy(&seed_d_z[32], keygen->z.data, 32);
```
`keygen->d.data` and `keygen->z.data` are not checked for NULL before being passed to `memcpy`. If the application supplies NULL pointers (which would be a misuse of the API), this will crash. The code should validate these pointers.

**Patch 3 line 2203-2209 (aesni_mb_asym_process ML_KEM encap):**
```c
			struct rte_crypto_ml_kem_encap_op *encap = &mlkem->encap;
			sess->asym.ml_kem.encap_params.m_32 = encap->message.data;
			sess->asym.ml_kem.encap_params.m_len = encap->message.length;
			encap->ek.length = aesni_mb_ml_kem_ek_sz(sess->asym.ml_kem.alg);
			encap->cipher.length = aesni_mb_ml_kem_ct_sz(sess->asym.ml_kem.alg);
			rc = imb_ml_kem_set_pubkey(sess->asym.ml_kem.ctx,
				encap->ek.data, encap->ek.length);
```
`encap->message.data` and `encap->ek.data` are not checked for NULL. If NULL, the library will receive a NULL pointer.

**Patch 3 line 2216-2222 (aesni_mb_asym_process ML_KEM decap):**
```c
			struct rte_crypto_ml_kem_decap_op *decap = &mlkem->decap;
			decap->dk.length = aesni_mb_ml_kem_dk_sz(sess->asym.ml_kem.alg);
			decap->cipher.length = aesni_mb_ml_kem_ct_sz(sess->asym.ml_kem.alg);
			rc = imb_ml_kem_set_privkey(sess->asym.ml_kem.ctx,
				decap->dk.data, decap->dk.length);warning: review has no Review-Result line, severity inferred from prose (response may be truncated)