|WARNING| examples: add Wycheproof validation app

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: Wycheproof Validation Example

## Summary

This patch adds a new example application for validating cryptodev PMDs against Google Wycheproof JSON test vectors. The implementation is comprehensive, covering AEAD, MAC, and asymmetric crypto operations. Below are the findings organized by severity.

---

## Errors (Must Fix)

### 1. Resource Leak on Error Path - `app_init()`

**Location:** `app_init()`, lines 187-275

The function allocates the hash device (configures, sets up queue pair, starts) when the target device doesn't support hashing, but the error path at label `error:` does not clean up this hash device if a later allocation fails.

```c
/* After line 265, if any subsequent allocation fails (e.g., op_pool creation),
 * we goto error: which frees pools but does not stop/close env.hash_dev_id
 * when env.hash_dev_own is true.
 */
```

**Fix:** Add hash device cleanup to the error path:

```c
error:
	if (env.hash_dev_own) {
		rte_cryptodev_stop(env.hash_dev_id);
		rte_cryptodev_close(env.hash_dev_id);
	}
	rte_mempool_free(env.asym_op_pool);
	/* ... rest of cleanup ... */
```

---

### 2. Variable Overwrite Before Read - `compute_hash()`

**Location:** `compute_hash()`, lines 1026-1094

The function initializes `ret = -ENOMEM` at line 1043, then unconditionally overwrites it at line 1087 (`ret = -EIO`) when `completed->status != RTE_CRYPTO_OP_STATUS_SUCCESS`, without ever reading the initial value. This is a dead store.

```c
int ret = -ENOMEM;
/* ... allocations ... */
if (completed->status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
	ret = -EIO;   /* unconditional overwrite, initial -ENOMEM never used */
	goto out;
}
```

**Fix:** Remove the initialization and assign -ENOMEM only when actually returning that error:

```c
int ret;

/* ... */
session = rte_cryptodev_sym_session_create(env.hash_dev_id, &xform, env.session_pool);
if (session == NULL) {
	ret = -ENOMEM;
	goto out;
}
/* ... similarly for other allocations ... */
```

---

### 3. Statistics Accumulation Using `=` Instead of `+=`

**Location:** Multiple process functions accumulate stats, but in `process_hmac()` and similar functions, the pattern `stats->passed++` and `stats->failed++` is correct (uses increment). However, verify that all stat updates use `++` or `+=`, not `=`.

**Review:** After checking the code, all statistics use `++` correctly. This is **not an error** - no action needed.

---

## Warnings (Should Fix)

### 1. Missing Release Notes

**Finding:** This patch adds a new example application but does not update the release notes to document the new feature.

**Fix:** Add an entry to `doc/guides/rel_notes/release_26_11.rst` (or the appropriate release version):

```rst
New Features
------------

* **Added Wycheproof validation example.**

  Added ``dpdk-wycheproof_validation`` example application for validating
  cryptodev PMDs against Google Wycheproof JSON test vectors. Supports AEAD
  (AES-GCM, AES-CCM, ChaCha20-Poly1305, SM4-GCM), MAC (HMAC-SHA, HMAC-SHA3,
  HMAC-SM3, AES-CMAC, AES-GMAC), and asymmetric crypto (DSA, ECDSA, ECDH).
```

---

### 2. Non-Constant Function Pointer Arrays

**Finding:** None found. The code does not contain mutable function pointer arrays declared at file scope.

---

### 3. Use of `rte_malloc()` for Queue Structures

**Finding:** The application uses `rte_malloc()` for various buffers (AAD, digests, output), which is correct for DMA-accessible memory. Queue descriptors are not allocated by this application (they are internal to the cryptodev PMD), so this warning does not apply.

---

### 4. `bool` vs `int` for Boolean Variables

**Finding:** The code correctly uses `bool` for `env.debug` and `env.hash_dev_own`. The `have_device` variable in `parse_args()` also uses `bool` correctly. No issues.

---

### 5. RST Documentation - Definition Lists vs Bullet Lists

**Location:** `README.md`, table at line 10

The README uses a Markdown table for the Wycheproof-to-DPDK mapping, which is appropriate for that format. However, DPDK documentation should be in RST, not Markdown.

**Fix:** Convert `README.md` to `README.rst` (or move content to `doc/guides/sample_app_ug/wycheproof_validation.rst`) and use proper RST formatting:

```rst
Wycheproof to DPDK AEAD Mapping
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

key
   AEAD key

iv
   AEAD IV

aad
   AEAD associated data

msg
   Encryption input or expected decryption output

ct
   Expected encryption output or decryption input

tag
   Expected encryption tag or decryption digest
```

---

## Info / Style Issues

### 1. Hardcoded Overhead Constant - Not Applicable

This application does not deal with MTU or Ethernet frame overhead. No issue.

---

### 2. Explicit Comparisons

**Finding:** The code correctly uses explicit comparisons throughout:
- `if (session == NULL)` (line 585, 1115, etc.)
- `if (capability == NULL)` (line 553)
- `if (vector->key_len == 0)` (line 355)

All pointer and integer comparisons are explicit. No issues.

---

### 3. `volatile` Usage

**Finding:** No use of `volatile` in the code. No issues.

---

### 4. Shared Variable Access

**Finding:** The application is single-threaded (single lcore, synchronous operation). The `env` global is not shared between threads, and all crypto operations are enqueue/dequeue within the same lcore. No atomic operations needed.

---

### 5. `memcmp()` on Secret Data

**Finding:** The application uses `memcmp()` to compare:
- Ciphertexts and digests (lines 754, 755)
- Generated tags (lines 1008, 1222)
- Shared secrets (line 1475)

**Concern:** For **digest/tag/MAC verification** (lines 755, 1008, 1222), `memcmp()` leaks timing information and should use `rte_memeq_timingsafe()`.

**However:** The application is a **test harness**, not production crypto code. It is explicitly validating test vectors and needs to detect any byte mismatch to report failures. Timing-safe comparison is not required for test validation tools.

**Conclusion:** This is acceptable for an example/test application. Do not flag as an error.

---

### 6. Error Handling - Asymmetric Pool Creation

**Location:** Lines 223-224

The comment says asymmetric pools are "best-effort," and the code does not check if the allocations succeeded. Later, functions gate on `env.asym_session_pool == NULL` (line 1098, 1281, 1420). This is correct defensive coding for optional features.

---

### 7. Command-Line Argument Validation

**Finding:** `parse_args()` correctly validates all arguments:
- Checks device ID validity (line 135)
- Validates numeric parsing with overflow checks (line 98, 147)
- Requires `--vectors` to be present (line 161)

No issues.

---

### 8. Jansson Dependency Check

**Finding:** The meson.build correctly checks for Jansson and skips the build if not found (lines 11-13). The Makefile also checks (lines 12-14). Proper dependency handling.

---

### 9. Makefile Trailing Newline

**Location:** `Makefile`, line 40

The file does not end with a newline character (the diff shows `\ No newline at end of file`).

**Fix:** Add a newline at the end of the Makefile.

---

### 10. `strncmp()` Return Value Check

**Location:** `process_file()`, lines 1719-1724

The code uses `strncmp(algorithm, "AES-", strlen("AES-")) == 0` to match prefixes. This is correct usage.

---

### 11. Directory Traversal

**Location:** `process_path()`, lines 1753-1765

The code constructs file paths with `snprintf()` and checks for overflow (line 1761). The directory read loop correctly filters for `.json` files. No path traversal vulnerability.

---

### 12. Unused Variable

**Finding:** No unused variables detected by inspection.

---

---

## Final Recommendations

### Must Fix (Errors)
1. Add hash device cleanup to `app_init()` error path
2. Remove dead store initialization of `ret` in `compute_hash()`

### Should Fix (Warnings)
1. Add release notes entry
2. Convert README.md to RST or move to `doc/guides/sample_app_ug/`
3. Add trailing newline to Makefile

### Optional (Info)
- Consider adding a `--help` option that prints usage
- The README could document the `--mbuf-dataroom` option (currently undocumented)

---

## Overall Assessment

This is a **well-structured and thorough example application**. The code demonstrates good understanding of DPDK crypto API patterns, handles error paths correctly (aside from the hash device cleanup omission), and properly validates test vectors. The implementation is production-quality aside from the noted issues.

The most critical fix is the resource leak on the error path in `app_init()`. The other issues are minor and do not affect correctness.