|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

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

### Errors

**Buffer size calculation (Correctness)**
The patch increases `FLE_POOL_BUF_SIZE` from 256 to 288 bytes (9 x 32) to accommodate 8 SGEs plus 16 bytes of `old_icv` storage. However, the commit message states the layout uses "8 entries plus 16 bytes of old_icv storage at index 8," which implies the `old_icv` is stored starting at byte offset 256 (8 x 32). If `old_icv` is 16 bytes and needs to fit entirely within the buffer, the calculation appears correct. But if the hardware or code expects alignment or additional space beyond the 16-byte `old_icv`, this should be verified. The fix assumes exactly 9 x 32 = 288 bytes is sufficient--if the actual requirement is larger (e.g., cache-line padding), the buffer could still overflow.

**Recommendation**: Verify that 288 bytes is sufficient for all paths through `build_authenc_gcm_fd` when AAD and decrypt are active. If the layout requires additional padding or if other code paths write beyond byte 288, increase the size accordingly or add a compile-time assertion (e.g., `RTE_BUILD_BUG_ON(sizeof(layout) > FLE_POOL_BUF_SIZE)`).

---

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

### Errors

None. The fix correctly clamps `frames_to_send` to `loop + 1` and iterates from 0 to free all allocated FLE buffers up to and including the failed entry. This addresses the resource leak on the error path.

---

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

### Warnings

**Copyright year update**
The patch updates `dpaa2_sec_priv.h` copyright to `2016,2020-2026` but does not update the copyright in the modified capability structure comment. Per AGENTS.md, copyright years/holders are not subject to AI review, but consistency across the file is good practice.

**Recommendation**: No action required per guidelines, but consider updating all copyright lines in files with functional changes for consistency.

---

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

No issues found. The patch correctly sets `.ecn = 1` in both tunnel-mode security capability entries.

---

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

### Errors

**`getenv()` forbidden in drivers**
The patch calls `getenv("DRIVER_STRICT_ORDER")` and `getenv("DRIVER_DUMP_MODE")` in `dpaa2_sec_get_devargs()`. Per AGENTS.md, `getenv()` is forbidden in `drivers/` (it is only allowed in EAL and examples/test code). Environment variable access bypasses the EAL device argument system and introduces untestable, environment-dependent behavior in drivers. Configuration must come through devargs or the EAL framework.

**Recommendation**: Remove the `getenv()` calls. If runtime configuration without command-line arguments is required, use a different mechanism such as:
- Exposing driver parameters via sysfs or a control interface
- Using EAL's `-a` devargs or `--vdev` parameters exclusively
- Documenting that users must set devargs in their application or wrapper scripts

**Code example to remove**:
```c
env_set:
	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);
		if (dpaa2_sec_dp_dump > DPAA2_SEC_DP_FULL_DUMP)
			dpaa2_sec_dp_dump = DPAA2_SEC_DP_FULL_DUMP;
	}
```

Replace with devargs-only configuration or remove the feature.

### Warnings

**Control flow after `rte_kvargs_process()`**
The patch adds `if (!ret) return;` after `rte_kvargs_process()`. If `rte_kvargs_process()` succeeds (returns 0), the function returns immediately and skips the environment variable fallback. However, `rte_kvargs_process()` returns 0 on success, so `if (!ret)` should be `if (ret == 0)` for clarity. The logic is correct but the style is implicit.

**Recommendation**: Use explicit comparison: `if (ret == 0) return;` to match DPDK style guidelines.

---

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

### Warnings

**Missing release notes**
The patch adds a new authentication algorithm (`RTE_CRYPTO_AUTH_AES_GMAC`) and updates the driver capabilities. This is a significant feature addition that should be documented in the release notes under the current release file (not mentioned in the patch).

**Recommendation**: Add an entry to `doc/guides/rel_notes/release_XX_YY.rst` (where XX_YY is the current release, e.g., 26.03) under "New Features" describing the AES-GMAC support.

**Example**:
```rst
* **crypto/dpaa2_sec: Added AES-GMAC support.**

  Added support for AES-GMAC authentication in both symmetric auth-only
  and IPsec lookaside protocol modes.
```

### Info

**`memset()` before structure use**
The patch adds `memset(&authdata, 0, sizeof(authdata));` and `memset(&cipherdata, 0, sizeof(cipherdata));` at the start of `dpaa2_sec_set_ipsec_session()`. These structures are stack-allocated and fully initialized before use in all code paths, so the `memset()` is defensive but not strictly necessary. However, for crypto/security structures that may have padding bytes read by hardware or logged in debug dumps, zeroing is acceptable practice.

**No action required** (defensive programming in crypto code is reasonable).

---

## Summary

### Critical Issues (Must Fix)
- **Patch 5**: Remove `getenv()` calls in driver code (forbidden in `drivers/` per AGENTS.md)

### Recommended Fixes
- **Patch 1**: Verify 288-byte buffer size is sufficient for all GCM decrypt + AAD code paths
- **Patch 5**: Use explicit comparison `if (ret == 0)` instead of `if (!ret)`
- **Patch 6**: Add release notes entry for AES-GMAC feature

### No Issues
- Patch 2: FLE pool leak fix is correct
- Patch 3: AES-CTR IV size range expansion is correct
- Patch 4: ECN capability addition is correct