|WARNING| [v3,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/170679

_AI Code Review_

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

# DPDK Patch Review

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

### Errors

**Buffer overflow fix incomplete - missing bounds verification**

The patch increases `FLE_POOL_BUF_SIZE` from 256 to 288 bytes to accommodate 9 SGE entries (9 x 32 = 288), but does not verify that the code cannot exceed this new limit. If future changes add more SGE entries or the old_icv storage pattern is replicated elsewhere, the overflow will recur.

**Suggested fix:** Add a compile-time or runtime assertion in `build_authenc_gcm_fd` to verify the SGE layout does not exceed `FLE_POOL_BUF_SIZE`. For example:
```c
RTE_BUILD_BUG_ON(offsetof(struct qbman_fle, data) + (9 * 32) > FLE_POOL_BUF_SIZE);
```
or a runtime check after the last SGE is assigned.

---

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

### Errors

**Resource leak on second failure within same burst**

The cleanup loop frees FLE buffers for indices `0..loop` after `build_sec_fd` fails at index `loop`. However, if a *second* failure occurs in a subsequent iteration, the `frames_to_send` variable is overwritten, and the FLE buffers from the first failure are not freed again. The pool will leak those buffers.

**Suggested fix:** Instead of reassigning `frames_to_send`, free only the current failed entry and break immediately, then free all `0..loop` unconditionally before `skip_tx`:
```c
if (ret) {
    DPAA2_SEC_DP_DEBUG("FD build failed");
    free_fle(&fd_arr[loop], dpaa2_qp);
    break;
}
```
Then before `skip_tx`, add:
```c
for (i = 0; i < loop; i++)
    free_fle(&fd_arr[i], dpaa2_qp);
```

This pattern ensures all allocated FLE buffers are freed regardless of which iteration failed.

---

**Error path does not set ops status**

When `build_sec_fd` fails, the patch jumps to `skip_tx` but does not mark the failed crypto operations with an error status. The application will never know which operations failed, causing silent data loss or corruption.

**Suggested fix:** Before jumping to `skip_tx`, set the status of the failed operation:
```c
if (ret) {
    DPAA2_SEC_DP_DEBUG("FD build failed");
    (*ops)->status = RTE_CRYPTO_OP_STATUS_ERROR;
    frames_to_send = loop + 1;
    for (loop = 0; loop < frames_to_send; loop++)
        free_fle(&fd_arr[loop], dpaa2_qp);
    goto skip_tx;
}
```

The same issue and fix applies to both `dpaa2_sec_enqueue_burst` and `dpaa2_sec_enqueue_burst_ordered`.

---

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

### Warnings

**Missing release notes**

The patch changes the advertised IV size capability for AES-CTR from a fixed 16 bytes to a range [12, 16] with 4-byte increment. This is a user-visible change to the device capabilities structure and should be documented in the current release notes (`doc/guides/rel_notes/release_26_03.rst` or equivalent).

**Suggested fix:** Add a bullet under "New Features" or "Modified" describing the IV size change and its motivation.

---

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

No issues found.

---

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

### Errors

**Use of getenv() in driver code**

The patch calls `getenv("DRIVER_STRICT_ORDER")` and `getenv("DRIVER_DUMP_MODE")` in the driver init path. Per AGENTS.md Forbidden Tokens section, `getenv()` is forbidden in `drivers/` (only allowed in EAL and test code). Environment variables bypass the standard devargs mechanism and make configuration opaque to tooling and logs.

**Suggested fix:** Remove the `getenv()` calls. If runtime configuration without devargs is required, use a different mechanism such as reading from a well-known file or adding EAL-level support for driver environment variable passthrough.

---

**Undefined behavior - atoi() on NULL pointer**

If `getenv()` returns `NULL` (variable not set), passing it to `atoi()` is undefined behavior.

**Suggested fix:** Check the return value of `getenv()` before calling `atoi()`:
```c
env = getenv(DRIVER_STRICT_ORDER);
if (env != NULL)
    internals->en_loose_ordered = !atoi(env);
```

The same issue exists for `DRIVER_DUMP_MODE`.

---

**Missing error handling on rte_kvargs_process failure**

The patch adds:
```c
ret = rte_kvargs_process(kvlist, key, check_devargs_handler, (void *)cryptodev);
rte_kvargs_free(kvlist);
if (!ret)
    return;
```

If `rte_kvargs_process` succeeds (returns 0), the function returns and skips the environment variable fallback. However, if `rte_kvargs_process` fails (returns negative), execution falls through to `env_set` and overwrites the devargs-derived configuration. This is likely not the intended behavior -- a failure in devargs processing should be logged and possibly returned as an error, not silently ignored.

**Suggested fix:** Check the return value and handle failure explicitly:
```c
ret = rte_kvargs_process(kvlist, key, check_devargs_handler, (void *)cryptodev);
rte_kvargs_free(kvlist);
if (ret < 0) {
    DPAA2_SEC_ERR("Failed to process devargs for key %s", key);
    return;
}
if (ret == 0)
    return;
```

---

### Warnings

**Documentation update needed**

The patch adds a new configuration mechanism but updates the documentation in a way that may be confusing. The text says "as a fallback when devargs are not provided" but does not clearly explain the precedence (devargs take priority, then environment variables). The example `export drv_dump_mode=1` should clarify this is system-wide and applies to all DPAA2 devices.

**Suggested fix:** Add a note in the documentation:
> The environment variable is checked only when the corresponding devarg is not provided. Devargs take precedence over environment variables. Environment variables apply system-wide to all DPAA2 cryptodev instances.

---

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

### Errors

**Incomplete cipher/auth swap for IPsec GMAC - authdata->key not cleared**

In `dpaa2_sec_ipsec_proto_init()`, the patch moves the GMAC key from `authdata` to `cipherdata` and sets `authdata->algtype = OP_PCL_IPSEC_HMAC_NULL`. However, it does not zero out `authdata->key`, `authdata->keylen`, or other fields that may have been set earlier if the caller initialized `authdata`. The SEC hardware or descriptor construction may read stale key data from `authdata`.

**Suggested fix:** Explicitly clear `authdata` fields after overriding:
```c
cipherdata->key = (size_t)session->auth_key.data;
cipherdata->keylen = session->auth_key.length;
cipherdata->key_enc_flags = 0;
cipherdata->key_type = RTA_DATA_IMM;
cipherdata->algtype = OP_PCL_IPSEC_AES_NULL_WITH_GMAC;
authdata->key = 0;
authdata->keylen = 0;
authdata->algtype = OP_PCL_IPSEC_HMAC_NULL;
authdata->algmode = 0;
```

Alternatively, ensure that the caller (`dpaa2_sec_set_ipsec_session`) zeroes `authdata` before calling `dpaa2_sec_ipsec_proto_init()` -- the patch already adds `memset(&authdata, 0, sizeof(authdata));`, so this may be sufficient. Verify that no code path writes to `authdata` before the AES-GMAC case in `dpaa2_sec_ipsec_proto_init()`.

---

**Early return in dpaa2_sec_ipsec_proto_init for GMAC bypasses further auth setup**

The `RTE_CRYPTO_AUTH_AES_GMAC` case in `dpaa2_sec_ipsec_proto_init()` does `return 0;` immediately after setting `cipherdata` and `authdata->algtype`. This skips any subsequent auth-related initialization that may be required (e.g., setting `authdata->key_enc_flags`, `authdata->key_type`, etc.). Other auth cases in the same switch statement do not return early.

**Why it matters:** If future code expects those fields to be initialized for all auth algorithms, GMAC will be an exception and may cause bugs.

**Suggested fix:** Remove the early `return 0;` and use `break;` to fall through to the common auth setup at the end of the function (if any). Alternatively, add a comment explaining why the early return is safe.

---

### Warnings

**Missing release notes**

The patch adds a new authentication algorithm (AES-GMAC) and updates the features matrix. This is a user-visible feature addition and should be documented in the release notes under "New Features" for the dpaa2_sec driver.

**Suggested fix:** Add a bullet in `doc/guides/rel_notes/release_26_03.rst` (or the current release notes file):
> * **crypto/dpaa2_sec: Added AES-GMAC support.**
>
>   Added AES-GMAC authentication-only and IPsec lookaside protocol support.

---

**Auth-only GMAC session uses GCM encap/decap descriptors - verify hardware compatibility**

The auth-only AES-GMAC path calls `cnstr_shdsc_gcm_encap/decap()` with `authonly=1`. This is the expected pattern, but the reviewer cannot verify from the patch whether the DPAA2 SEC hardware correctly handles GCM descriptors in auth-only mode with zero-length payload and per-packet IV from `sym_op->auth.iv`. If the hardware expects the IV in a different location or requires additional setup, the implementation will fail.

**Suggested fix:** Add a comment referencing hardware documentation or test results confirming that `cnstr_shdsc_gcm_encap/decap` with `authonly=1` is the correct descriptor construction for AES-GMAC auth-only operations.

---

## Summary

### Patch 1/6
- **Error:** Missing bounds verification for new `FLE_POOL_BUF_SIZE` limit

### Patch 2/6
- **Error:** Resource leak on second failure within same burst
- **Error:** Failed operations not marked with error status

### Patch 3/6
- **Warning:** Missing release notes

### Patch 4/6
- No issues

### Patch 5/6
- **Error:** Use of forbidden `getenv()` in driver code
- **Error:** Undefined behavior - `atoi(NULL)` if environment variable not set
- **Error:** Incorrect error handling on `rte_kvargs_process` failure
- **Warning:** Documentation should clarify devargs vs environment variable precedence

### Patch 6/6
- **Error:** `authdata->key` not cleared after cipher/auth swap (verify `memset` coverage)
- **Error:** Early return in `dpaa2_sec_ipsec_proto_init` for GMAC bypasses further setup
- **Warning:** Missing release notes
- **Warning:** Auth-only GMAC descriptor construction should be verified against hardware