|WARNING| [v5,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/171447

_AI Code Review_

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

# DPDK Patch Review

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

### Errors

**Missing error checks on IMB API calls (Error)**

In `pmd_aesni_gcm.c`, lines 283-293 and throughout the file, the `imb_aes*_gcm_pre()` functions are called without checking their return values. Intel IPsec MB 3.0.0+ changed these functions to return `int` (success 0, failure non-zero), but the code assumes they always succeed.

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

/* GOOD - check return value */
int ret = imb_aes128_gcm_pre(key, &sess->gdata_key, mb_mgr);
if (ret != 0) {
    IPSEC_MB_LOG(ERR, "Key expansion failed");
    return -EINVAL;
}
```

This applies to all three key sizes (128, 192, 256) in `aesni_gcm_session_configure()`.

**Version macro used incorrectly for wrapper functions (Error)**

Lines 7-144 in `pmd_aesni_gcm.c` define wrapper functions only for `IMB_VERSION(3, 0, 0) > IMB_VERSION_NUM`. However, the wrappers are then called unconditionally in `aesni_gcm_set_ops()` (lines 157-203). When building against IPsec MB >= 3.0, the wrappers are not compiled but the code tries to use them, causing undefined symbol errors.

The `#if` condition at line 7 should be `<=` not `>`:
```c
/* GOOD */
#if IMB_VERSION(3, 0, 0) <= IMB_VERSION_NUM
```
Or the wrapper calls in `aesni_gcm_set_ops()` need version guards.

**Bitmask shift using `1` on 64-bit type (Error)**

In `pmd_aesni_mb.c` at line 2609, the code uses `1 <<` in a 64-bit feature flags context:
```c
RTE_CRYPTODEV_FF_SYM_OPERATION_CHAINING |
```
While the line shown is not itself the problem, the surrounding context suggests feature flags are 64-bit. If any of the `RTE_CRYPTODEV_FF_*` constants being OR'd together uses `1 << n` with `n >= 32`, it will be undefined behavior. The patch does not show whether new flags are introduced that would trigger this, but it is a correctness risk in the PMD feature flags pattern.

Use `UINT64_C(1) << n` or `RTE_BIT64(n)` for all 64-bit feature flag definitions.

### Warnings

**Release notes formatting (Warning)**

In `doc/guides/rel_notes/release_26_11.rst` at line 30, the bullet formatting is inconsistent:
```rst
* **Updated AESNI_MB crypto driver.**

  * Added support for version 3.0 of IPsec MB Library.
```
The nested bullet should not have a second-level indent in the `.rst` source. It will render correctly, but DPDK release notes convention is to use a definition list for feature sub-items rather than nested bullets. Consider:

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

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

### Info

**Pre-existing: Use of RTE_DEFINE_PER_LCORE for ops array (Info)**

At line 130 in `pmd_aesni_gcm_priv.h`:
```c
RTE_DEFINE_PER_LCORE(struct aesni_gcm_ops, gcm_ops[GCM_NUM_KEY_TYPES]);
```
This defines a per-lcore (thread-local) array of operations structs. Each thread gets its own copy. The `mb_mgr` pointer added to `struct aesni_gcm_ops` at line 131 is set in `aesni_gcm_set_ops()` but is the same `IMB_MGR *` for all three key types on a given lcore. Storing it three times (once per key type) is redundant.

Not introduced by this patch (the pattern pre-exists), but worth noting: the `mb_mgr` pointer could be stored once per-lcore instead of three times.

---

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

### Errors

**Missing error checks on IMB_AES_KEYEXP_256 (Error)**

In `pmd_aesni_mb.c` at lines 245 and 662, `IMB_AES_KEYEXP_256()` is called without checking its return value. This function can fail (e.g., null key pointer, allocation failure), and proceeding with uninitialized keys will cause incorrect encryption or crashes.

```c
/* BAD - no error check */
IMB_AES_KEYEXP_256(mb_mgr, xform->auth.key.data,
    sess->cipher.expanded_nia5_keys.encode,
    sess->cipher.expanded_nia5_keys.decode);

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

This applies to both call sites (auth and cipher setup).

### Warnings

**Release notes indentation (Warning)**

In `doc/guides/rel_notes/release_26_11.rst` at lines 29-33, the sub-items under "Updated AESNI_MB crypto driver" use inconsistent formatting:
```rst
  * Added support for version 3.0 of IPsec MB Library.
  * Added support for the following wireless algorithms:
       - NEA4, NIA4, NCA4: Snow 5G confidentiality, integrity and AEAD modes.
```
The mix of second-level bullets (`*`) and dashes (`-`) is not consistent with DPDK release notes style. Use a definition list for the algorithm names or flatten to a single list.

---

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

### Errors

**Resource leak on asym session configure failure (Error)**

In `pmd_aesni_mb.c`, `aesni_mb_asym_session_configure()` (lines 988-1073), when `imb_ml_kem_new()` or `imb_ml_dsa_new()` succeeds but a later validation fails, the session context is leaked:

```c
/* Line 1017 - ML-KEM session created */
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;
}
/* Lines 1018-1020 - initialize params (cannot fail) */
/* If a later switch case returns error without freeing ctx, it leaks */
```

However, tracing the code: all paths in this function either succeed and store the context pointer, or fail before creating the context. The `default:` case at line 1053 returns `-ENOTSUP` but is only reachable if `sess->asym.xform_type` is neither `ML_KEM` nor `ML_DSA`, which means neither `imb_ml_kem_new()` nor `imb_ml_dsa_new()` has been called yet.

Upon re-reading: **no leak exists**. Each `case` block creates its context and returns 0 on success, or returns an error before creating the context. The `default` case is unreachable after a successful `imb_*_new()`.

**Correction: no error here.** The code is correct. *(Self-correcting per the evidence rule: the claim was based on incomplete tracing. After reading the full function, all early returns occur before context creation.)*

**Missing error check on imb_ml_kem_set_privkey/pubkey (Error)**

In `aesni_mb_asym_process()` at lines 2588, 2595, 2612, 2619, 2643, and 2657, calls to `imb_ml_kem_set_privkey()`, `imb_ml_kem_set_pubkey()`, `imb_ml_dsa_set_privkey()`, and `imb_ml_dsa_set_pubkey()` check the return code and conditionally call the next function, but if the `set_*key()` call fails, `rc` is left at its failed value and the operation is marked `RTE_CRYPTO_OP_STATUS_ERROR`. This is correct.

**Correction: no error here.** The pattern `if (rc == 0) rc = next_call()` is correct.

**Missing break statement in switch (Error)**

At line 2711 in `pmd_aesni_mb.c`, the `case RTE_CRYPTO_ML_DSA_OP_VERIFY:` block does not have a `break` at its end before the `default:` case (line 2713). C requires an explicit `break` or the execution falls through to the next case, causing the `default` case to set `op->status = RTE_CRYPTO_OP_STATUS_INVALID_ARGS` even when the verify operation succeeded.

```c
case RTE_CRYPTO_ML_DSA_OP_VERIFY:
    /* ... verify logic ... */
    /* MISSING break; */
default:
    op->status = RTE_CRYPTO_OP_STATUS_INVALID_ARGS;
    break;
```

Add `break;` at line 2712 (after the verify block).

### Warnings

**Non-const function pointer arrays in capabilities (Warning)**

In `pmd_aesni_mb_priv.h`, the `aesni_mb_capabilities[]` array at line 34 is not declared `const`, even though its contents are fully initialized at compile time and never modified. The array includes function pointers (indirectly, via the capabilities structs). A non-const ops table can be overwritten by bugs or exploits.

```c
/* GOOD */
static const struct rte_cryptodev_capabilities aesni_mb_capabilities[] = {
```

This is pre-existing (the patch adds entries but does not change the declaration), but the new asymmetric capabilities at lines 1109-1161 inherit the issue.

---

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

### Clean

No correctness issues found. The patch adds a test suite registration following the existing pattern. No resource leaks, no logic errors.

---

# Summary

**Errors:**
1. Missing error checks on `imb_aes*_gcm_pre()` calls (patch 1)
2. Version macro condition incorrect for wrapper functions (patch 1)
3. Missing `break` statement after `RTE_CRYPTO_ML_DSA_OP_VERIFY` case (patch 3)
4. Missing error checks on `IMB_AES_KEYEXP_256()` calls (patch 2)

**Warnings:**
1. Release notes formatting inconsistencies (patches 1, 2)
2. Non-const capabilities array (patch 3, pre-existing but extended)

Review-Result: ERROR