|WARNING| [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/170504

_AI Code Review_

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

# DPDK Patch Review

This review covers a 4-patch series adding IPsecMB 3.0 support to the AESNI_MB and AESNI_GCM crypto PMDs, including new wireless algorithms (NxA4/5/6) and PQC asymmetric crypto (ML-KEM, ML-DSA).

---

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

### Errors

**1. Missing error checks on IMB_AES_KEYEXP_256 (pmd_aesni_mb.c:855-857)**

The key expansion function can fail; its return value must be checked.

```c
/* BAD - unchecked call */
IMB_AES_KEYEXP_256(mb_mgr, xform->aead.key.data,
    sess->cipher.expanded_aes_keys.encode,
    sess->cipher.expanded_aes_keys.decode);

/* GOOD - check return value */
if (IMB_AES_KEYEXP_256(mb_mgr, xform->aead.key.data,
        sess->cipher.expanded_aes_keys.encode,
        sess->cipher.expanded_aes_keys.decode) != 0) {
    IPSEC_MB_LOG(ERR, "Key expansion failed");
    return -EINVAL;
}
```

Apply this fix at all 6 call sites: lines 334-336, 354-356, 815-817, 825-827, 835-837, 855-857.

**2. Missing error check on imb_aes*_gcm_pre wrappers (pmd_aesni_gcm.c:287-297)**

The pre-keying functions (via the ops table in `aesni_gcm_session_configure`) are called without checking if the wrapper succeeded. While the underlying library function may not return errors in practice, the wrapper signature and defensive programming require checking.

```c
/* BAD - no check */
imb_aes128_gcm_pre(key, &sess->gdata_key, mb_mgr);

/* GOOD - verify success */
imb_aes128_gcm_pre(key, &sess->gdata_key, mb_mgr);
/* If pre-keying fails, session is unusable */
if (sess->gdata_key.expanded_keys == NULL) {
    IPSEC_MB_LOG(ERR, "GCM pre-keying failed");
    return -EINVAL;
}
```

Apply at lines 287, 291, 295.

**3. Library version check macro usage inconsistency (multiple files)**

The `IMB_VERSION()` macro comparison direction is flipped in one location (pmd_aesni_gcm.c:8) compared to all others. This is error-prone and should be normalized.

```c
/* Inconsistent - tests if 3.0.0 is GREATER than current */
#if IMB_VERSION(3, 0, 0) > IMB_VERSION_NUM

/* Consistent pattern used elsewhere - tests if current is LESS than 3.0.0 */
#if IMB_VERSION(3, 0, 0) <= IMB_VERSION_NUM
```

Change line 8 in pmd_aesni_gcm.c to use `<=` with the body inverted to match the codebase convention.

---

### Warnings

**1. Wrapper function proliferation (pmd_aesni_gcm.c:8-145)**

The patch adds 140 lines of wrapper functions to bridge the function-pointer vs direct-call API change. This is correct but verbose. Consider whether a macro-based approach could reduce duplication:

```c
#define GCM_WRAPPER(bitlen, op) \
static void imb_aes##bitlen##_gcm_##op(...) { \
    state->gcm##bitlen##_##op(__VA_ARGS__); \
}
```

This is a suggestion for future cleanup, not a blocker.

**2. Missing release notes for removed ZUC-256 support (doc/guides/cryptodevs/aesni_mb.rst:80)**

The ZUC-256 removal is mentioned in the "Note" section (lines 80-81) but not in the release notes deprecation section. Applications using ZUC-256 will break silently. Add a "Removed Items" entry to release_26_11.rst.

---

### Info

- The `_fn` macro pattern (lines 13-145 in pmd_aesni_gcm.c) is clean and avoids function duplication effectively.
- The bit-length enum removal is well-handled with conditional compilation throughout.

---

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

### Errors

**1. Missing error checks on IMB_AES_KEYEXP_256 (pmd_aesni_mb.c:644-646, 660-662)**

Same issue as Patch 1. The key expansion function can fail; check its return value at both call sites.

```c
/* GOOD */
if (IMB_AES_KEYEXP_256(mb_mgr, xform->cipher.key.data,
        sess->cipher.expanded_aes_keys.encode,
        sess->cipher.expanded_aes_keys.decode) != 0) {
    IPSEC_MB_LOG(ERR, "Key expansion failed");
    return -EINVAL;
}
```

---

### Warnings

**1. Release notes formatting (doc/guides/rel_notes/release_26_11.rst:40-42)**

The bullet list uses inconsistent indentation. Should be 3 spaces after the `*`, not varying amounts:

```rst
/* CURRENT - inconsistent spacing */
  * Added support for the following wireless algorithms:
       - NEA4, NIA4, NCA4: Snow 5G confidentiality, integrity and AEAD modes.

/* GOOD */
  * Added support for the following wireless algorithms:
     - NEA4, NIA4, NCA4: Snow 5G confidentiality, integrity and AEAD modes.
```

**2. Blank line before "This section should contain" (doc/guides/rel_notes/release_26_11.rst:48)**

The added blank line before the comment is spurious and should be removed.

---

### Info

- The algorithm additions are well-structured with consistent capability descriptors.
- The digest size ranges (4-16 bytes) match the library's supported values.

---

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

### Errors

**1. Unchecked return values from IMB_AES_KEYEXP_256 (pmd_aesni_mb.c:835-837, 851-853, 867-869)**

Same issue as previous patches. All three call sites (for SNOW5G_NCA4, AES_NCA5, ZUC_NCA6) must check the return value.

**2. Missing error path cleanup for ML-KEM/ML-DSA session init (pmd_aesni_mb.c:999, 1027)**

The `imb_ml_kem_new()` and `imb_ml_dsa_new()` calls allocate resources via the library. If the subsequent parameter init macros fail (unlikely but possible), the context leaks.

```c
/* BAD - ctx leaks if macro "fails" (theoretical) */
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;
}
IMB_ML_KEM_KEYGEN_PARAMS_INIT(&sess->asym.ml_kem.keygen_params);

/* GOOD - clear ctx on any subsequent failure */
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_KEYGEN_PARAMS_INIT(&sess->asym.ml_kem.keygen_params) != 0) {
    imb_ml_kem_free(sess->asym.ml_kem.ctx);
    return -EINVAL;
}
```

Note: If the `_INIT` macros are void (current library), this is theoretical. But defensive coding requires cleanup.

**3. Unvalidated asym op type in aesni_mb_asym_process (pmd_aesni_mb.c:2544-2546, 2615-2617)**

The `op` field from the user-supplied crypto operation is used directly in switch statements without bounds checking. A corrupted or malicious op could access invalid enum values.

```c
/* BAD - unchecked op */
switch (mlkem->op) {

/* GOOD - validate first */
if (mlkem->op >= RTE_CRYPTO_ML_KEM_OP_END) {
    op->status = RTE_CRYPTO_OP_STATUS_INVALID_ARGS;
    return;
}
switch (mlkem->op) {
```

Apply at lines 2544 (mlkem->op) and 2615 (mldsa->op).

**4. Missing session validity check in aesni_mb_asym_process (pmd_aesni_mb.c:2558)**

The session is dereferenced without checking if it is NULL. A sessionless or malformed operation could cause a NULL pointer dereference.

```c
/* BAD - no NULL check */
struct aesni_mb_session *sess =
    CRYPTODEV_GET_ASYM_SESS_PRIV(aop->session);

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

---

### Warnings

**1. Missing input length validation in ML-KEM keygen (pmd_aesni_mb.c:2572-2574)**

The code copies 32 bytes from `keygen->d.data` and `keygen->z.data` without verifying `length` fields are >= 32. If the application provides shorter buffers, this is an out-of-bounds read.

```c
/* BAD - assumes 32-byte buffers */
memcpy(seed_d_z,      keygen->d.data, 32);
memcpy(&seed_d_z[32], keygen->z.data, 32);

/* GOOD */
if (keygen->d.length < 32 || keygen->z.length < 32) {
    op->status = RTE_CRYPTO_OP_STATUS_INVALID_ARGS;
    return;
}
memcpy(seed_d_z,      keygen->d.data, 32);
memcpy(&seed_d_z[32], keygen->z.data, 32);
```

Apply similar checks to ML-DSA seed lengths at line 2625 and sign/verify message lengths.

**2. asym_session_created flag not thread-safe (ipsec_mb_private.h:377)**

The `asym_session_created` flag is accessed from the dequeue path without synchronization. If sessions are created/destroyed concurrently with packet processing, this is a data race. Use `rte_atomic_load_explicit(..., rte_memory_order_relaxed)` or document that session lifecycle must not overlap with queue operations.

**3. Release notes formatting (doc/guides/rel_notes/release_26_11.rst:43-45)**

Same indentation issue as Patch 2. Inconsistent spacing after bullet points.

---

### Info

- The switch from dequeue-only-sym to dequeue-mixed-ops (lines 2783-2809) is clever and preserves backward compat.
- The size helper functions (lines 2488-2543) are clean and avoid duplication.

---

## PATCH 4/4: test/crypto: add asym test suite for AESNI_MB

### Info

- Trivial addition; no issues.

---

## Summary

| Severity | Count | Category |
|----------|-------|----------|
| Error    | 10    | Missing error checks (6 IMB_AES_KEYEXP_256, 2 session init cleanup, 2 input validation) |
| Warning  | 6     | Input validation (ML-KEM/DSA lengths), thread safety, release notes formatting |
| Info     | 4     | Code quality observations |

**Primary concerns:**
1. **All IMB_AES_KEYEXP_256 calls** (9 total across patches 1-3) must check return values.
2. **Asym op input validation** (patch 3): check op type bounds, session NULL, buffer lengths.
3. **Session init error paths** (patch 3): must free ML-KEM/ML-DSA contexts on failure.

**Secondary:**
- Add "Removed Items" section to release notes for ZUC-256.
- Fix release notes RST indentation in patches 2 and 3.
- Consider input length validation for crypto operations in patch 3.