|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 - SGE indices beyond 4 not validated**

The patch fixes the immediate buffer overflow by increasing `FLE_POOL_BUF_SIZE` from 256 to 288 bytes (8-9 entries), and extends `bpid`/`IVP` initialization to cover `sge+4`. However, the commit message states the layout uses "8 entries plus 16 bytes of old_icv storage at index 8", which implies `old_icv` is written to `sge[8]`. If that is the case, the code may access SGE indices beyond 4 (indices 5, 6, 7, 8) without initializing their `bpid`/`IVP` fields. The patch only adds initialization for `sge+4`.

**Suggested fix:**
Review the actual SGE layout in `build_authenc_gcm_fd()` to determine all indices that are written. If indices 5-8 are used, extend the `bpid`/`IVP` initialization loop to cover all of them, or explicitly initialize each required index.

```c
/* If sge[5], sge[6], sge[7], sge[8] are used, initialize them: */
DPAA2_SET_FLE_BPID(sge + 5, bpid);
DPAA2_SET_FLE_BPID(sge + 6, bpid);
DPAA2_SET_FLE_BPID(sge + 7, bpid);
DPAA2_SET_FLE_BPID(sge + 8, bpid);
/* or for IVP branch: */
DPAA2_SET_FLE_IVP((sge + 5));
/* ... etc */
```

**Missing Cc: stable at dpdk.org tag propagation to release notes**

The patch correctly includes `Cc: stable at dpdk.org` in the commit message but does not update the release notes (`doc/guides/rel_notes/release_*.rst`). Buffer overflow fixes are significant correctness bugs that should be documented.

**Suggested fix:**
Add an entry to the "Fixed Issues" section of the current release notes (`doc/guides/rel_notes/release_26_03.rst` or equivalent) describing the buffer overflow and IOMMU fault.

---

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

### Errors

**None** - the fix correctly clamps `frames_to_send` and iterates from 0 to free all allocated FLE buffers including the failed entry.

### Warnings

**Missing release notes entry**

This is a correctness bug (resource leak causing test hangs). It should be documented in the release notes.

**Suggested fix:**
Add an entry to the "Fixed Issues" section of the current release notes describing the FLE pool leak and its symptoms (crypto-perf hanging after pool exhaustion).

---

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

### Warnings

**Missing release notes entry**

This is an API-visible change (capability range expansion). Applications relying on the capability query will see a different result. This should be documented.

**Suggested fix:**
Add an entry to the "New Features" or "API Changes" section of the current release notes noting the expanded AES-CTR IV size range.

---

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

### Warnings

**Missing release notes entry**

Adding a new capability flag is an API-visible change. Applications querying security capabilities will see `.ecn = 1` where it was previously 0.

**Suggested fix:**
Add an entry to the "New Features" section of the current release notes noting that ECN capability is now advertised.

---

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

### Errors

**`getenv()` used in driver code without checking for NULL**

```c
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);
```

The code checks `if (env)` after `getenv()`, which is correct. However, the symbolic constants `DRIVER_STRICT_ORDER` and `DRIVER_DUMP_MODE` are not defined in this patch. If these are new macros, they should be defined in the header. If they are existing macros whose names differ from the devarg strings, this could cause silent failures.

**Suggested fix:**
Verify that `DRIVER_STRICT_ORDER` and `DRIVER_DUMP_MODE` are defined (or define them if they are new):
```c
#define DRIVER_STRICT_ORDER "drv_strict_order"
#define DRIVER_DUMP_MODE "drv_dump_mode"
```
If these macros already exist with different string values, reconcile them with the devarg key names.

**Logic error: devargs processing return value misinterpreted**

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

`rte_kvargs_process()` returns 0 on success and negative on error. The code returns early if `ret == 0` (success), meaning the `env_set` fallback is **only** reached when devargs parsing **fails**. This is backwards. The fallback should be reached when devargs are **absent or do not contain the key**, not when parsing fails.

**Suggested fix:**
The early return at the top of the function (`if (!devargs) goto env_set;`) and the `rte_kvargs_count()` check already handle the "devarg not present" case correctly. Remove the `if (!ret) return;` line - the environment variable fallback should never be reached if devargs were successfully parsed. Alternatively, invert the condition if the intent is to skip the fallback on success:
```c
ret = rte_kvargs_process(kvlist, key,
		check_devargs_handler, (void *)cryptodev);
rte_kvargs_free(kvlist);
if (ret == 0)  /* devargs successfully set the value */
	return;
/* else: fall through to env_set if parsing failed */
```
However, the current logic is confusing - typically you want to use the fallback **only** when devargs are absent, not when parsing fails.

**Documentation update incomplete**

The RST documentation shows example usage but does not specify:
- Whether the environment variable takes precedence over devargs or vice versa
- What happens if both are set
- Valid range of values for `drv_dump_mode` (0, 1, 2, etc.)

**Suggested fix:**
Clarify in `dpaa2_sec.rst` that environment variables are a **fallback** used only when devargs are not provided, and document the valid range for `drv_dump_mode`.

### Warnings

**Missing release notes entry**

This adds a new user-facing feature (environment variable configuration). It should be documented in the "New Features" section.

---

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

### Errors

**None** - the implementation correctly uses the GCM shared descriptor for auth-only GMAC, maps GMAC to the hardware protocol word for IPsec, and adds the capability entry.

### Warnings

**Missing release notes entry**

This adds support for a new algorithm (`RTE_CRYPTO_AUTH_AES_GMAC`). It should be documented in the "New Features" section of the release notes.

**Potential memset inefficiency (Info-level)**

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

These are stack structures that are fully initialized before use in all code paths. The `memset()` may be unnecessary overhead. However, this is a control-path function (`dpaa2_sec_set_ipsec_session`) called during session setup, not a hot path, so the performance impact is negligible. This is good defensive programming to avoid uninitialized field bugs if the structures grow in the future.

---

## Summary

### Errors (must fix)
1. **Patch 1**: Buffer overflow fix may be incomplete - only initializes `sge+4` but commit message implies `sge[8]` is used. Verify SGE layout and initialize all used indices.
2. **Patch 1**: Missing release notes entry (buffer overflow fix).
3. **Patch 5**: Undefined macros `DRIVER_STRICT_ORDER` and `DRIVER_DUMP_MODE` - verify they exist or define them.
4. **Patch 5**: Logic error - `if (!ret) return;` after `rte_kvargs_process()` skips fallback on success instead of on presence of devargs.

### Warnings (should fix)
1. **Patch 2**: Missing release notes entry (FLE pool leak fix).
2. **Patch 3**: Missing release notes entry (AES-CTR IV size range change).
3. **Patch 4**: Missing release notes entry (ECN capability addition).
4. **Patch 5**: Documentation incomplete (precedence rules, valid values not specified).
5. **Patch 5**: Missing release notes entry (environment variable feature).
6. **Patch 6**: Missing release notes entry (AES-GMAC support).

### Info
- Patch 5 uses `getenv()` but correctly checks for NULL before dereferencing.
- Patch 6 uses defensive `memset()` on stack structures in a non-hot-path function, which is acceptable.