|WARNING| [6/6] crypto/dpaa2_sec: add support for env variables

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

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

### Errors

**Buffer overflow fix incomplete:**
The patch increases FLE_POOL_BUF_SIZE from 256 to 288 bytes and extends bpid/IVP initialization to sge+4.
However, the calculation appears inconsistent with the commit message.
The message states "8 entries plus 16 bytes of old_icv storage at index 8" which would be 8x32 + 16 = 272 bytes, not 288.
Verify that 288 bytes (9x32) is actually needed, or if the allocation should be 272 bytes with explicit accounting for the partial entry.

**Missing bounds verification:**
The patch does not show where the old_icv write at index 8 occurs in build_authenc_gcm_fd.
Without seeing that code, cannot verify whether other code paths might also overflow the new 288-byte limit.
The fix assumes index 8 is the maximum, but there is no visible bounds check to enforce this.

### Warnings

**Incomplete commit message:**
The commit message describes an IOMMU fault symptom but does not explain the second issue (bpid/IVP initialization) clearly.
The connection between "sge+3" initialization and the need to cover "sge+4" is stated but not justified.
Consider adding: "When AAD is present, the input data SGE is at index 4, which was not initialized."

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

### Errors

**Loop variable reuse creates undefined behavior:**
```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 (loop = 0; loop < frames_to_send; loop++)  // Reuses 'loop' variable
        free_fle(&fd_arr[loop], dpaa2_qp);
    goto skip_tx;
}
```
The cleanup loop reuses the same `loop` variable that controls the outer loop.
After the cleanup loop completes, `loop` will equal `frames_to_send` (the failed index + 1).
When execution reaches `skip_tx`, if there is further loop logic, the outer loop counter is corrupted.
This is the "shared loop counter in nested loops" error pattern.

Fix: use a distinct variable name for the cleanup loop:
```c
frames_to_send = loop + 1;
for (int i = 0; i < frames_to_send; i++)
    free_fle(&fd_arr[i], dpaa2_qp);
```

The same error exists in both dpaa2_sec_enqueue_burst and dpaa2_sec_enqueue_burst_ordered.

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

### Errors

**New public API added without experimental tag or release notes:**
The patch adds `RTE_CRYPTO_AEAD_AES_GMAC` to `lib/cryptodev/rte_crypto_sym.h`, which is a public API header.
This is a new enum value in a stable API.
According to DPDK ABI policy, adding an enum value is an ABI change.
The patch must either:
1. Mark the new API as experimental (not applicable to enum values), OR
2. Document the ABI change in release notes and follow ABI versioning policy

**Missing release notes:**
The patch adds a new AEAD algorithm to the public cryptodev API and new driver capability.
This requires an entry in the current release notes (`doc/guides/rel_notes/release_26_07.rst` or similar based on target release).

### Warnings

**Copyright year update spans multiple years:**
```c
- *   Copyright 2016-2025 NXP
+ *   Copyright 2016-2026 NXP
```
The patch updates copyright to 2026.
According to guidelines, copyright years should reflect when the code was originally written.
If this is new code in 2026, adding 2026 is correct.
If the existing 2025 copyright is being bumped as part of routine maintenance, do not update it unless the file has substantive changes.

(Note: per AGENTS.md, copyright years are not subject to AI review. Omitted from final checklist.)

**AES-GMAC error message for AUTH path:**
```c
case RTE_CRYPTO_AUTH_AES_GMAC:
    DPAA2_SEC_ERR(
        "AES_GMAC is supported as AEAD algo for IPSEC proto only");
    return -ENOTSUP;
```
The error message is informative, but users may not know what "AEAD algo" means or how to reconfigure.
Consider: "Use RTE_CRYPTO_AEAD_AES_GMAC instead of RTE_CRYPTO_AUTH_AES_GMAC for IPsec protocol offload."

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

### Info

**Commit message could explain rationale:**
The commit message states the change aligns with "common usage and test-vector expectations" but does not cite a specific test suite or standard.
Consider adding: "Matches NIST SP 800-38A CTR mode IV recommendations (96-bit counter block)."

(No correctness or style errors found.)

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

(No issues found.)

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

### Errors

**getenv() usage in driver is forbidden:**
The AGENTS.md guidelines explicitly forbid `getenv()` in `drivers/` (see Forbidden Tokens).
```c
env = getenv(DRIVER_STRICT_ORDER);
...
env = getenv(DRIVER_DUMP_MODE);
```
Environment variable handling in drivers must go through EAL devargs or be handled at a higher layer (app/examples).
Drivers must not call `getenv()` directly.

Remove the environment variable fallback.
If configuration without command-line access is required, document the devargs in the driver guide and let the orchestration layer (deployment scripts, systemd environment files) construct the EAL arguments.

**atoi() usage without validation:**
```c
internals->en_loose_ordered = !atoi(env);
...
dpaa2_sec_dp_dump = atoi(env);
```
`atoi()` on untrusted input (environment variables) can silently produce 0 for malformed strings.
If environment variable support is added (after resolving the `getenv()` issue), validate the input:
```c
char *endptr;
long val = strtol(env, &endptr, 10);
if (*endptr != '\0' || val < 0 || val > MAX_ALLOWED)
    /* error */
```

### Warnings

**Missing definition of DRIVER_STRICT_ORDER and DRIVER_DUMP_MODE:**
The patch references `DRIVER_STRICT_ORDER` and `DRIVER_DUMP_MODE` macros but does not show their definitions.
If these are string literals, they should be `#define`d in the header.
If they are supposed to be documented, the driver guide must explain their purpose and valid values.

**Missing release notes:**
Adding user-visible configuration options (even if they should not use environment variables) requires documentation in release notes.

---

## Summary by Severity

### Errors (must fix)
1. **Patch 1:** Buffer size calculation inconsistency (256-288 vs stated 272 bytes)
2. **Patch 2:** Loop variable reuse in cleanup path (nested loop counter collision)
3. **Patch 3:** New public API enum value without ABI policy compliance
4. **Patch 3:** Missing release notes for new AEAD algorithm
5. **Patch 6:** Forbidden `getenv()` usage in driver
6. **Patch 6:** Unsafe `atoi()` without validation

### Warnings (should fix)
1. **Patch 1:** Commit message does not fully explain bpid/IVP initialization fix
2. **Patch 3:** AUTH path error message could be more user-friendly
3. **Patch 6:** Missing macro definitions for environment variable names
4. **Patch 6:** Missing release notes for new configuration options

### Info (consider)
1. **Patch 4:** Commit message could cite NIST SP 800-38A reference