|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

## Overview

This patch series adds support for IPsec Multi-Buffer library version 3.0 to the AESNI_MB and AESNI_GCM crypto PMDs. It includes:
1. API migration from v2.0 to v3.0 (length units, function pointers)
2. New wireless algorithms (NEA4/5/6, NIA4/5/6, NCA4/5/6)
3. Post-quantum crypto support (ML-KEM, ML-DSA)
4. Test infrastructure updates

---

## PATCH 1/4: crypto/ipsec_mb: add support for IPsecMB 3.0

### Errors

**1. `aesni_gcm_set_ops` assigns `mb_mgr` pointer without NULL check**

```c
/* pmd_aesni_gcm.c:159 */
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;
```

The function accepts a `mb_mgr` parameter but does not verify it is non-NULL before dereferencing in the wrapper functions. If `aesni_gcm_set_ops(ops, NULL)` is called, all subsequent operations will segfault.

**Fix:** Add NULL check:
```c
static void
aesni_gcm_set_ops(struct aesni_gcm_ops *ops, IMB_MGR *mb_mgr)
{
	if (mb_mgr == NULL)
		return;
	...
}
```

**2. Potential NULL dereference in GCM pre-expansion functions**

```c
/* pmd_aesni_gcm.c:287 */
case 16:
	sess->key_length = GCM_KEY_128;
	imb_aes128_gcm_pre(key, &sess->gdata_key, mb_mgr);
	break;
```

If `mb_mgr` is NULL when passed to session configuration, `imb_aes128_gcm_pre()` will be called with NULL. The wrapper functions forward this to `state->gcm128_pre()` which will dereference NULL.

**Fix:** Check `mb_mgr` at function entry:
```c
static int
aesni_gcm_session_configure(IMB_MGR *mb_mgr, void *session, ...)
{
	if (mb_mgr == NULL)
		return -EINVAL;
	...
}
```

---

### Warnings

**3. Function pointer wrapper conditionally compiled but used unconditionally**

```c
/* pmd_aesni_gcm.c:6 - wrapper block */
#if IMB_VERSION(3, 0, 0) > IMB_VERSION_NUM
/* wrappers defined here */
#endif

/* pmd_aesni_gcm.c:159 - usage site */
ops[GCM_KEY_128].pre = imb_aes128_gcm_pre;
```

When compiled against IPsec MB >=3.0, the wrapper functions are not defined, so `imb_aes128_gcm_pre` is an undeclared identifier. This will cause a compilation failure.

**Fix:** The wrappers should be defined for >=3.0, calling the IMB API directly:
```c
#if IMB_VERSION(3, 0, 0) <= IMB_VERSION_NUM
static void imb_aes128_gcm_pre(const void *key,
		struct gcm_key_data *key_data, IMB_MGR *state) {
	imb_aes_gcm_pre_128(state, key, key_data);
}
/* repeat for other sizes */
#endif
```

**4. Capabilities updated but feature flags unchanged**

```c
/* pmd_aesni_mb.c:2585 */
aesni_mb_data->feature_flags =
	...
	RTE_CRYPTODEV_FF_SYM_SESSIONLESS |
	RTE_CRYPTODEV_FF_IN_PLACE_SGL |
	...
```

The patch adds ZUC/Snow3G algorithm variants and removes ZUC-256 support, but does not update the feature flags or remove stale capability entries. For example, ZUC-256 capabilities remain in `pmd_aesni_mb_priv.h` under `#if IMB_VERSION(3, 0, 0) > IMB_VERSION_NUM` guards, but the code paths are now unreachable.

**Fix:** Clean up dead code for ZUC-256 when building against >=3.0.

**5. Conditional compilation blocks clutter control flow**

```c
/* pmd_aesni_mb.c:1195 */
#if IMB_VERSION(3, 0, 0) > IMB_VERSION_NUM
	if (job->cipher_mode == IMB_CIPHER_SNOW3G_UEA2 ||
			job->cipher_mode == IMB_CIPHER_KASUMI_UEA1)
		cipher_len = (job->msg_len_to_cipher_in_bits >> 3) + ...
	else
#endif
		cipher_len = job->msg_len_to_cipher_in_bytes + ...
```

This pattern repeats throughout. While correct, it makes the code harder to review. Consider extracting length conversions into inline helper functions gated by version macros at the top of the file.

---

### Info

**6. Macro redefinitions in compatibility layer**

```c
/* pmd_aesni_mb_priv.h:22 */
#if IMB_VERSION(3, 0, 0) > IMB_VERSION_NUM
#define IMB_CIPHER_SNOW3G_UEA2 IMB_CIPHER_SNOW3G_UEA2_BITLEN
#define IMB_CIPHER_KASUMI_UEA1 IMB_CIPHER_KASUMI_UEA1_BITLEN
...
#endif
```

This is a clean approach to API migration. The aliases allow the rest of the code to use the v3.0 names unconditionally.

---

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

### Errors

**7. Missing capability validation for new algorithms**

```c
/* pmd_aesni_mb.c:434 */
case RTE_CRYPTO_CIPHER_SNOW5G_NEA4:
	sess->template_job.cipher_mode = IMB_CIPHER_SNOW5G_NEA4;
	is_nxan = 1;
	break;
```

The new NEA4/5/6 and NIA4/5/6 algorithms are added without verifying hardware support. The code assumes if IPsec MB >=3.0 is present, the algorithms are available, but this may not be true on all CPUs.

**Fix:** Check CPU features at device probe time and conditionally enable algorithms:
```c
if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) &&
    imb_get_feature(mb_mgr, IMB_FEATURE_SNOW5G)) {
	/* enable SNOW5G NEA4/NIA4/NCA4 */
}
```

**8. Incorrect AAD handling for NCA algorithms**

```c
/* pmd_aesni_mb.c:814 */
case RTE_CRYPTO_AEAD_SNOW5G_NCA4:
	sess->template_job.cipher_mode = IMB_CIPHER_SNOW5G_NCA4;
	sess->template_job.hash_alg = IMB_AUTH_SNOW5G_NCA4;
	sess->template_job.u.GCM.aad_len_in_bytes = xform->aead.aad_length;
```

The code uses `u.GCM.aad_len_in_bytes` for Snow5G NCA4, but then assigns to `u.NCA.aad` in the job setup path:
```c
/* pmd_aesni_mb.c:1785 */
case IMB_AUTH_SNOW5G_NCA4:
case IMB_AUTH_AES_NCA5:
case IMB_AUTH_ZUC_NCA6:
	job->u.NCA.aad = op->sym->aead.aad.data;
	break;
```

This is a union type confusion. Snow5G/AES/ZUC NCA modes use `u.NCA.*`, not `u.GCM.*`.

**Fix:**
```c
case RTE_CRYPTO_AEAD_SNOW5G_NCA4:
	...
	sess->template_job.u.NCA.aad_len_in_bytes = xform->aead.aad_length;
	break;
```

---

### Warnings

**9. Key expansion for AES NEA5/NCA5 silently fails if key is wrong size**

```c
/* pmd_aesni_mb.c:835 */
case RTE_CRYPTO_AEAD_AES_NCA5:
	...
	if (xform->aead.key.length != 32) {
		IPSEC_MB_LOG(ERR, "Invalid key length");
		return -EINVAL;
	}
	sess->template_job.key_len_in_bytes = 32;
	IMB_AES_KEYEXP_256(mb_mgr, xform->aead.key.data,
		sess->cipher.expanded_aes_keys.encode,
		sess->cipher.expanded_aes_keys.decode);
```

`IMB_AES_KEYEXP_256` does not return an error code. If it fails internally (e.g., out of memory), the expanded keys are left in an undefined state, but the session is marked as configured.

**Fix:** IPsec MB does not provide error returns for key expansion. Document this as a known limitation or add a post-expansion validation step if IMB provides one in v3.0.

**10. Digest size validation inconsistent between algorithms**

```c
/* pmd_aesni_mb.c:831 */
if (sess->auth.req_digest_len < 4 || sess->auth.req_digest_len > 16) {
	IPSEC_MB_LOG(ERR, "Invalid digest size");
	return -EINVAL;
}
```

This check is repeated for all three NCA variants. However, the corresponding NIA (auth-only) algorithms have the same validation in the capabilities table:
```c
/* pmd_aesni_mb_priv.h:977 */
.digest_size = {
	.min = 4,
	.max = 16,
	.increment = 1
},
```

If the app requests a 3-byte digest, the capability check passes but session config fails. This is correct but inconsistent with the "fail early" principle.

**Fix:** This is not a bug, but the error message should clarify that only 4-16 byte digests are supported for these algorithms.

---

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

### Errors

**11. ML-KEM/ML-DSA session resources not freed on configuration failure**

```c
/* pmd_aesni_mb.c:1004 */
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()` succeeds but a later validation fails (e.g., invalid key type), the function returns without calling `imb_ml_kem_free()`. This leaks the ML-KEM context.

**Fix:**
```c
ret = imb_ml_kem_new(...);
if (ret) {
	IPSEC_MB_LOG(ERR, "Failed to create ML KEM session");
	return ret;
}
/* later error path: */
default:
	imb_ml_kem_free(sess->asym.ml_kem.ctx);
	sess->asym.ml_kem.ctx = NULL;
	ret = -ENOTSUP;
	IPSEC_MB_LOG(ERR, "Unsupported type");
	return ret;
```

**12. Double-free risk in `aesni_mb_asym_session_clear`**

```c
/* pmd_aesni_mb.c:1050 */
static void
aesni_mb_asym_session_clear(void *priv_sess)
{
	struct aesni_mb_session *sess = (struct aesni_mb_session *) priv_sess;
	if (sess->asym.xform_type == RTE_CRYPTO_ASYM_XFORM_ML_KEM) {
		imb_ml_kem_free(sess->asym.ml_kem.ctx);
		sess->asym.ml_kem.ctx = NULL;
	} else if (sess->asym.xform_type == RTE_CRYPTO_ASYM_XFORM_ML_DSA) {
		imb_ml_dsa_free(sess->asym.ml_dsa.ctx);
		sess->asym.ml_dsa.ctx = NULL;
	}
}
```

If `aesni_mb_asym_session_clear()` is called twice on the same session (e.g., error recovery path + normal teardown), the second call will pass NULL to `imb_ml_kem_free()`. Check if IMB tolerates this; if not, guard the free calls:
```c
if (sess->asym.ml_kem.ctx != NULL) {
	imb_ml_kem_free(sess->asym.ml_kem.ctx);
	sess->asym.ml_kem.ctx = NULL;
}
```

**13. Missing NULL check on session in `aesni_mb_asym_process`**

```c
/* pmd_aesni_mb.c:2552 */
static void
aesni_mb_asym_process(struct rte_crypto_op *op)
{
	struct rte_crypto_asym_op *aop = op->asym;
	struct aesni_mb_session *sess =
		CRYPTODEV_GET_ASYM_SESS_PRIV(aop->session);
```

If `aop->session` is NULL (sessionless operation path), `CRYPTODEV_GET_ASYM_SESS_PRIV` will dereference NULL.

**Fix:**
```c
if (aop->session == NULL) {
	op->status = RTE_CRYPTO_OP_STATUS_INVALID_SESSION;
	return;
}
struct aesni_mb_session *sess = CRYPTODEV_GET_ASYM_SESS_PRIV(aop->session);
```

**14. `aesni_mb_dequeue_burst` array `deqd_ops` re-use assumes no asym ops**

```c
/* pmd_aesni_mb.c:2791 */
struct rte_crypto_op *tmp_dq[IMB_MAX_BURST_SIZE];
uint16_t sym_cnt = 0;
nb_submit_ops = rte_ring_dequeue_burst(qp->ingress_queue, (void **)tmp_dq, n, NULL);
for (i = 0; i < nb_submit_ops; i++) {
	if (tmp_dq[i]->type == RTE_CRYPTO_OP_TYPE_ASYMMETRIC) {
		aesni_mb_asym_process(tmp_dq[i]);
		ops[processed_jobs++] = tmp_dq[i];
	} else {
		deqd_ops[sym_cnt++] = tmp_dq[i];
	}
}warning: review has no Review-Result line, severity inferred from prose (response may be truncated)