|WARNING| [v2,6/6] crypto/dpaa2_sec: support AES-GMAC

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/170358

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-30

# DPDK Patch Review - series.patch

## Summary

Reviewed 6 patches from the dpaa2_sec crypto driver series. Found **2 correctness errors** and **1 documentation issue**.

---

## Patch 1/6: crypto/dpaa2_sec: fix buffer overflow in GCM decrypt

**Errors:**

None.

**Warnings:**

None.

**Info:**

The patch correctly identifies and fixes a buffer overflow where the FLE pool buffer (256 bytes = 8x32) was too small for the worst-case SGE layout (9 entries x 32 bytes = 288 bytes). The increase to 288 bytes and the addition of bpid/IVP initialization for sge+4 are both correct.

---

## Patch 2/6: crypto/dpaa2_sec: fix FLE pool leak on sec FD build failure

**Errors:**

1. **Loop variable reuse causes incomplete cleanup (use-after-free risk)**

The cleanup code reuses the outer loop variable `loop` for the cleanup iteration after setting `frames_to_send = loop + 1`. This causes the cleanup loop to start at the failed index (the value of `loop` after the break) rather than at 0, freeing only entries `[loop..loop]` (i.e., just the failed entry itself) instead of `[0..loop]` (all allocated entries).

```c
/* BAD - loop variable is reused, cleanup starts at wrong index */
ret = build_sec_fd(*ops, &fd_arr[loop], bpid, dpaa2_qp);
if (ret) {
    DPAA2_SEC_DP_DEBUG("FD build failed");
    frames_to_send = loop + 1;
    for (loop = 0; loop < frames_to_send; loop++)  /* loop already == failed index */
        free_fle(&fd_arr[loop], dpaa2_qp);
    goto skip_tx;
}
```

When `build_sec_fd` fails at index `N`, the outer loop breaks with `loop == N`. The cleanup `for` loop then initializes `loop = 0`, but this happens **after** the outer loop has already set `loop = N`. The cleanup loop condition `loop < frames_to_send` (where `frames_to_send = N+1`) will be true, so it will iterate once starting from `loop = N`, freeing only `fd_arr[N]`, then increment `loop` to `N+1` and exit. Entries `0..N-1` are never freed.

**Suggested fix:**

Use a distinct loop variable for cleanup:

```c
ret = build_sec_fd(*ops, &fd_arr[loop], bpid, dpaa2_qp);
if (ret) {
    DPAA2_SEC_DP_DEBUG("FD build failed");
    frames_to_send = loop + 1;
    for (uint32_t i = 0; i < frames_to_send; i++)
        free_fle(&fd_arr[i], dpaa2_qp);
    goto skip_tx;
}
```

This pattern appears in both `dpaa2_sec_enqueue_burst` and `dpaa2_sec_enqueue_burst_ordered`; both instances must be fixed.

**Warnings:**

None.

---

## Patch 3/6: crypto/dpaa2_sec: increase ivsize range for AES-CTR

**Errors:**

None.

**Warnings:**

None.

**Info:**

The change is correct. NIST SP 800-38A allows IV lengths from 1 to block size (16 bytes) for CTR mode, with 96 bits (12 bytes) being the most common. The new range `{.min = 12, .max = 16, .increment = 4}` supports both 12-byte and 16-byte IVs.

---

## Patch 4/6: crypto/dpaa2_sec: add missing ECN capability

**Errors:**

None.

**Warnings:**

None.

**Info:**

Setting `.ecn = 1` in the IPsec tunnel-mode security capabilities is correct and indicates the driver supports ECN bit copying during encap/decap.

---

## Patch 5/6: crypto/dpaa2_sec: add support for env variables

**Errors:**

2. **`getenv()` forbidden in drivers/**

The patch adds `getenv()` calls in a driver (`drivers/crypto/dpaa2_sec/`). Per the guidelines (Forbidden Tokens table), `getenv()` is not allowed in `lib/` and `drivers/` except in EAL. Environment variable access in drivers violates portability and determinism requirements.

```c
/* BAD - getenv() in a driver */
env = getenv(DRIVER_STRICT_ORDER);
if (env)
    internals->en_loose_ordered = !atoi(env);

env = getenv(DRIVER_DUMP_MODE);
if (env) {
    dpaa2_sec_dp_dump = atoi(env);
    ...
}
```

**Suggested fix:**

Remove the `getenv()` calls entirely. If configuration from the environment is required, this should be done via devargs (which the driver already supports) or by reading configuration from a file/registry in a platform-specific manner. Devargs are the standard DPDK mechanism for driver configuration.

If the goal is to allow configuration without modifying EAL arguments, consider:
- Using a JSON configuration file read by the application and passed via devargs
- Extending the devargs parsing to accept a config file path
- Documenting the existing devargs mechanism more prominently

**Warnings:**

None.

---

## Patch 6/6: crypto/dpaa2_sec: support AES-GMAC

**Errors:**

None.

**Warnings:**

3. **Documentation does not match code behavior for IPsec GMAC IV source**

The commit message states: "For the IPsec lookaside protocol path, AES-GMAC maps to OP_PCL_IPSEC_AES_NULL_WITH_GMAC."

However, the code in `dpaa2_sec_set_ipsec_session()` copies the salt from `ipsec_xform->salt` into the PDB for the `OP_PCL_IPSEC_AES_NULL_WITH_GMAC` case (added to both encap and decap switch statements at lines 3259 and 3400). This means the IV construction for IPsec GMAC follows the same pattern as IPsec GCM: 4-byte salt from the xform, plus an 8-byte IV from the packet.

The documentation added to `dpaa2_sec.rst` only states "RTE_CRYPTO_AUTH_AES_GMAC" is supported, with no mention of the IPsec protocol path or how the IV is supplied in that case. The user has no guidance on whether the IV is per-packet (like symmetric GMAC) or built from salt+IV (like IPsec GCM).

**Suggested fix:**

Add a note to the documentation clarifying the two usage modes:

```rst
* ``RTE_CRYPTO_AUTH_AES_GMAC`` (symmetric auth-only: per-packet IV via auth.iv;
  IPsec lookaside protocol: 4-byte salt from ipsec_xform, 8-byte IV per packet)
```

Or add a separate subsection under "Supported Features" explaining GMAC IV handling for both paths, similar to how GCM is documented.

**Info:**

The symmetric auth-only path is correct: IV is passed via `session->iv.offset/length` and the shared descriptor is built with `cnstr_shdsc_gcm_encap/decap`.

The IPsec protocol path workaround (placing the GMAC key in `cipherdata` because the SEC hardware treats `OP_PCL_IPSEC_AES_NULL_WITH_GMAC` as a cipher type) is a valid hardware limitation workaround and is correctly implemented.

---

## Final Recommendation

- **Patch 1/6**: Accept (correct fix)
- **Patch 2/6**: **Reject** - loop variable reuse causes incomplete cleanup; must use distinct variable
- **Patch 3/6**: Accept (correct)
- **Patch 4/6**: Accept (correct)
- **Patch 5/6**: **Reject** - `getenv()` not allowed in drivers; use devargs only
- **Patch 6/6**: Accept with **Warning** - add IPsec IV documentation

---

## Errors Summary

1. **Patch 2**: Loop variable reuse in cleanup path frees only the failed entry, not all allocated entries (resource leak, entries 0..N-1 never freed)
2. **Patch 5**: `getenv()` used in driver code (forbidden token, violates portability)

## Warnings Summary

3. **Patch 6**: Documentation does not describe IPsec GMAC IV construction (salt+IV, same as GCM)