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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-13

# DPDK Wycheproof Validation Example - Code Review

## ERRORS

**Resource Leak on Error Path**

Line 218, `app_init()`:
```c
env.session_pool = rte_cryptodev_sym_session_pool_create("WYCHEPROOF_SESSION_POOL",
    64, session_size, 0, 0, rte_socket_id());
if (env.session_pool == NULL) {
    ret = -rte_errno;
    goto error;
}
```
The `error` label at line 310 frees `env.mbuf_pool` but does not free the session pools or op pools allocated after the mbuf pool. If session pool creation succeeds but a later allocation fails (e.g., `env.op_pool` creation fails), the session pool is leaked.

**Suggested fix**: The error path should free all resources allocated so far:
```c
error:
    rte_mempool_free(env.asym_op_pool);
    rte_mempool_free(env.asym_session_pool);
    rte_mempool_free(env.op_pool);
    rte_mempool_free(env.session_pool);
    rte_mempool_free(env.mbuf_pool);
    /* ... existing close ... */
```

---

**Non-constant-time digest comparison (timing side channel)**

Line 1947, `process_gmac()`:
```c
(vector.tag_len != 0 &&
 memcmp(digest, vector.tag, vector.tag_len) != 0))) ||
```
When verifying a MAC tag in `process_gmac()`, the code uses `memcmp()` to compare the generated tag against the expected tag. This is a timing side channel: an attacker can measure how long the comparison takes to learn information about the correct tag byte-by-byte.

**Suggested fix**: Replace `memcmp(digest, vector.tag, vector.tag_len)` with `!rte_memeq_timingsafe(digest, vector.tag, vector.tag_len)` in all MAC verification paths (GMAC at line 1947, HMAC at lines 1828 and 1842).

Note: The same issue appears in `process_hmac()` at lines 1828 and 1842. All three locations must be fixed.

---

**Key material not zeroed before free**

Line 348, `free_vector()`:
```c
static void
free_vector(struct wycheproof_data *vector)
{
    rte_free_sensitive(vector->key);
    rte_free(vector->iv);
    rte_free(vector->aad);
    rte_free(vector->msg);
    rte_free(vector->ct);
    rte_free(vector->tag);
    memset(vector, 0, sizeof(*vector));
}
```
The private key fields in asymmetric structures (`priv` in ECDH, line 2220) are freed with `rte_free_sensitive()`, which is correct. However, symmetric keys (`vector->key`) use `rte_free_sensitive()` but the digest buffer in `run_aead()` (line 699) and `run_hmac()` (line 783) uses plain `rte_free()`. While digest buffers are not keys, they are authentication tags that gate acceptance of attacker-supplied data and should be wiped.

**Suggested fix**: Replace `rte_free(digest)` with `rte_free_sensitive(digest)` at lines 699, 783, and similar locations where digest buffers are freed.

Actually, on re-examination: digest buffers are outputs (tags/MACs), not secret key material. They are public in a valid exchange. The current code correctly uses `rte_free_sensitive()` only for `vector->key` (which is the HMAC/CMAC/GMAC key or AEAD key). The digest/tag itself is not a secret. **Withdraw this finding.**

---

**Missing `rte_pause()` in spin loop**

Line 493, `dequeue_one()`:
```c
while (rte_cryptodev_dequeue_burst(dev_id, 0, &completed, 1) == 0) {
    if (rte_get_timer_cycles() > deadline)
        return NULL;
    rte_pause();
}
```
This is correct -- `rte_pause()` is present. **No issue.**

---

## WARNINGS

**Makefile enables experimental API without justification**

Line 24, `examples/crypto_validation/wycheproof_validation/Makefile`:
```c
CFLAGS += -O3 -DALLOW_EXPERIMENTAL_API $(shell $(PKGCONF) --cflags libdpdk jansson)
```
The Makefile adds `-DALLOW_EXPERIMENTAL_API` unconditionally. Per the guidelines, examples must not enable experimental API via build flags. The code uses `rte_cryptodev_asym_session_create()` and other asymmetric crypto APIs which are experimental, so the define is necessary, but it should not be in the Makefile for an example.

**Suggested fix**: Remove `-DALLOW_EXPERIMENTAL_API` from the Makefile. If the example genuinely requires experimental API, it should be documented why, and the usage should be gated or the example itself marked as requiring experimental features.

Actually, checking the code: the asymmetric API is only used if `env.asym_session_pool != NULL` (line 1636, 1969, 2083, 2323), which is allocated best-effort (line 233). The code does not require experimental API to compile -- it only uses it at runtime if available. However, the header files for `rte_cryptodev_asym_*` may require the define to declare the functions.

On further inspection: the code includes `<rte_cryptodev.h>` (line 19) which declares asymmetric functions under `#ifdef RTE_CRYPTODEV_EXPERIMENTAL_API`. If those functions are used, the define is required to compile. This is acceptable in an example that explicitly targets experimental features, **but** the Makefile is installing this in `examples/`, which per the guidelines should not use experimental API.

Given the usage is gated and the example is specifically for validation of crypto features (some of which are experimental), **demote to Warning**: the experimental API usage should be documented in the .rst file, and ideally the example should build and run (with reduced coverage) even without asymmetric support.

---

**Missing release notes for directory move**

Lines 5-22, MAINTAINERS and examples tree:
The patch moves `examples/fips_validation/` to `examples/crypto_validation/fips_validation/` but does not update the release notes to mention this directory reorganization. While the Wycheproof addition is documented, the FIPS move is an API-facing change (developers building FIPS validation will need to update their paths).

**Suggested fix**: Add a note in the release notes under a "Reorganization" or "Other Changes" section:
```
* **Reorganized crypto validation examples.**

  Moved ``examples/fips_validation`` to ``examples/crypto_validation/fips_validation``
  to group validation applications under a common directory.
```

---

**`bool` preferred over `int` for pure true/false variable**

Line 74, `struct app_env`:
```c
bool hash_dev_own;
```
This is correct -- the field is already `bool`. **No issue.**

---

**Documentation does not match code for ECDH behavior**

Line 43, `doc/guides/sample_app_ug/wycheproof_validation.rst`:
```
Asymmetric coverage (DSA, ECDH, ECDSA) requires a PMD that advertises the
corresponding asymmetric xform capabilities. The ``crypto_openssl`` PMD used
above does not advertise the ECDSA or ECDH xforms, so those vectors are skipped;
select a PMD that advertises them to exercise the asymmetric families.
```
The documentation states that ECDH vectors are skipped if the PMD does not advertise ECDH. However, the code at line 2234 in `process_ecdh_ecpoint()` skips vectors that have no expected shared secret with the reason "point validation out of scope":
```c
if (shared_len == 0) {
    debug_vector("SKIP", name, &vector,
        "no shared secret (point validation out of scope)");
    stats->skipped_unsupported++;
    goto next;
}
```
This is a valid design choice (ECDH shared-secret compute does not include point validation), but the documentation should mention this limitation explicitly.

**Suggested fix**: Add a note in the .rst under the Run section or a limitations section:
```
Note: ECDH vectors that test invalid-curve point rejection are skipped, as
the DPDK ECDH primitive performs shared-secret computation only and does not
validate that the peer's public key is on the curve. Such validation is
expected to be performed separately via the ECDH PUB_KEY_VERIFY operation.
```

---

## CLEAN ISSUES (Info)

**HMAC keyless hash comment could be clearer**

Line 1504, `compute_hash()`:
```c
/* Compute a plain (keyless) message digest through the cryptodev auth path. */
```
The comment is correct. The function sets `xform.auth.key.length = 0` at line 1524 (implicit zero-initialization). This is acceptable for plain hashing (SHA-1/SHA-2/SHA-3). No issue.

---

**CCM AAD offset constant is undocumented**

Line 32, `#define AES_CCM_AAD_OFFSET 18`:
The comment on line 31 explains the offset, but the value `18` is not derived in the code or comment. CCM's B0 block is 16 bytes, plus a 2-byte AAD length field = 18. This is correct but could be more explicit.

**Suggested improvement** (Info only):
```c
/* CCM AAD is prefixed with B0 block (16 bytes) + AAD length (2 bytes). */
#define AES_CCM_AAD_OFFSET (16 + 2)
```

---

## SUMMARY

The patch adds a comprehensive Wycheproof validation example for DPDK cryptodev. The code is generally well-structured and handles error paths carefully, with one significant resource leak, one timing side channel in MAC verification, and a Makefile experimental API usage that should be reconsidered for an example.

**Pass 1 (Correctness)**: Resource leak on session pool allocation failure, non-constant-time MAC comparison.

**Pass 2 (Style/Process)**: All items verified correct or demoted to Warning/Info.

**Pass 3 (Evidence)**: All findings quote specific lines.

Review-Result: ERROR