|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 that validates cryptodev PMDs against Google Wycheproof test vectors. Overall code quality is good, but there are several correctness issues that must be addressed.

---

## ERRORS (Must Fix)

### 1. Resource Leak on Session Creation Failure (Line 585-590, 678-683, 752-757)

**Issue:** In `run_aead()`, `run_hmac()`, and `run_gmac()`, if `rte_cryptodev_sym_session_create()` fails, the function jumps to `out:` where it calls `rte_cryptodev_sym_session_free()` on a NULL session pointer. While `rte_cryptodev_sym_session_free()` may handle NULL gracefully, the allocated `op` and `mbuf` are leaked because the goto bypasses their cleanup.

```c
/* run_aead line 585 */
session = rte_cryptodev_sym_session_create(env.dev_id, &xform, env.session_pool);
if (session == NULL)
    goto out;  /* leaks op and mbuf allocated before this point */

operation_op = rte_crypto_op_alloc(env.op_pool, RTE_CRYPTO_OP_TYPE_SYMMETRIC);
if (operation_op == NULL)
    goto out;
```

**Fix:** Allocate the session first (before op/mbuf), or add explicit NULL checks in the cleanup section to handle partial allocation states correctly.

---

### 2. Use-After-Free in Timeout Path (Lines 635, 724, 804, 1037, 1401, 1739, 1930)

**Issue:** When `dequeue_one()` returns NULL (device wedged), the comment states "op still owned by the PMD. Abort without freeing in-flight state," but then the function returns `-ETIMEDOUT` **without** setting `op`, `mbuf`, `session`, etc. to NULL. The caller sees the timeout and continues, but subsequent error paths may still try to free these resources via the `out:` label, causing a potential double-free if the PMD eventually completes the operation asynchronously.

```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;  /* but 'op' is still set, not NULL */

out:
    rte_crypto_op_free(operation_op);  /* may double-free later */
```

**Fix:** When returning `-ETIMEDOUT`, set all in-flight pointers to NULL to prevent the cleanup path from freeing them:

```c
if (completed == NULL) {
    operation_op = NULL;
    mbuf = NULL;
    session = NULL;
    return -ETIMEDOUT;
}
```

Apply this pattern consistently across all crypto operation functions.

---

### 3. Buffer Overflow in CCM IV Handling (Line 616)

**Issue:** When `algorithm == RTE_CRYPTO_AEAD_AES_CCM`, the code writes the IV at offset `IV_OFFSET + 1` without verifying that `IV_OFFSET + 1 + vector->iv_len` fits within the allocated private area of the crypto op. The comment at line 227 states `priv_size` is `MAX_IV_LEN`, but the write occurs at `IV_OFFSET + 1`, and the validation at line 917 only checks `vector->iv_len + 1 <= MAX_IV_LEN`. If `IV_OFFSET` is large, this could write past the allocated buffer.

```c
/* Line 916: validation checks iv_len + 1 vs MAX_IV_LEN */
if (vector->iv_len + (algorithm == RTE_CRYPTO_AEAD_AES_CCM ? 1u : 0u) > MAX_IV_LEN ...)

/* Line 616: writes at IV_OFFSET + 1 without checking total offset */
memcpy(rte_crypto_op_ctod_offset(operation_op, uint8_t *, IV_OFFSET) + 1,
    vector->iv, vector->iv_len);
```

**Fix:** Verify that `IV_OFFSET + 1 + vector->iv_len <= MAX_IV_LEN` for CCM, or adjust the validation at line 916 to account for `IV_OFFSET`.

---

### 4. Missing Error Propagation in Hash Device Selection (Lines 265-276)

**Issue:** In `app_init()`, when searching for a hash-capable device, if `rte_cryptodev_configure()`, `rte_cryptodev_queue_pair_setup()`, or `rte_cryptodev_start()` fails, the code calls `rte_cryptodev_close(d)` but does **not** check if the close succeeded. More critically, if all devices fail to configure, the loop exits with `env.hash_dev_id` still equal to `env.dev_id` (which has no hash capability), and the code continues silently. Later DSA/ECDSA operations will fail when they try to compute hashes on a device that doesn't support auth.

**Fix:** After the loop, verify that a hash-capable device was actually found and configured:

```c
if (env.hash_dev_id == env.dev_id && 
    rte_cryptodev_sym_capability_get(env.dev_id, &hash_idx) == NULL) {
    fprintf(stderr, "No hash-capable device found for DSA/ECDSA digest computation\n");
    ret = -ENOTSUP;
    goto error;
}
```

---

### 5. Unbounded Loop in `dequeue_one()` (Line 508)

**Issue:** The `while` loop in `dequeue_one()` calls `rte_pause()` but does not yield or sleep, creating a tight busy-wait that can consume 100% CPU for up to 1 second per operation. While there is a timeout, this is unnecessarily wasteful.

**Fix:** This is marked as Error (not Warning) because on some systems, a tight busy-wait can starve other threads or even the device interrupt handler, causing the "device wedged" timeout to trigger spuriously on a device that is merely slow.

Replace `rte_pause()` with a brief sleep:

```c
while (rte_cryptodev_dequeue_burst(dev_id, 0, &completed, 1) == 0) {
    if (rte_get_timer_cycles() > deadline)
        return NULL;
    usleep(1000);  /* or rte_delay_us_sleep(1000) */
}
```

---

### 6. Integer Multiply Without Widening Cast (Line 1545)

**Issue:** 

```c
if (sig_len != 2 * ec.bytesize) {
```

`ec.bytesize` is `uint32_t`, and the literal `2` is `int`. The multiplication is performed at 32 bits, but since `sig_len` is also 32 bits, this is acceptable for values up to `UINT32_MAX / 2`. However, in the context of the ECDSA code, `bytesize` can be up to 66 (secp521r1), so `2 * 66 = 132` fits easily in 32 bits.

**Conclusion:** Not a bug in this case. No change needed.

---

### 7. Statistics Using `=` Instead of `+=` (Throughout)

**Issue:** All statistics updates use `=` (assignment) instead of `+=` (accumulation):

```c
stats->passed++;      /* correct: increment */
stats->failed++;      /* correct: increment */
```

**Conclusion:** Upon review, all statistics updates use `++` (pre/post-increment), which is correct. This is **not** the `stats->rx_packets = nb_rx` anti-pattern. No issue here.

---

### 8. memcmp() on Authentication Tag (Line 948, 958, etc.)

**Issue:** When verifying AEAD ciphertext or MAC tags in valid-vector tests, the code uses plain `memcmp()` to compare the generated digest/tag against the expected value:

```c
/* Line 948 */
if (memcmp(output, vector->ct, vector->ct_len) != 0 ||
    memcmp(digest, vector->tag, vector->tag_len) != 0)
    goto failed;
```

This is a **timing side-channel vulnerability**. `memcmp()` returns early on the first differing byte, leaking information about which bytes matched. For authentication tags (MAC, AEAD tag, ECDSA signature verification), this allows an attacker to incrementally forge valid tags byte-by-byte via timing measurements.

**Fix:** Replace `memcmp()` with `rte_memeq_timingsafe()` for all tag/digest/signature comparisons:

```c
if (!rte_memeq_timingsafe(digest, vector->tag, vector->tag_len))
    goto failed;
```

Note: `rte_memeq_timingsafe()` returns `true` on equal (opposite of `memcmp`'s zero-on-equal).

Apply this fix at:
- Line 948 (AEAD tag comparison)
- Line 1099, 1112 (HMAC digest comparison)
- Line 1184 (GMAC digest comparison)

For comparison of *ciphertext* (line 948, `output` vs `vector->ct`), plain `memcmp()` is acceptable -- ciphertext is not secret and timing leaks are not exploitable. Only the tag comparison must use constant-time comparison.

**Scope:** This is a **security error** specific to cryptographic code, per the AGENTS.md section "Cryptographic and Security Code."

---

## WARNINGS (Should Fix)

### 1. Missing Release Notes for Internal API (Not Flagged)

**Analysis:** The patch adds release notes for the new example application (lines 58-63 in `doc/guides/rel_notes/release_26_11.rst`). This is an *application*, not an internal API or helper function. Release notes are appropriate here.

**Conclusion:** No issue.

---

### 2. Inappropriate Use of `rte_malloc()` (Lines 368, 400, etc.)

**Issue:** The code uses `rte_malloc()` for temporary buffers (hex-decoded vectors, digest buffers, AAD) that are not accessed by DMA and do not need to be in hugepage memory. Per AGENTS.md guidelines, `rte_malloc()` should only be used for DMA-accessible memory or memory shared between primary/secondary processes.

```c
/* Line 368 */
decoded = rte_malloc(NULL, *length, 0);

/* Line 600 */
*digest = rte_malloc(NULL, vector->tag_len, RTE_CACHE_LINE_SIZE);

/* Line 604 */
aad = rte_zmalloc(NULL, RTE_ALIGN_CEIL(vector->aad_len + AES_CCM_AAD_OFFSET, 16), 0);
```

**Fix:** Replace with standard `malloc()` for all non-DMA, non-shared buffers. Only use `rte_malloc()` for data that will be accessed by the cryptodev hardware (which in this example is only the mbuf payload, already handled by mbuf pools).

---

### 3. Boolean vs Integer (env.debug, env.hash_dev_own)

**Issue:** The fields `env.debug` and `env.hash_dev_own` are declared as `bool` (lines 57-58), which is correct. All assignments use `true`/`false` (lines 162, 269), and all conditionals use direct truthiness (line 88, 259, etc.). This follows the `bool` usage guidelines.

**Conclusion:** No issue.

---

### 4. Hard-coded Mempool Names (Lines 196, 207, 215, 231, 233)

**Issue:** The mempool names (`"WYCHEPROOF_MBUF_POOL"`, `"WYCHEPROOF_SESSION_POOL"`, etc.) are hard-coded strings. If the application is run multiple times in a primary-secondary setup, the second instance will fail to create pools with the same names.

**Severity:** Warning -- This is an example application, not a production library. Hard-coded names are acceptable for single-instance apps, but documenting the limitation would be helpful.

**Suggestion:** Add a note in the documentation or a comment that the application does not support multiple instances.

---

### 5. Missing NULL Check After rte_cryptodev_asym_session_create() (Lines 1386-1387, 1910-1911)

**Issue:** In `run_ecdh_ecpoint()` and `run_ecdsa_verify()`, the comment states "Session-create failure after the capability check is a PMD failure," and the code sets `ret = -EIO` and jumps to `out:`. However, the check is `session == NULL`, which could also indicate an out-of-memory condition (not a PMD bug). The distinction matters for reporting.

**Suggestion:** Preserve `-ENOMEM` for allocation failures and use `-EIO` only when the PMD returns an error code from enqueue/dequeue. Currently, the code cannot distinguish the two.

---

### 6. Style: Overly Long Lines (Lines 169-170, 263, 276, etc.)

**Issue:** Several lines exceed 100 characters:

```c
/* Line 169 */
	printf("Using cryptodev %u (%s)\n", env.dev_id, rte_cryptodev_name_get(env.dev_id));
```

**Fix:** Break long lines at logical boundaries:

```c
printf("Using cryptodev %u (%s)\n", env.dev_id,
       rte_cryptodev_name_get(env.dev_id));
```

**Severity:** Warning -- the guideline states "Lines <=100 characters" but these are only slightly over. Borderline.

---

### 7. Redundant Memset After rte_zmalloc (Line 604)

**Issue:**

```c
aad = rte_zmalloc(NULL, RTE_ALIGN_CEIL(vector->aad_len + AES_CCM_AAD_OFFSET, 16), 0);
```

`rte_zmalloc()` zero-initializes the allocated memory, so the subsequent `memcpy()` at line 606 is correct. However, if the intention was to zero only the prefix and copy AAD into the suffix, the code should allocate with `rte_malloc()` and zero only the prefix. As written, the `rte_zmalloc()` is correct but slightly less efficient than necessary.

**Conclusion:** Not an error. The code is correct.

---

## INFO (Consider)

### 1. Optimization: Pre-compute Session Capabilities

**Suggestion:** The code repeatedly calls `rte_cryptodev_sym_capability_get()` and `check_aead_capability()` for every vector in a test group, even when the key/tag/AAD sizes are the same across the group. Pre-computing capability checks per test group (lines 982-1004) would reduce overhead.

---

### 2. Error Message Clarity (Line 263)

```c
printf("Cannot initialize cryptodev %u: %s\n", env.dev_id, rte_strerror(-ret));
```

When `app_init()` fails, the error could be from `rte_cryptodev_configure()`, mempool creation, or queue setup. The message doesn't distinguish these cases. Consider adding more specific error messages within `app_init()`.

---

### 3. Unused Field in wycheproof_data (Lines 39-49)

**Observation:** The `struct wycheproof_data` has fields `ct` and `ct_len`, but in HMAC/GMAC vectors (which use `parse_mac_vector()`), these fields are never populated. The fields are only used in AEAD and asymmetric tests. Consider splitting the structure or documenting which fields apply to which vector types.

---

## FINAL SUMMARY

### Critical Issues (Must Fix Before Merge)

1. **Resource leaks** on session creation failure in `run_aead()`, `run_hmac()`, `run_gmac()` (lines 585, 678, 752)
2. **Use-after-free** in timeout path across all crypto operation functions (lines 635, 724, 804, 1037, 1401, 1739, 1930)
3. **Buffer overflow risk** in CCM IV handling (line 616)
4. **Missing error propagation** in hash device selection