|WARNING| [v4,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/171285

_AI Code Review_

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

# DPDK Patch Review

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

### Errors

**Error: Resource leak on GCM operation failure**

In `pmd_aesni_gcm.c`, the function `aesni_gcm_session_configure()` allocates no resources but the early returns for unsupported modes after calling `aesni_gcm_set_ops()` do not leak. However, in `process_gcm_crypto_op()`, multiple error paths return -EINVAL without releasing resources. Specifically, after `rte_pktmbuf_append()` or when validating digest length:

```c
/* line 461-464 in process_gcm_crypto_op */
if (unlikely(sym_op->aead.digest.data == NULL)) {
    sym_op->aead.digest.data = rte_pktmbuf_append(m_dst,
            session->gen_digest_length);
    if (sym_op->aead.digest.data == NULL) {
        IPSEC_MB_LOG(ERR, "Cannot allocate digest data");
        return -EINVAL;  /* ERROR: mbuf append failed but no cleanup */
    }
}
```

The function does not track allocated mbufs or temporary buffers for cleanup. If `rte_pktmbuf_append()` fails partway through processing a chain, the caller has no way to know which mbufs were modified. This is not strictly a leak in this patch (pre-existing), but the new IMB 3.0 wrapper functions preserve the same pattern without addressing it.

**Error: Wrapper functions missing state parameter validation**

Lines 8-144: The wrapper functions for library versions < 3.0.0 take an `IMB_MGR *state` parameter but never check if it is NULL before dereferencing `state->gcm128_pre(...)`. If `ops->mb_mgr` were ever NULL (which the initialization in `aesni_gcm_set_ops()` prevents but is not enforced by the function signature), this would segfault.

```c
/* line 12-16 */
static void imb_aes128_gcm_pre(const void *key, \
        struct gcm_key_data *key_data, IMB_MGR *state) \
{ \
    state->gcm128_pre(key, key_data);  /* NULL deref if state is NULL */
}
```

### Warnings

**Warning: Pre-existing unchecked return value from `rte_pktmbuf_append()`**

Lines 461-464, 689-692: The code checks if `rte_pktmbuf_append()` returns NULL, which is correct. However, the error path returns -EINVAL without cleaning up any partial state from earlier in the function. This is pre-existing (visible in context), but the patch propagates the pattern into the new IMB 3.0 code paths without improvement.

**Warning: Release notes formatting inconsistency**

In `doc/guides/rel_notes/release_26_11.rst` line 37-38:
```rst
* **Updated AESNI_MB crypto driver.**

  * Added support for version 3.0 of IPsec MB Library.
```

The sub-bullet under "Updated AESNI_MB crypto driver" is indented with two spaces, which is inconsistent. It should be indented with three spaces to align with RST list conventions for nested items.

---

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

### Errors

**Error: Missing IV validation in NxA cipher/auth setup**

Lines 650-664 in `pmd_aesni_mb.c`:
```c
if (xform->cipher.iv.length != 16) {
    IPSEC_MB_LOG(ERR, "Invalid cipher IV length");
    return -EINVAL;
}
```

This validation is correct for cipher modes. However, for the auth-only NEA4/NEA5/NEA6 modes (lines 243-260), there is no IV validation in `aesni_mb_set_session_auth_parameters()`. The code sets up `job->u.NIA._iv` in `set_mb_job_params()` (line 1807) but never checks that `session->auth_iv.length` matches the required 16 bytes. A mismatch would cause `rte_crypto_op_ctod_offset()` to return a pointer to uninitialized or truncated data.

**Error: Inconsistent digest size validation**

Lines 870, 894, 918: For NCA4/NCA5/NCA6 AEAD modes, digest size is validated to be 4-16 bytes. However, the corresponding auth-only modes (NIA4/NIA5/NIA6) allow digest sizes 4-16 bytes (lines 560-562, 583-585, 606-608) without checking against the AEAD mode's constraint. If a user configures NIA5 with a 3-byte digest, the code will accept it in capabilities but fail at runtime. The capabilities table (lines 1148-1150, 1172-1174, 1196-1198) shows `digest_size.min = 4`, which is correct, but the auth parameter setup does not enforce this.

### Warnings

**Warning: Release notes formatting inconsistency**

Lines 39-41 in `doc/guides/rel_notes/release_26_11.rst`:
```rst
  * Added support for the following wireless algorithms:
       - NEA4, NIA4, NCA4: Snow 5G confidentiality, integrity and AEAD modes.
```

The sub-list uses 7 spaces of indentation (3 for the parent bullet, 4 for alignment). RST convention is 3 spaces per level. Use:
```rst
  * Added support for the following wireless algorithms:

    - NEA4, NIA4, NCA4: Snow 5G confidentiality, integrity and AEAD modes.
    - NEA5, NIA5, NCA5: AES 256 confidentiality, integrity and AEAD modes.
    - NEA6, NIA6, NCA6: ZUC 256 confidentiality, integrity and AEAD modes.
```

---

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

### Errors

**Error: Session context not freed on error path**

Lines 1035-1038 in `pmd_aesni_mb.c`:
```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;  /* ERROR: does not call imb_ml_kem_free if ctx was partially allocated */
}
```

If `imb_ml_kem_new()` fails after allocating internal state, the `ctx` pointer may be non-NULL but invalid. The code returns immediately without calling `imb_ml_kem_free()`. The same issue exists for ML-DSA at lines 1055-1058.

**Error: Double-check cryptodev op status**

Lines 2596-2600:
```c
if (op->status == RTE_CRYPTO_OP_STATUS_NOT_PROCESSED)
    op->status = (rc == 0) ? RTE_CRYPTO_OP_STATUS_SUCCESS :
        RTE_CRYPTO_OP_STATUS_ERROR;
```

If `op->status` was set to `RTE_CRYPTO_OP_STATUS_INVALID_ARGS` in the switch statement (e.g., line 2550), this code will not overwrite it, which is correct. However, if `rc != 0` but the switch case forgot to set `op->status`, it will remain `NOT_PROCESSED` and be set to `ERROR` here. The issue is that some paths set `op->status = RTE_CRYPTO_OP_STATUS_INVALID_ARGS` (lines 2550, 2577) while others rely on the final check. This is inconsistent: either all error paths should set status, or none should.

**Error: Missing validation of keygen/encap/decap parameters**

Lines 2607-2614 (ML-KEM keygen):
```c
memcpy(seed_d_z,      keygen->d.data, 32);
memcpy(&seed_d_z[32], keygen->z.data, 32);
```

The code assumes `keygen->d.data` and `keygen->z.data` are non-NULL and have length >= 32. If an application submits a keygen op with NULL seed pointers or `length < 32`, this will segfault or leak sensitive data. There is no validation before the `memcpy()`.

### Warnings

**Warning: Wrapper macro definitions missing const correctness**

Lines 2510-2528: Helper macros `aesni_mb_ml_kem_ek_sz()` and similar return size constants but are not declared with `const` return type. While C does not enforce this, the function signature could indicate immutability:
```c
static inline size_t aesni_mb_ml_kem_ek_sz(const IMB_ML_KEM_ALG alg)
```
This is fine as-is, but the `alg` parameter could be `const` as well (though enums are passed by value, so it's cosmetic).

**Warning: Release notes inconsistency**

Lines 43-45 in `doc/guides/rel_notes/release_26_11.rst`:
```rst
  * Added support for the following PQC asymmetric algorithms:
       - ML-KEM: Module-Lattice-Based Key-Encapsulation Mechanism.
       - ML-DSA: Module-Lattice-Based Digital Signature Algorithm.
```

Same indentation issue as in Patch 2. Use 3-space nesting, not 7.

---

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

### Clean

No issues found. The patch correctly registers the test suite using `REGISTER_DRIVER_TEST()` macro and follows the existing pattern for other PMDs.

---

## Summary Across All Patches

**Correctness:**
- Resource leak: partial mbuf chain allocation without cleanup (pre-existing, propagated by new code paths)
- NULL pointer dereference risk in wrapper functions if `mb_mgr` is NULL (defensive coding issue)
- Session context leak on `imb_ml_kem_new()`/`imb_ml_dsa_new()` failure
- Missing validation of keygen seed pointers and lengths in ML-KEM/ML-DSA ops
- Missing IV length validation for NIA4/NIA5/NIA6 auth modes
- Inconsistent digest size validation between AEAD and auth-only modes

**Style and documentation:**
- RST list indentation inconsistencies across all release notes updates

Review-Result: ERROR