|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

**Version detection logic in wrapper functions**

The wrapper functions for library versions < 3.0 are only used when `IMB_VERSION(3, 0, 0) > IMB_VERSION_NUM`. However, in `aesni_gcm_set_ops()` these wrappers are called unconditionally on all versions:

```c
/* From pmd_aesni_gcm.c, line 159 onwards */
ops[GCM_KEY_128].pre = imb_aes128_gcm_pre;
ops[GCM_KEY_128].init = imb_aes128_gcm_init;
/* ... etc */
```

For version 3.0+, these wrapper function names don't exist -- the code should be calling the direct IMB API functions. This will fail to compile with IPsec MB 3.0.

**Suggested fix:** The wrapper section and the function pointer assignments in `aesni_gcm_set_ops()` need matching version guards:

```c
#if IMB_VERSION(3, 0, 0) > IMB_VERSION_NUM
    /* wrapper definitions here */
    ops[GCM_KEY_128].pre = imb_aes128_gcm_pre;
#else
    ops[GCM_KEY_128].pre = IMB_AES128_GCM_PRE;  /* or direct API call */
#endif
```

---

**Missing IMB_MGR parameter in pre-3.0 gcm_pre calls**

In `aesni_gcm_session_configure()`, the calls to `imb_aes128/192/256_gcm_pre()` pass an `mb_mgr` parameter:

```c
imb_aes128_gcm_pre(key, &sess->gdata_key, mb_mgr);
```

But in versions < 3.0, the library functions `mb_mgr->gcm128_pre()` etc. take only two arguments `(key, key_data)`. The wrapper functions defined at the top of the file correctly match this signature, but the *call sites* at lines 285-293 pass three arguments, causing a compile error on pre-3.0 builds.

**Suggested fix:** The call sites need version guards:

```c
#if IMB_VERSION(3, 0, 0) > IMB_VERSION_NUM
    mb_mgr->gcm128_pre(key, &sess->gdata_key);
#else
    imb_aes128_gcm_pre(key, &sess->gdata_key, mb_mgr);
#endif
```

Or unify the wrapper to always take `mb_mgr` and discard it internally for < 3.0.

---

**Inconsistent `IMB_MGR` constness**

`aesni_mb_set_session_cipher_parameters()` changes the function signature from `const IMB_MGR *mb_mgr` to `IMB_MGR *mb_mgr` (line 348), but the function body does not write to `mb_mgr` on the code paths shown. The removal of `const` suggests version 3.0 requires a mutable pointer, but this is not explained and could indicate a missing initialization call.

If version 3.0 genuinely requires non-const, the commit message should state this. If the function doesn't write to `mb_mgr`, the constness removal is a red flag that something is missing.

---

### Warnings

**Release notes formatting**

In `doc/guides/rel_notes/release_26_11.rst`, the list under "Updated AESNI_MB crypto driver" should use a definition list (term + description) rather than a simple bullet:

```rst
* **Updated AESNI_MB crypto driver.**

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

is acceptable, but the sub-item would be clearer as:

```rst
* **Updated AESNI_MB crypto driver.**

  v3.0 support
     Added support for version 3.0 of IPsec MB Library.
```

This is a minor style point, not a blocker.

---

**Documentation does not mention migration path for removed features**

The release notes state that ZUC 256 support and `RTE_CRYPTODEV_FF_NON_BYTE_ALIGNED_DATA` are removed in version 3.0, but do not guide users on what to do if they depend on these. Adding a sentence like "Applications using ZUC-256 must migrate to an alternative PMD or remain on IPsec MB 2.x" would help.

---

### Info

**Pre-existing: Missing GMAC IV length validation**

At line 517 in `process_gcm_crypto_op()`, the GMAC operations use `session->iv.length` directly without checking it is within the valid range. This is pre-existing code (the patch does not touch the IV setup), but GMAC IV length should be between 1 and the GCM maximum. An out-of-range IV length from a malformed session could cause buffer overruns in the library call.

**Labelled pre-existing:** This issue is not introduced by this patch.

---

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

### Errors

None.

---

### Warnings

**Capability structure duplication**

The three new cipher/auth/aead capability blocks (NEA4/NIA4/NCA4, NEA5/NIA5/NCA5, NEA6/NIA6/NCA6) in `pmd_aesni_mb_priv.h` lines 894-1118 are nearly identical, differing only in the algorithm enum. This is 225 lines of copy-paste code. While not incorrect, it makes future updates error-prone (e.g., if the block size or IV constraints change).

**Suggested fix:** Consider a macro to generate these blocks, similar to the `_fn()` pattern used in patch 1 for the wrapper functions. Example:

```c
#define DEFINE_NXA_CAPABILITIES(suffix, cipher_algo, auth_algo, aead_algo) \
    { /* NEA##suffix */ ... }, \
    { /* NIA##suffix */ ... }, \
    { /* NCA##suffix */ ... }

DEFINE_NXA_CAPABILITIES(4, RTE_CRYPTO_CIPHER_SNOW5G_NEA4, ...)
DEFINE_NXA_CAPABILITIES(5, RTE_CRYPTO_CIPHER_AES_NEA5, ...)
DEFINE_NXA_CAPABILITIES(6, RTE_CRYPTO_CIPHER_ZUC_NEA6, ...)
```

This is a code quality suggestion, not a functional bug.

---

**Missing validation of AAD length in NCA4/5/6 AEAD setup**

In `aesni_mb_set_session_aead_parameters()` lines 829-885, the code sets `sess->template_job.u.NCA.aad_len_in_bytes = xform->aead.aad_length;` without checking it against the advertised capability range (0-65535). While the capability advertises this range, a runtime check would catch configuration errors early. The existing GCM and CCM paths have similar omissions (pre-existing), but new code should add the check.

**Suggested fix:**

```c
if (xform->aead.aad_length > 65535) {
    IPSEC_MB_LOG(ERR, "Invalid AAD length");
    return -EINVAL;
}
```

---

**Release notes list formatting**

The release notes list at line 39-42 of `release_26_11.rst` would be clearer as a definition list:

```rst
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.
```

instead of the current simple bullet format.

---

### Info

None.

---

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

### Errors

**Missing session structure size accounting for asym union**

The `aesni_mb_session` structure in `pmd_aesni_mb_priv.h` gains a large `asym` union at line 1403-1421 (26 bytes for the pointers plus parameter structs), but `pmd_aesni_mb.c` line 3205 sets:

```c
aesni_mb_data->session_priv_size = sizeof(struct aesni_mb_session);
```

This is correct in principle (the `sizeof` will include the union), but the union is only conditionally compiled (`#if IMB_VERSION(3, 0, 0) <= IMB_VERSION_NUM`). If the code is built against IPsec MB 2.x, the `asym` member does not exist, the session size is smaller, and the driver advertises the wrong session size to the framework. This can cause heap corruption if the framework allocates a session using the smaller size but 3.0-era code (or a later rebuild) expects the larger size.

**Suggested fix:** Either:
1. Make the `asym` union always present (with a dummy/reserved field for < 3.0), or
2. Adjust `session_priv_size` conditionally:

```c
#if IMB_VERSION(3, 0, 0) <= IMB_VERSION_NUM
aesni_mb_data->session_priv_size = sizeof(struct aesni_mb_session);
#else
aesni_mb_data->session_priv_size = sizeof(struct aesni_mb_session) - sizeof_asym_union;
#endif
```

Option 1 is cleaner.

---

**Missing error path cleanup in asym session configure**

In `aesni_mb_asym_session_configure()` at line 1022 and 1047, if `imb_ml_kem_new()` or `imb_ml_dsa_new()` succeeds but returns a context pointer, and a later error occurs (e.g., unsupported type check at line 1056), the function returns `-ENOTSUP` without calling `imb_ml_kem_free()` or `imb_ml_dsa_free()`. This leaks the IMB library context.

The checks at lines 1028 and 1053 (`if (xform->mlkem.type != ...)`) come *after* the `_new()` calls. If these checks fail, the context is leaked.

**Suggested fix:** Move the type validation before the `_new()` call, or add cleanup in the error paths:

```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 later validation fails: */
if (invalid) {
    imb_ml_kem_free(sess->asym.ml_kem.ctx);
    sess->asym.ml_kem.ctx = NULL;
    return -EINVAL;
}
```

---

**Asym ops processed in dequeue without session validity check**

In `aesni_mb_dequeue_burst()` at line 2818, the asym processing loop:

```c
if (tmp_dq[i]->type == RTE_CRYPTO_OP_TYPE_ASYMMETRIC) {
    aesni_mb_asym_process(tmp_dq[i]);
    ops[processed_jobs++] = tmp_dq[i];
}
```

calls `aesni_mb_asym_process()` without checking if `aop->session` is NULL or if the session was configured with `aesni_mb_asym_session_configure()`. If an application enqueues an asym op with a NULL or wrongly-typed session, `aesni_mb_asym_process()` will dereference `CRYPTODEV_GET_ASYM_SESS_PRIV(aop->session)` at line 2566, causing a segfault.

**Suggested fix:** Add a session validity check in the dequeue loop or at the start of `aesni_mb_asym_process()`:

```c
if (aop->session == NULL) {
    op->status = RTE_CRYPTO_OP_STATUS_INVALID_SESSION;
    ops[processed_jobs++] = op;
    continue;
}
```

---

### Warnings

**Asym session clear does not zero the session structure**

`aesni_mb_asym_session_clear()` at line 2067 frees the IMB contexts and sets the pointers to NULL, but does not zero the rest of the session private data. The `keygen_params`, `encap_params`, etc. fields may contain sensitive entropy (seeds, randomness) from previous operations. These should be zeroed with `rte_memzero_explicit()` before the session is freed to avoid leaving key material in the heap.

**Suggested fix:**

```c
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;
        rte_memzero_explicit(&sess->asym.ml_kem, sizeof(sess->asym.ml_kem));
    } 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;
        rte_memzero_explicit(&sess->asym.ml_dsa, sizeof(sess->asym.ml_dsa));
    }
}
```

---

**Mixing sym and asym ops in the same QP without segregation**

The dequeue logic at line 2818 processes sym and asym ops from the same ring. If a burst contains both, the asym ops are completed inline (blocking the dequeue) while sym ops are submitted to the IMB scheduler. If an asym operation is slow (e.g., ML-DSA keygen), this stalls the entire queue pair. The code does not document this limitation or provide a way to separate asym workloads to dedicated QPs.

**Suggested documentation:** Add a note in `aesni_mb.rst` that asym and sym ops share queue pairs and asym ops are processed synchronously, which may impact latency.

---

**Release notes formatting**

The list at lines 43-45 of `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.
```

should use a definition list:

```rst
ML-KEM
   Module-Lattice-Based Key-Encapsulation Mechanism.

ML-DSA
   Module-Lattice-Based Digital Signature Algorithm.
```

---

### Info

None.

---

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

### Errors

None.

---

### Warnings

None.

---

### Info

None.

---

# Summary

**Patch 1/4** has **three Errors** (version guard mismatch in wrapper usage, parameter count mismatch in `gcm_pre` calls, unclear const removal) and **two Warnings** (release notes formatting, missing migration guidance).

**Patch 2/4** has **zero Errors** and **three Warnings** (capability duplication, missing AAD validation, release notes formatting).

**Patch 3/4** has **three Errors** (session size accounting for conditional union, missing cleanup in asym session configure error paths, missing session validity check in asym dequeue) and **three Warnings** (warning: review has no Review-Result line, severity inferred from prose (response may be truncated)