|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: examples/wycheproof_validation

## Summary

This patch adds a new example application that validates DPDK cryptodev PMDs against Google Wycheproof JSON test vectors. The implementation includes support for AEAD algorithms (AES-GCM, AES-CCM, SM4-GCM, ChaCha20-Poly1305), MAC algorithms (HMAC variants, AES-CMAC, AES-GMAC), and asymmetric algorithms (DSA, ECDSA, ECDH).

## Errors

### Resource Leak on Error Path

**Location:** `app_init()` (lines 259-282)

The function creates multiple mempools but does not clean up all resources when a later step fails. For example:

```c
env.asym_session_pool = rte_cryptodev_asym_session_pool_create(...);
env.asym_op_pool = rte_crypto_op_pool_create(...);

queue_pair.mp_session = env.session_pool;
ret = rte_cryptodev_queue_pair_setup(env.dev_id, 0, &queue_pair, ...);
if (ret < 0)
    goto error;  /* asym pools not freed */

ret = rte_cryptodev_start(env.dev_id);
if (ret < 0)
    goto error;  /* asym pools still not freed */
```

The `error:` label at line 318 only frees the pools that were allocated before the `asym_session_pool` was added. The asymmetric pools (lines 273-277) are leaked if any step after their creation fails.

**Fix:** Adjust the error cleanup to include the asymmetric pools:

```c
error:
    rte_mempool_free(env.asym_op_pool);
    rte_mempool_free(env.asym_session_pool);
    rte_mempool_free(env.op_pool);
    /* ... existing cleanup ... */
```

---

### Resource Leak in Hash Device Setup

**Location:** `app_init()` secondary hash device loop (lines 286-313)

When setting up a separate hash device, if `rte_cryptodev_queue_pair_setup()` or `rte_cryptodev_start()` fails, the function calls `rte_cryptodev_close(d)` and continues searching. However, if `rte_cryptodev_configure()` succeeds but the subsequent queue setup fails, the device remains configured but not started, and the continue skips cleanup of any intermediate state.

While `rte_cryptodev_close()` is called on the queue-setup-or-start failure path, the `continue` inside the loop means that if *all* devices fail this way, we exit the loop without recording a usable hash device. Later code that calls `compute_hash()` will use `env.hash_dev_id`, which may point to a device that was configured but not successfully started.

**Fix:** After the loop completes, verify that `env.hash_dev_own` is true or that `env.hash_dev_id` points to a device with the SHA256 capability. If no suitable device was found, document that DSA/ECDSA paths will fail at runtime (which is acceptable given the comment "Digest computation for DSA/ECDSA needs..."), or return an error if DSA/ECDSA support is required.

Alternatively, ensure that if a device is partially configured (configure succeeded, queue setup failed), the `rte_cryptodev_close()` call properly cleans it up. The current code assumes `rte_cryptodev_close()` is sufficient, which is likely correct, but the logic should be verified or commented.

---

### Incorrectly Split P1363 DSA Signature

**Location:** `process_dsa_p1363()` (lines 1351-1353)

```c
ret = run_dsa_verify(&dsa, vector.msg, vector.msg_len, sig, sig_len / 2,
    sig + sig_len / 2, sig_len / 2, &status);
```

The comment at line 1341 checks for even signature length: `if ((sig_len & 1) != 0 || sig_len == 0)`. However, the P1363 DSA signature format is `r || s`, where each component is a fixed-width big-endian integer. The code assumes that both `r` and `s` are `sig_len / 2` bytes. This is correct for standard P1363 encoding where `sig_len = 2 * len(q)`.

However, if the signature length is not exactly twice the expected component length (e.g., due to malformed test data that passed the even-length check), this could result in passing incorrect component lengths to `run_dsa_verify()`.

**Analysis:** The code does check for even length and non-zero length at line 1341. For a well-formed P1363 signature, `sig_len = 2 * component_length`, so `sig_len / 2` is correct for each component. The check is adequate given the P1363 format constraint.

**Correction:** The code is actually correct. The even-length check ensures `sig_len / 2` is an integer, and the P1363 format guarantees equal-length `r` and `s`. No error here. (Removed per final check.)

---

### ECDH Shared Secret Memory Not Zeroed Before Free

**Location:** `run_ecdh_ecpoint()` (lines 1411-1472)

The function allocates `out_x` and `out_y` (line 1442-1443) to receive the computed shared secret point. On success, `out_x` is returned to the caller via `*shared_x`, but on error or when the operation fails, `out_x` and `out_y` are freed without zeroing (lines 1467-1468).

**Cryptographic secrets must be wiped before free.** The shared secret `x` coordinate is sensitive material. Even on an operation failure, the buffers may contain partial results or uninitialized data that could leak information.

**Fix:** Use `rte_free_sensitive()` instead of `rte_free()` for `out_x` and `out_y`:

```c
out:
    rte_free_sensitive(out_x);
    rte_free_sensitive(out_y);
```

Note: The caller also needs to use `rte_free_sensitive()` when freeing `result_x` after comparison (line 1558 in `process_ecdh_ecpoint()`).

---

### ECDH and ECDSA Session Leaks on Early Return

**Location:** `run_ecdh_ecpoint()` and `run_ecdsa_verify()` (lines 1447-1450, 1645-1648)

Both functions have this pattern:

```c
if (rte_cryptodev_asym_session_create(..., &session) < 0 || session == NULL) {
    ret = -ENOTSUP;
    goto out;
}
```

If `rte_cryptodev_asym_session_create()` returns `< 0`, the session pointer's value is undefined (it may not have been written to). The code then jumps to `out:`, which calls `rte_cryptodev_asym_session_free(env.dev_id, session)` unconditionally if `session != NULL`.

If the session pointer was not initialized to NULL before the create call, and the create fails, the code may pass an uninitialized pointer to `rte_cryptodev_asym_session_free()`.

**However:** Both functions initialize `session = NULL` in their local variable declarations (lines 1405, 1602). So the pointer is always initialized to NULL, and the `if (session != NULL)` check in the `out:` label (lines 1471, 1678) protects against freeing an invalid pointer.

**Correction:** The code is actually correct. The session pointers are initialized to NULL, so the check at the cleanup label is safe. No error here. (Removed per final check.)

---

### Use-After-Free Risk in Digest Computation

**Location:** `compute_hash()` (lines 1243-1302)

The function allocates `op_digest` (line 1277), attaches it to the crypto op, and enqueues the op. After dequeue, it copies the digest to the caller's buffer (line 1293) and then frees `op_digest` (line 1297).

**However:** The `rte_crypto_op_free(op)` at line 1299 may internally reference the digest buffer if the op still holds a pointer to it. While the DPDK crypto op API typically does not retain pointers after dequeue, the safest pattern is to free the op *before* freeing any attached buffers.

**Analysis:** Reviewing the DPDK crypto op lifecycle: after `rte_cryptodev_dequeue_burst()`, the op is detached from the device and its buffers should no longer be accessed by hardware. Freeing `op_digest` before freeing the op is safe as long as the op itself does not access the digest in its destructor. Given that `rte_crypto_op_free()` is a simple pool return, this is likely safe.

**Correction:** The code is acceptable. The op is dequeued and the digest data is copied out before freeing the digest buffer. The op free is a pool return and does not access the digest. No error here. (Removed per final check.)

---

## Warnings

### Missing Release Notes

**Location:** Documentation (patch header)

The patch adds a new example application, which is a significant user-facing change. Per the guidelines, "New drivers or subsystems must have release notes." A new example under `examples/` qualifies as a user-visible addition that should be documented in the current release notes.

**Fix:** Add an entry to `doc/guides/rel_notes/release_*.rst` (the current release file) under a "New Features" or "Examples" section:

```rst
* **Added Wycheproof cryptodev validation example.**

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

---

### Hardcoded DPDK Release Date in Future

**Location:** Patch header (Date: Tue, 15 Sep 2026)

The patch date is "2026-09-15", which is in the future relative to the current date (2026-09-15 per the system prompt). This suggests the patch is either misdated or is part of a forward-looking test corpus. Future dates in patches can confuse tooling and reviewers.

**Fix:** Ensure the patch date reflects the actual submission date. If this is a forward-looking example for testing purposes, document that in the commit message or cover letter.

---

### Asymmetric Pools Best-Effort Comment

**Location:** `app_init()` lines 279-281

The comment states "Asymmetric pools are best-effort; DSA/ECDSA paths gate on the device asymmetric feature flag at runtime." This is a reasonable approach for an example, but the code should ensure that if the pools fail to allocate, the application does not crash when a user tries to run DSA/ECDSA vectors.

**Current behavior:** The code checks for `env.asym_session_pool == NULL` and `env.asym_op_pool == NULL` in `run_dsa_verify()`, `run_ecdh_ecpoint()`, and `run_ecdsa_verify()`, returning `-ENOTSUP`. This is correct.

**Recommendation:** Add a user-facing message when the asymmetric pools fail to allocate, so users understand why DSA/ECDSA vectors are being skipped. For example:

```c
if (env.asym_session_pool == NULL || env.asym_op_pool == NULL) {
    if (env.debug)
        printf("Asymmetric session/op pools not available; "
               "DSA/ECDSA/ECDH vectors will be skipped\n");
}
```

---

### Makefile Missing Newline at EOF

**Location:** `examples/wycheproof_validation/Makefile` (line 40)

The Makefile ends with `test -d build && rmdir -p build || true` without a trailing newline. POSIX text files should end with a newline.

**Fix:** Add a newline after the last line of the Makefile.

---

### Debug Output for Empty Shared Secret

**Location:** `process_ecdh_ecpoint()` (lines 1540-1544)

```c
if (shared_len == 0) {
    debug_vector("SKIP", name, &vector,
        "no expected shared secret (point validation out of scope)");
    stats->skipped_unsupported++;
    goto next;
}
```

The debug message is clear, but the logic implies that vectors with `shared_len == 0` are silently skipped even when `--debug` is not specified. Users running without `--debug` will see these vectors counted in `skipped_unsupported` but won't know why.

**Recommendation:** This is acceptable for an example, but consider logging a summary at the end: "X vectors skipped due to out-of-scope operations (invalid-curve ECDH, etc.)." Alternatively, document this behavior in the README.

**Current README coverage:** The README states "Invalid-curve vectors (empty `shared`) are skipped because on-curve validation is a separate ECDH `PUB_KEY_VERIFY` op..." This is documented, so the behavior is acceptable.

---

### README: RST Definition Lists

**Location:** `examples/wycheproof_validation/README.md` (lines 10-19)

The README uses a Markdown table for the Wycheproof-to-DPDK mapping. If this documentation is converted to RST for inclusion in the DPDK docs, the table format should use RST syntax. Additionally, the list of supported algorithms (lines 5-9) is formatted as a Markdown list with inline code.

If this file will be processed by Sphinx, consider using RST definition lists for clearer structure:

```rst
The initial implementation supports:

AES-GCM, AES-CCM, CHACHA20-POLY1305, SM4-GCM
   AEAD algorithms using ``aead_test_schema_v1.json``.

HMAC-SHA1/224/256/384/512, HMAC-SHA3-*, HMAC-SM3, AES-CMAC
   MAC algorithms using ``mac_test_schema_v1.json``.
```

**Current status:** The file is `.md`, so Markdown syntax is correct. However, if integrated into DPDK docs (under `doc/guides/sample_app_ug/`), it should be converted to RST with definition lists per the guidelines.

---

## Info

### Makefile Standalone Build

**Location:** `examples/wycheproof_validation/Makefile`

The Makefile checks for `pkg-config` availability of `libdpdk` and `jansson` (lines 8-14) and provides both shared and static build targets. This is a good pattern for standalone examples.

**Observation:** The `PKGCONF ?= pkg-config` on line 6 allows users to override the tool, which is useful for cross-compilation. Well done.

---

### Wycheproof Schema Recognition

**Location:** `process_file()` (lines 1713-1775)

The dispatcher recognizes multiple Wycheproof schemas and algorithms, including those not yet implemented (AES-CBC-PKCS5, AES-EAX, RSA, etc.). The code explicitly logs these as "recognized-but-unsupported" rather than silently folding them into the generic "unsupported" count.

**Comment:** This is excellent design. It helps users understand which algorithms are on the roadmap versus which are unrecognized test files.

---

### Debug Output for Malformed Vectors

**Location:** Various `process_*` functions (e.g., lines 995-998, 1164-1166)

When a vector fails to parse (invalid JSON structure, missing required fields), the code uses `env.debug` to log "SKIP ... malformed vector" and increments `skipped_unsupported`. This prevents the application from crashing on corrupt or unexpected test data.

**Comment:** Good defensive programming. The debug logs provide actionable information when troubleshooting test corpus issues.

---

### ECDSA e Truncation

**Location:** `run_ecdsa_verify()` (lines 1630-1632)

```c
/* ECDSA