|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 Crypto Driver Patch Review

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

### Errors

1. **Function pointer wrapper signature mismatch (GCM decrypt finalize)**
   In `pmd_aesni_gcm.c`, the wrapper functions for `gcm_dec_finalize` have a void return type,
   but they are assigned to `aesni_gcm_finalize_t` function pointers which the code later uses
   in contexts that may expect error returns. While the IPsecMB library's `gcm_dec_finalize`
   itself returns void, the DPDK code calls these function pointers and then uses `rte_memeq_timingsafe()`
   separately to check the digest. However, there is no verification that the finalize wrappers
   are consistently typed with their usage. Review that the void return is intentional and
   that all error paths are handled via the subsequent `rte_memeq_timingsafe()` call.

2. **Macro redefinition without `#undef` for pre-3.0 compatibility defines**
   In `pmd_aesni_mb_priv.h`, lines 24-28 define macros like `IMB_CIPHER_SNOW3G_UEA2` to map
   to the old `_BITLEN` versions when `IMB_VERSION < 3.0.0`. However, these macros are not
   undefined before redefinition in later code if the header is included multiple times
   (though this is unlikely given header guards). More importantly, the code does not verify
   that the old `_BITLEN` enum values actually exist in older library versions -- if a user
   compiles with a very old IPsecMB that predates even the `_BITLEN` naming,
   the build would fail with undeclared identifiers. Add a check or comment
   that IPsecMB >= 2.0 is required for the `_BITLEN` versions to exist.

3. **Missing bounds check on `iv_len` parameter in gmac_init wrapper**
   In `pmd_aesni_gcm.c`, the wrapper functions `imb_aes*_gmac_init` take an `iv_len` parameter
   (lines 106-120 for the 128-bit case). This parameter comes from `session->iv.length`
   or other session fields. While the session setup code validates IV lengths,
   the wrappers themselves do not guard against a caller passing an out-of-range `iv_len`.
   If a malformed session or race condition provides a large `iv_len`,
   the underlying `state->gmac*_init()` could read out of bounds. Verify that session
   validation is airtight, or add a defensive bounds check in the wrapper.

### Warnings

1. **Inconsistent error path in `aesni_gcm_session_configure`**
   At line 289 (`pmd_aesni_gcm.c`), the function calls `imb_aes128_gcm_pre()`
   but does not check if it can fail (though the IPsecMB API likely makes these `pre` functions
   always succeed). However, the code does not document this assumption. If a future IPsecMB
   version makes these functions failable, there is no error handling. Add a comment
   noting that `gcm*_pre` is assumed to always succeed, or wrap the call in an error check.

2. **Type widening in wrapper function calls may truncate length parameters**
   The wrapper functions in `pmd_aesni_gcm.c` cast length parameters from `uint64_t`
   to the IPsecMB library's native types. While the library likely uses `uint64_t` internally,
   the wrappers do not verify that the lengths fit in the target type. If the library's
   signature changes to use `size_t` (which could be 32 bits on some platforms),
   a very large `uint64_t` length could be truncated. This is unlikely in practice
   but worth noting for portability. Consider adding a compile-time assertion
   that `sizeof(size_t) >= 8` on supported platforms, or document the assumption.

3. **Release notes claim support for v3.0, but code has fallback for v2.x**
   The code includes extensive `#if IMB_VERSION(3, 0, 0) > IMB_VERSION_NUM` blocks
   to support older library versions, yet the release notes (`release_26_11.rst`)
   and documentation (`aesni_gcm.rst`, `aesni_mb.rst`) state "The latest version
   of the library supported by this PMD is v3.0" without mentioning backward compatibility.
   This could confuse users. Either update the docs to clarify that v2.x is still supported
   (but deprecated), or remove the v2.x compatibility code if v3.0 is now mandatory.

### Info

1. **Wrapper functions are only generated for IPsecMB < 3.0**
   The `#if IMB_VERSION(3, 0, 0) > IMB_VERSION_NUM` block at lines 9-145 in `pmd_aesni_gcm.c`
   defines wrapper functions that are only needed when linking against IPsecMB < 3.0.
   For v3.0+, the code uses the direct API functions (which have the same signature).
   This is correct, but the dual code path is complex. Consider a comment at the top
   of the file explaining the versioning strategy to aid future maintainers.

2. **Array initialization of `ops[GCM_KEY_*]` in `aesni_gcm_set_ops`**
   The function `aesni_gcm_set_ops` assigns function pointers and the `mb_mgr` pointer
   to the `ops` array. The order is clear, but there is no verification that `GCM_KEY_128`,
   `GCM_KEY_192`, and `GCM_KEY_256` are contiguous enum values starting at 0. If the enum
   order changes, the assignments could index out of bounds. Consider using designated
   initializers or a compile-time assertion on the enum values.

3. **Removal of ZUC 256 support noted in docs but not in release notes deprecation section**
   The `aesni_mb.rst` update adds a "Note" section stating that ZUC 256 support was removed
   in IPsecMB v3.0, but this is not listed in the deprecation section of the release notes.
   If ZUC 256 was a previously supported feature, its removal should be noted under
   "Removed Items" in `release_26_11.rst` for visibility to users upgrading.

---

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

### Errors

1. **Missing validation of AAD length for NxA4/5/6 AEAD algorithms**
   At lines 803-869 (`pmd_aesni_mb.c`), the code sets `sess->template_job.u.GCM.aad_len_in_bytes`
   or `sess->template_job.u.NCA.aad_len_in_bytes` from `xform->aead.aad_length` without
   validating that the AAD length is within the algorithm's supported range. While the code
   checks digest size (lines 820-824, 843-847, 865-869), it does not verify AAD length.
   If the algorithm has a maximum AAD length (e.g., per the 5G NR spec), exceeding it
   could cause the IPsecMB library to fail silently or produce incorrect results.
   Add a bounds check on `xform->aead.aad_length` for each algorithm.

2. **Key expansion called but expanded keys not assigned for NCA6 (ZUC)**
   At lines 854-858 (`pmd_aesni_mb.c`), the ZUC NCA6 case calls `IMB_AES_KEYEXP_256()`
   and assigns the expanded keys to `sess->template_job.enc_keys` and `dec_keys`.
   However, ZUC is a stream cipher and does not use AES expanded keys --
   this is a copy-paste error from the AES NCA5 case. The correct approach for ZUC
   is to copy the key to a session-local buffer (similar to ZUC EIA3 at lines 252-254)
   and set `sess->template_job.u.ZUC_EIA3._key` (or the NCA6 equivalent field).
   Using AES key expansion for ZUC will produce wrong results or crash.

3. **Missing IV length validation for NEA4/5/6 cipher modes**
   At lines 475-490 (`pmd_aesni_mb.c`), the cipher configuration sets the cipher mode
   for NEA4/5/6 but does not validate that `xform->cipher.iv.length == 16` (the required
   IV size per the capabilities table at lines 886-964, `pmd_aesni_mb_priv.h`).
   The subsequent `is_nxan` block at lines 633-648 checks key length but not IV length.
   If a user provides an IV of the wrong length, the behavior is undefined. Add:
   ```c
   if (xform->cipher.iv.length != 16) {
       IPSEC_MB_LOG(ERR, "Invalid cipher IV length");
       return -EINVAL;
   }
   ```
   in the `is_nxan` block.

### Warnings

1. **Inconsistent digest size validation across AEAD algorithms**
   The NCA4/5/6 AEAD cases check `4 <= digest_len <= 16` (lines 820-824, 843-847, 865-869),
   but other AEAD algorithms (e.g., AES GCM) do not have this check in the same function.
   While the capabilities table defines the valid range, explicit validation at session
   configure time is good practice. Consider adding similar checks for all AEAD algorithms,
   or document why NCA4/5/6 need it but GCM does not.

2. **Release notes list features without specifying IPsecMB version requirement**
   The release notes at lines 39-41 (`release_26_11.rst`) state "Added support for NEA4/5/6,
   NIA4/5/6, NCA4/5/6" but do not mention that these require IPsecMB v3.0 or later.
   Users with an older IPsecMB will see these features in the release notes but will not
   be able to use them (the code is gated by `#if IMB_VERSION(3, 0, 0) <= IMB_VERSION_NUM`).
   Add a note: "Requires IPsecMB v3.0 or later" to each bullet point.

3. **NEA/NIA/NCA naming in docs does not match enum names**
   The release notes use "NEA4, NIA4, NCA4: Snow 5G...", but the enum names in the code are
   `RTE_CRYPTO_CIPHER_SNOW5G_NEA4`, etc. While the mapping is clear to a domain expert,
   the docs should use the full enum names (e.g., "SNOW5G_NEA4") for clarity,
   or at least cross-reference the enum names in a note.

### Info

1. **NEA5 uses AES-256 in CTR mode per the 5G spec**
   The code at lines 480-482 sets `IMB_CIPHER_AES_NEA5` as the cipher mode. While this is
   correct per the IPsecMB library, a comment noting that NEA5 is AES-256-CTR (per 3GPP TS 33.501)
   would aid understanding for developers unfamiliar with 5G crypto. Similarly for NIA5 (AES-256-CMAC).

2. **ZUC NEA6/NIA6 are ZUC-256 variants**
   The code uses `IMB_CIPHER_ZUC_NEA6` and `IMB_AUTH_ZUC_NIA6`, which are the 256-bit key
   versions of ZUC. This is distinct from the older ZUC-128 (ZUC EEA3/EIA3) and the removed
   ZUC-256 EEA3/EIA3 from patch 1/4. A comment clarifying the relationship between these
   ZUC variants would help avoid confusion (e.g., "NEA6 is ZUC-256 per 5G spec;
   distinct from the deprecated ZUC256_EIA3").

---

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

### Errors

1. **Missing error check on `imb_ml_kem_new()` and `imb_ml_dsa_new()`**
   At lines 969 and 1000 (`pmd_aesni_mb.c`), the code calls `imb_ml_kem_new()` and `imb_ml_dsa_new()`
   and checks the return value (`ret`), but the error path at lines 970-973 and 1001-1004
   only logs and returns -- it does not set `sess->asym.ml_kem.ctx = NULL` or `sess->asym.ml_dsa.ctx = NULL`.
   If `imb_ml_*_new()` fails, `ctx` is left uninitialized (garbage pointer). Later, in
   `aesni_mb_asym_session_clear()` (lines 1046-1055), the code calls `imb_ml_kem_free(sess->asym.ml_kem.ctx)`
   without checking if `ctx` is NULL. This will dereference an uninitialized pointer, causing a crash.
   Fix: set `ctx = NULL` on allocation failure:
   ```c
   ret = imb_ml_kem_new(mb_mgr, sess->asym.ml_kem.alg, &sess->asym.ml_kem.ctx);
   if (ret) {
       sess->asym.ml_kem.ctx = NULL;  // <-- add this
       IPSEC_MB_LOG(ERR, "Failed to create ML KEM session");
       return ret;
   }
   ```
   And add a NULL check in the clear function:
   ```c
   if (sess->asym.ml_kem.ctx != NULL)
       imb_ml_kem_free(sess->asym.ml_kem.ctx);
   ```

2. **Use-after-free in `aesni_mb_asym_process()` for ML_DSA sign operation**
   At lines 2651-2655 (`pmd_aesni_mb.c`), the code sets `sess->asym.ml_dsa.sign_params.ctx`
   and `sess->asym.ml_dsa.sign_params.rnd_32` to point to `siggen->ctx.data` and `siggen->seed.data`.
   These are pointers into the `rte_crypto_op` structure, which is owned by the caller.
   After `imb_ml_dsa_sign()` completes (line 2669), the op may be immediately freed by the application,
   but the session still holds these dangling pointers. If the session is reused for another
   operation before being reconfigured, the old pointers could be accessed.
   While this is unlikely (the params are overwritten on each call), it is safer to clear
   the pointers after use or document that the session must not be accessed after `process()` completes.

3. **ML-KEM keygen uses stack buffer for seed that goes out of scope**
   At lines 2605-2607 (`pmd_aesni_mb.c`), the code declares `uint8_t seed_d_z[64]` on the stack,
   copies `keygen->d.data` and `keygen->z.data` into it, then sets
   `sess->asym.ml_kem.keygen_params.seed_d_z = seed_d_z`. This pointer is then passed to
   `imb_ml_kem_keypair()` at line 2611. If `imb_ml_kem_keypair()` stores this pointer
   for later use (asynchronous operation or internal caching), the pointer will dangle
   when `seed_d_z` goes out of scope. Verify that `imb_ml_kem_keypair()` does not retain
   the pointer