|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 pool size calculation**

The patch increases `FLE_POOL_BUF_SIZE` from 256 to 288 bytes to accommodate 9 SGE entries (9 x 32 = 288). However, the comment states "8 entries plus 16 bytes of old_icv storage at index 8", which would be 8 x 32 + 16 = 272 bytes, not 288.

If the layout truly requires 8 SGEs plus 16 bytes of ICV, the size should be 272. If it requires 9 full 32-byte entries, the comment should be corrected to reflect that. Verify which is correct and ensure the size and comment match.

### Warnings

**Missing release notes**

This is a correctness bug fix (buffer overflow causing IOMMU faults). The patch should update the current release notes to document this fix, as it resolves a critical issue that would cause SEC engine failures.

---

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

### Errors

**Potential use-after-free on loop variable**

```c
frames_to_send = loop + 1;
for (loop = 0; loop < frames_to_send; loop++)
    free_fle(&fd_arr[loop], dpaa2_qp);
```

The cleanup loop reuses the same `loop` variable that was just used in the outer loop. When the inner cleanup loop completes, `loop` will equal `frames_to_send`, modifying the outer loop's counter. This breaks the outer loop's continuation when `goto skip_tx` returns control.

Use a distinct loop counter for the cleanup:

```c
frames_to_send = loop + 1;
for (int i = 0; i < frames_to_send; i++)
    free_fle(&fd_arr[i], dpaa2_qp);
```

This pattern appears in both `dpaa2_sec_enqueue_burst` and `dpaa2_sec_enqueue_burst_ordered`.

### Warnings

**Missing release notes**

FLE pool exhaustion is a serious resource leak that causes the driver to stop functioning. This fix should be documented in the release notes.

---

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

No issues found.

---

## 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**

```c
env = getenv(DRIVER_STRICT_ORDER);
if (env)
    internals->en_loose_ordered = !atoi(env);

env = getenv(DRIVER_DUMP_MODE);
```

The guidelines prohibit `getenv()` in `drivers/` unless absolutely necessary. Driver configuration should use devargs, not environment variables. If environment variable support is required, it must be justified and the variables should be documented as an alternative mechanism, not a primary configuration method.

The patch documentation states this is "useful in environments where command-line access is restricted", but this is not a strong justification--most deployment systems can configure EAL arguments programmatically. If this is retained, add a clear note in the code explaining why `getenv()` is acceptable here.

**Undefined macro constants**

The code references `DRIVER_STRICT_ORDER` and `DRIVER_DUMP_MODE` macros, but these are not defined anywhere in the patch. These should be string literals matching the devarg names, or `#define` constants should be added.

Suggested fix:

```c
#define DRIVER_STRICT_ORDER "drv_strict_order"
#define DRIVER_DUMP_MODE "drv_dump_mode"
```

or use the string literals directly in `getenv()` calls.

**Missing return value check on atoi()**

`atoi()` is used without validating that `env` is non-NULL or non-empty. While the `if (env)` check prevents NULL dereference, `atoi()` on an empty string or invalid input returns 0, which may silently set incorrect configuration.

Use `strtol()` with error checking, or at minimum check `env[0] != '\0'` before calling `atoi()`.

### Warnings

**Copyright year update without code changes**

The documentation file copyright is updated from "2016 NXP" to "2016,2026 NXP". Copyright years should reflect when the code was written, not when it is modified. If this patch is the first change to the file in 2026, the year update is correct; otherwise, it may be premature.

**Missing release notes**

New feature (environment variable configuration fallback) should be documented in the release notes.

---

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

### Errors

**memset after declarations in dpaa2_sec_set_ipsec_session**

```c
PMD_INIT_FUNC_TRACE();

memset(&authdata, 0, sizeof(authdata));
memset(&cipherdata, 0, sizeof(cipherdata));

RTE_SET_USED(dev);
```

The `memset()` calls are placed after `PMD_INIT_FUNC_TRACE()` and before `RTE_SET_USED(dev)`, mixing initialization code with macro invocations. DPDK style prefers declarations and initialization at the start of the block.

Move the `memset()` calls to immediately after the variable declarations, or use C99 designated initializers at declaration:

```c
struct alginfo authdata = {0};
struct alginfo cipherdata = {0};
```

### Warnings

**Missing release notes**

New feature (AES-GMAC support) must be documented in the release notes as a new capability.

**Features matrix updated but no device operations verified**

The patch adds `AES GMAC (128)`, `AES GMAC (192)`, `AES GMAC (256)` to `dpaa2_sec.ini`, but the guidelines require verifying that device operations match documentation per `features.rst`. Ensure that the GMAC implementation correctly maps to the expected device operation flow for both symmetric auth-only and IPsec protocol modes.

---

## Summary

### Critical Issues (must fix before merge)

1. **Patch 1**: Verify buffer size calculation (272 vs 288 bytes)
2. **Patch 2**: Loop counter collision in cleanup path (use-after-free risk on loop variable)
3. **Patch 5**: Undefined macros `DRIVER_STRICT_ORDER` and `DRIVER_DUMP_MODE`
4. **Patch 5**: Prohibited `getenv()` in driver code without strong justification

### Important Issues (should fix)

1. **Patches 1, 2, 5, 6**: Missing release notes for bug fixes and new features
2. **Patch 5**: Missing error handling on `atoi()` (use `strtol()`)
3. **Patch 6**: `memset()` placement violates style (use designated initializers)

### Info

- **Patch 5**: Copyright year update may be premature depending on when changes were made