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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: Wycheproof Validation Example

## Summary

This patch adds a new example application for validating cryptodev PMDs against Google Wycheproof test vectors. The review identifies several **correctness bugs** related to resource management, error handling, and memory operations, along with style and documentation issues.

---

## ERRORS (Must Fix)

### Correctness Bugs

1. **Resource leak on error path in `run_aead()`** (line 595-653)
   - When `rte_cryptodev_enqueue_burst()` fails, the function sets `ret = -EIO` and jumps to `out:`, but `operation_op` is still owned by the failed enqueue and should not be freed.
   - However, the code frees it anyway at `out:` via `rte_crypto_op_free(operation_op)`.
   - **Fix**: Set `operation_op = NULL` after a failed enqueue so the cleanup path skips the free:
     ```c
     if (rte_cryptodev_enqueue_burst(env.dev_id, 0, &operation_op, 1) != 1) {
         ret = -EIO;
         operation_op = NULL;  /* op ownership unclear after enqueue failure */
         goto out;
     }
     ```
   - Same pattern in `run_hmac()`, `run_gmac()`, `compute_hash()`, `run_dsa_verify()`, `run_ecdh_ecpoint()`, `run_ecdsa_verify()`.

2. **Missing error check on `rte_pktmbuf_append()` in multiple functions**
   - `rte_pktmbuf_append()` returns `NULL` if there is insufficient tailroom, but the code unconditionally dereferences the result with `memcpy()`.
   - Locations: `run_aead()` line 621, `run_hmac()` line 713, `run_gmac()` line 774, `compute_hash()` line 1128.
   - **Fix**: Check the return value before `memcpy()`:
     ```c
     uint8_t *data = rte_pktmbuf_append(mbuf, input_len);
     if (data == NULL) {
         ret = -EMSGSIZE;
         goto out;
     }
     memcpy(data, input, input_len);
     ```

3. **Memory allocation not checked for alignment requirement in `run_aead()`** (line 633)
   - `*digest = rte_malloc(NULL, vector->tag_len, RTE_CACHE_LINE_SIZE);`
   - `rte_malloc()` can return `NULL` on failure, but the code only checks `*digest == NULL && vector->tag_len != 0`.
   - When `tag_len == 0`, the code proceeds without allocating, then uses `*digest` at line 651 (`sym_op->aead.digest.data = *digest;`), passing uninitialized pointer to PMD.
   - **Fix**: Initialize `*digest = NULL` at declaration and verify it is either `NULL` (when `tag_len == 0`) or valid allocated memory before use.

4. **Use-after-free potential in asymmetric session cleanup**
   - In `run_dsa_verify()`, `run_ecdh_ecpoint()`, and `run_ecdsa_verify()`, the code calls `rte_cryptodev_asym_session_free(env.dev_id, session)` at the `out:` label.
   - However, if `rte_cryptodev_asym_session_create()` returns `< 0` but `session != NULL` (partial initialization), the free may access stale session data.
   - **Fix**: Check `session != NULL` before calling `asym_session_free()`, or ensure the create API guarantees `session` is `NULL` on failure.

5. **Missing `closedir()` on early return in `process_path()`** (line 1948)
   - If `snprintf()` fails the length check, the function returns `-ENAMETOOLONG` without closing `directory`.
   - **Fix**:
     ```c
     if (snprintf(...) >= (int)sizeof(file_path)) {
         ret = -ENAMETOOLONG;
         closedir(directory);
         return ret;
     }
     ```

6. **Session-create failure treated as PMD failure instead of capability check** (multiple locations)
   - In `run_ecdh_ecpoint()` line 1584 and `run_ecdsa_verify()` line 1807, after the capability check passes, the code treats a `session_create()` failure as `-EIO` (PMD failure).
   - This causes the tool to report false failures when the PMD legitimately lacks support for specific parameters.
   - **Fix**: Return `-ENOTSUP` instead of `-EIO` so the caller can skip the vector appropriately.

7. **Integer overflow in signature length check** (line 1331)
   - `if ((sig_len & 1) != 0 || sig_len == 0)` rejects odd-length or zero-length signatures, but does not check for overflow when dividing: `sig_len / 2` is used later without verifying `sig_len <= UINT32_MAX / 2`.
   - If `sig_len` is near `UINT32_MAX`, the division could wrap.
   - **Fix**: Add explicit check: `if (sig_len == 0 || (sig_len & 1) != 0 || sig_len > UINT32_MAX - 1) { ... }` (though in practice `sig_len` is bounded by decode_hex buffer size, making this low-risk).

8. **`memcmp()` used for digest comparison (timing side channel)** (line 896)
   - `memcmp(digest, vector.tag, vector.tag_len)` in `process_hmac()` leaks timing information about how many prefix bytes of the tag match.
   - For MAC verification, use `rte_memeq_timingsafe()` to prevent timing attacks.
   - **Fix**:
     ```c
     if (!rte_memeq_timingsafe(digest, vector.tag, vector.tag_len)) {
         debug_vector("FAIL", name, &vector, "generated tag mismatch");
         stats->failed++;
     }
     ```
   - Same issue in `validate_aead_vector()` lines 822-823, `process_gmac()` line 953.

---

## WARNINGS (Should Fix)

### Release Notes and Documentation

1. **Release notes entry incomplete**
   - The release notes mention "AEAD, MAC, DSA, ECDH and ECDSA algorithm families" but do not list the specific algorithms (AES-GCM, AES-CCM, ChaCha20-Poly1305, HMAC-SHA*, AES-CMAC, AES-GMAC, DSA P1363, ECDH ecpoint, ECDSA P1363).
   - **Suggestion**: Expand the list to match the commit message detail, or state "covering commonly-used AEAD, MAC, and asymmetric algorithms."

2. **Documentation does not explain exit code semantics**
   - The doc states "exit code is nonzero when a parsed and supported vector fails validation" but does not clarify what happens when all vectors are skipped (e.g., PMD supports none of the algorithms in the file).
   - **Suggestion**: Add a note in `wycheproof_validation.rst` explaining that success means all *supported* vectors passed, not that all vectors were executed.

### Style and Formatting

3. **`bool` preferred over `int` for `have_device` flag** (line 132)
   - The variable `have_device` is only ever assigned `true` or checked in boolean context, but is declared as `int` in older code style.
   - Per the guidelines, this should be `bool` for clarity.

4. **Incorrect use of `rte_malloc()` for general allocations** (line 373, 382, etc.)
   - The patch allocates multiple small structures (`vector->key`, `vector->iv`, `decoded`) with `rte_malloc()`, which allocates from hugepage memory.
   - Hugepage memory should only be used for DMA-visible buffers or shared memory.
   - **Suggestion**: Use `malloc()` for non-DMA vector data (`vector->key`, `vector->iv`, `vector->aad`, `vector->msg`, `vector->ct`, `vector->tag`). Only use `rte_malloc()` for PMD-visible buffers (`*digest`, `aad` in CCM, asymmetric operation fields).

5. **`volatile` would be incorrect if inter-thread access were added**
   - The current code has no inter-thread shared state, but if future changes add polling threads, developers might incorrectly use `volatile` for `stats` counters.
   - This is not a current error, but the review notes that atomics would be required if threading were introduced.

6. **Missing session pool sizing comment is unclear** (line 207)
   - The comment "The hashing device may differ from the target device, so size the session pool for the largest sym session across all devices" is placed inside a block scope with a loop variable `d`.
   - The scope is unnecessary and the comment could be clearer.
   - **Suggestion**: Move comment outside the block and clarify: "Session pool must accommodate the largest session size across all devices (target device plus optional separate hash device for DSA/ECDSA)."

---

## INFO (Consider)

### Alternative Approaches

1. **`dequeue_one()` timeout could be configurable**
   - The 1-second timeout in `dequeue_one()` is hardcoded. For hardware devices with high latency, this might be too short; for software devices, it might be too long.
   - **Consideration**: Add a `--timeout` option or document the timeout value in the usage guide.

2. **Asymmetric session creation error messages**
   - When `rte_cryptodev_asym_session_create()` fails after the capability check passes, the error is silent (returned as `-EIO` or `-ENOMEM`).
   - **Consideration**: Add a debug message explaining the failure to aid PMD developers.

3. **Path traversal in `process_path()` is basic**
   - The code only processes `.json` files in the top-level directory; it does not recurse into subdirectories.
   - Wycheproof vectors are typically organized in subdirectories (e.g., `aes_gcm_test.json` in `testvectors/`).
   - **Consideration**: Document this limitation or add recursive directory traversal.

4. **`memset(vector, 0, sizeof(*vector))` in `parse_vector()`**
   - The vector is zeroed before parsing, but if parsing fails partway through (e.g., `decode_hex()` fails on the 3rd field), the cleanup at `error:` calls `free_vector()` which attempts to free uninitialized pointers.
   - This is actually correct because `rte_free(NULL)` is safe, but a comment explaining this would improve readability.

---

## Summary of Key Findings

**Correctness bugs (highest priority):**
- Resource leaks on enqueue failure (multiple functions)
- Missing NULL checks on `rte_pktmbuf_append()`
- Timing side channel in digest comparison (use `rte_memeq_timingsafe()`)
- Missing `closedir()` on error path
- Session-create failure mishandled as PMD error instead of capability skip

**Style issues:**
- Inappropriate use of `rte_malloc()` for non-DMA data
- `bool` preferred over `int` for boolean flags

**Documentation:**
- Release notes could be more specific
- Exit code semantics unclear for all-skipped scenarios

The patch is well-structured overall, but the resource management bugs must be fixed before merging, and the timing side channel in MAC comparison is a security concern.