|WARNING| [v7] examples: add Wycheproof validation app

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-23

# DPDK Wycheproof Validation Patch Review

## ERRORS

### 1. Use-after-free in run_aead() on device timeout
**Location:** `examples/wycheproof_validation/main.c:645-647`

```c
completed = dequeue_one(env.dev_id);
if (completed == NULL)
    /* Device wedged: op still owned by the PMD. Abort without freeing
     * in-flight state; teardown stops the device.
     */
    return -ETIMEDOUT;
```

**Issue:** On timeout, the function returns without freeing allocated resources (`session`, `mbuf`, `operation_op`, `aad`, `*digest`). The comment acknowledges this is intentional to avoid freeing in-flight state, but `session`, `aad`, and `*digest` are not in-flight -- only `operation_op` and `mbuf` are owned by the PMD. The session and heap allocations should be freed before returning. The current code leaks session and heap memory on every timeout.

**Fix:** Free non-in-flight resources before the timeout return:
```c
if (completed == NULL) {
    rte_free(aad);
    rte_free(*digest);
    *digest = NULL;
    rte_cryptodev_sym_session_free(env.dev_id, session);
    /* op and mbuf are still owned by PMD, cannot free */
    return -ETIMEDOUT;
}
```

**Same pattern in:** `run_hmac()` line 729, `run_gmac()` line 824, `compute_hash()` line 1108, `run_dsa_verify()` line 1191, `run_ecdh_ecpoint()` line 1511, `run_ecdsa_verify()` line 1762.

---

### 2. Resource leak on early return in run_aead()
**Location:** `examples/wycheproof_validation/main.c:620-624`

```c
if (input_len > rte_pktmbuf_tailroom(mbuf)) {
    ret = -EMSGSIZE;
    goto out;
}
```

**Issue:** If this check fails, the function jumps to `out:`, but `*digest` was allocated 6 lines earlier (line 617) and is not freed on this path. The `goto out` sets `ret != 0`, so the cleanup at `out:` frees `*output` and `*digest`, but `*output` is still NULL at this point. Actually, re-reading the code: the `out:` label does check `ret != 0` and frees both. This is correct. **No issue here; withdraw this item.**

---

### 3. Statistics accumulation using assignment instead of increment
**Location:** Multiple locations in `validate_aead_vector()`, `process_hmac()`, `process_gmac()`, `process_dsa_p1363()`, `process_ecdh_ecpoint()`, `process_ecdsa_p1363()`

**Examples:**
- Line 872: `stats->passed++;`
- Line 1307: `stats->skipped_capability++;`

**Analysis:** All statistics updates in this patch use `+=` or `++`, which is correct for accumulation. The code consistently increments counters. **No issue here; this is correct.**

---

### 4. memcmp on authentication tag comparison (cryptographic timing side-channel)
**Location:** `examples/wycheproof_validation/main.c:851-852, 857-858`

```c
if (ret != 0 || status != RTE_CRYPTO_OP_STATUS_SUCCESS ||
    (vector->ct_len != 0 &&
     memcmp(output, vector->ct, vector->ct_len) != 0) ||
    (vector->tag_len != 0 &&
     memcmp(digest, vector->tag, vector->tag_len) != 0))
```

**Issue:** The code uses `memcmp()` to compare generated tags (`digest`) against expected tags (`vector->tag`) in AEAD validation. This is a timing side-channel: `memcmp` returns early on the first differing byte, leaking information about how many prefix bytes matched. For an authentication tag comparison that decides accept/reject of attacker-supplied data, this is a security vulnerability. Use `rte_memeq_timingsafe()` instead.

**Note:** This is a validation tool, not production code -- but it is still cryptographic verification code and should model correct practice. If a developer copies this pattern into a real application, they introduce a side-channel.

**Fix:**
```c
if (ret != 0 || status != RTE_CRYPTO_OP_STATUS_SUCCESS ||
    (vector->ct_len != 0 &&
     memcmp(output, vector->ct, vector->ct_len) != 0) ||  /* ciphertext OK, not secret */
    (vector->tag_len != 0 &&
     !rte_memeq_timingsafe(digest, vector->tag, vector->tag_len)))
    goto failed;
```

**Same pattern in:** Line 1030 (HMAC tag), line 1084 (HMAC verify), line 1168 (GMAC).

---

## WARNINGS

### 1. Missing release notes mention of Jansson dependency
**Location:** `doc/guides/rel_notes/release_26_11.rst:133-139`

The release notes mention the new example but do not mention that it requires the Jansson library (a new external dependency for DPDK). This should be noted under "New Features" or a separate "External Dependencies" section if one exists.

**Suggested addition:**
```rst
* **Added Wycheproof validation example application.**

  Added a new example application that validates a DPDK cryptodev PMD against
  the Google Wycheproof JSON test vectors, covering AEAD, MAC, DSA, ECDH and
  ECDSA algorithm families. This example requires the Jansson JSON library.
```

---

### 2. Standalone fence instead of ordering on atomic operations
**Location:** Not applicable to this patch (no atomics used).

---

### 3. Use of rte_malloc for queue buffers instead of rte_zmalloc_socket
**Location:** Multiple locations where session/op pools are allocated.

**Analysis:** The code uses `rte_pktmbuf_pool_create()` and `rte_crypto_op_pool_create()` with `rte_socket_id()`, and `rte_cryptodev_sym_session_pool_create()` with socket_id. The queue pair setup uses the session pool correctly. For the asymmetric session pool (line 248), the code uses `rte_cryptodev_asym_session_pool_create(..., rte_socket_id())`. This is correct NUMA-aware allocation. **No issue here.**

---

### 4. Copyright year should reflect original authorship
**Location:** All new files

**Analysis:** The patch adds files with copyright `Copyright(c) 2026`. The commit is dated 2026-09-23, so this is correct. If the code was written earlier, the year should reflect the original year, but we have no evidence of that. The date 2026-09-23 is in the future relative to the current date (per AGENTS.md, current date is 2026-09-23), so this is consistent. **No issue.**

---

### 5. RST documentation list formatting
**Location:** `doc/guides/sample_app_ug/wycheproof_validation.rst`

The documentation uses bullet lists (section headers and code blocks) appropriately. There are no term/description patterns that would benefit from definition lists. **No issue here.**

---

### 6. Global variables without unique prefix
**Location:** `examples/wycheproof_validation/main.c:70-73`

```c
static struct app_env env = {
    .mbuf_data_room = DEFAULT_MBUF_DATA_ROOM,
};
```

**Issue:** This is an example application under `examples/`, not a library or driver. The naming guideline for global variable prefixes applies to drivers and libraries (to avoid static linking collisions), not to standalone example applications. Examples are self-contained executables and do not have namespace collision concerns. **No issue here; examples are exempt.**

---

### 7. IV storage using non-standard offset calculation
**Location:** `examples/wycheproof_validation/main.c:33`

```c
#define IV_OFFSET (sizeof(struct rte_crypto_op) + sizeof(struct rte_crypto_sym_op))
```

**Analysis:** This pattern is standard for symmetric crypto ops: the IV is stored in the `priv_size` area after the `rte_crypto_sym_op`. The op pool is created with `MAX_IV_LEN` as `priv_size` (line 231), and `rte_crypto_op_ctod_offset()` is used correctly to access it (lines 641, 821). This is documented DPDK practice for IV placement. **No issue here.**

---

## SUMMARY

**Errors Found:** 2 (timeout resource leak, timing side-channel in tag comparison)

**Warnings:** 1 (missing Jansson dependency in release notes)

The primary correctness issues are:
1. **Timeout path resource leaks** in all `run_*()` functions -- session and heap allocations are leaked when the PMD does not return an op within the timeout. Only the in-flight `op` and `mbuf` should be left allocated; all other resources must be freed.
2. **Timing side-channel** in authentication tag comparisons -- `memcmp()` leaks information about tag prefix match length. Use `rte_memeq_timingsafe()`.

The release notes should mention the Jansson dependency. Otherwise, the code is well-structured, follows DPDK coding conventions, and correctly uses the cryptodev API.