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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: Wycheproof Validation Example

## Summary
This patch adds a new example application that validates cryptodev PMDs against Google Wycheproof test vectors. The implementation is generally solid but has several correctness issues that must be addressed.

---

## ERRORS (Must Fix)

### 1. Resource Leak on Error Path in `run_aead()`
**Location:** `run_aead()` function, line ~622

When `rte_pktmbuf_alloc()` succeeds but the input data exceeds tailroom, the function returns `-EMSGSIZE` without freeing the allocated mbuf.

```c
mbuf = rte_pktmbuf_alloc(env.mbuf_pool);
if (mbuf == NULL)
    goto out;
input = operation == RTE_CRYPTO_AEAD_OP_ENCRYPT ? vector->msg : vector->ct;
input_len = operation == RTE_CRYPTO_AEAD_OP_ENCRYPT ? vector->msg_len : vector->ct_len;
if (input_len > rte_pktmbuf_tailroom(mbuf)) {
    ret = -EMSGSIZE;
    goto out;  /* BUG: mbuf allocated but not freed on this path */
}
```

**Fix:** The `goto out` is correct--the `out` label does call `rte_pktmbuf_free(mbuf)`. However, verify that `rte_pktmbuf_free(NULL)` is safe (it is in DPDK). **Actually correct--not an error.** (Removing this item per guidelines.)

### 2. Resource Leak in `run_aead()` on AAD Allocation Failure for AES-CCM
**Location:** `run_aead()` function, line ~647

When AES-CCM AAD allocation fails, the function goes to `out` but the `aad` pointer is still NULL, so `rte_free(aad)` does nothing. However, other resources (mbuf, digest, session, op) may have been allocated earlier and are correctly freed by the `out` label. **Actually correct--not an error.**

### 3. Missing NULL Check After `rte_cryptodev_asym_session_create()` Partial Failure
**Location:** Multiple locations: `run_dsa_verify()` (line ~1424), `run_ecdh_ecpoint()` (line ~1655), `run_ecdsa_verify()` (line ~1939)

The code checks `session != NULL` after a failed `rte_cryptodev_asym_session_create()`, but the session pointer is uninitialized. If `rte_cryptodev_asym_session_create()` returns `-1` without modifying the session pointer, the subsequent `session != NULL` check on an uninitialized pointer is undefined behavior.

```c
void *session = NULL;  /* initialized to NULL, which is correct */
...
if (rte_cryptodev_asym_session_create(env.dev_id, &xform, env.asym_session_pool,
        &session) < 0 || session == NULL) {
    ret = -ENOTSUP;
    goto out;
}
```

**Actually, the initialization `void *session = NULL;` makes this safe.** If the create call fails without writing to `session`, it remains NULL and the check is well-defined. **Not an error.**

### 4. Use of `rte_malloc()` for General-Purpose Allocations
**Location:** Throughout the file: `decode_hex()`, `run_aead()`, `run_hmac()`, `run_gmac()`, `run_ecdh_ecpoint()`, etc.

The code uses `rte_malloc()` / `rte_zmalloc()` for allocations that are not shared memory and do not require hugepage backing (decoded hex strings, temporary buffers, digest outputs). These should use standard `malloc()` / `calloc()` to avoid consuming limited hugepage resources unnecessarily.

Hugepage allocations are required for:
- Mbuf data (already using `rte_pktmbuf_alloc()` correctly)
- DMA-accessible buffers (digest, AAD, operands attached to crypto ops via `phys_addr`)

However, intermediate buffers like decoded vectors, group parameters (`dsa.p`, `ec.wx`, etc.), and signature buffers do not need hugepage backing **unless** they are directly referenced by crypto ops via `phys_addr`. In this code, `rte_malloc_virt2iova()` is called on digest/AAD buffers, **so they DO need to be rte_malloc'd for DMA access.**

**Revised assessment:** The usage is **correct** for buffers attached to crypto ops (digest, AAD, operands). The decoded hex buffers in `decode_hex()` (vector data) are not DMA'd, but they are temporary and freed quickly. Using `rte_malloc()` there is suboptimal but not a correctness bug. This becomes a **Warning** rather than Error.

### 5. Missing Error Check on `rte_cryptodev_asym_session_free()`
**Location:** Multiple locations: error paths in `run_dsa_verify()`, `run_ecdh_ecpoint()`, `run_ecdsa_verify()`

The `rte_cryptodev_asym_session_free()` call is not checked for errors. However, this is called on cleanup paths where the function is already returning an error. The Linux kernel pattern (and DPDK's) is that free functions always succeed or are best-effort; not checking the return is acceptable. **Not an error per guidelines (free functions).**

### 6. Potential Integer Overflow in `decode_hex()`
**Location:** `decode_hex()` function, line ~390

```c
hex_length = strlen(hex);
if ((hex_length & 1) != 0 || hex_length / 2 > UINT32_MAX)
    return -EINVAL;

*length = hex_length / 2;
```

If `hex_length` is `SIZE_MAX` (the maximum value of `size_t`), then `hex_length / 2` is `SIZE_MAX / 2`, which is less than `UINT32_MAX` on 64-bit systems (since `UINT32_MAX` is `0xFFFFFFFF` and `SIZE_MAX / 2` is `0x7FFFFFFFFFFFFFFF`). However, the allocation `rte_malloc(NULL, *length, 0)` will succeed, and the subsequent loop will write far beyond any reasonable memory. 

**Actually, the check `hex_length / 2 > UINT32_MAX` is incorrect on 64-bit.** On a 64-bit system, `size_t` is 64 bits, so `hex_length` could be very large. The check should be `hex_length > 2 * UINT32_MAX` or equivalently `hex_length / 2 > UINT32_MAX` **which is what the code does**. But: if `hex_length` is `2^33` (valid on 64-bit), then `hex_length / 2` is `2^32`, which equals `UINT32_MAX + 1`, failing the check. So the boundary is off-by-one.

**Corrected check:**
```c
if ((hex_length & 1) != 0 || hex_length > 2 * (size_t)UINT32_MAX)
    return -EINVAL;
```

This is an **Error** (integer overflow risk).

### 7. Missing Bounds Check in `normalize_operand()`
**Location:** `normalize_operand()` function, line ~1577

```c
static int
normalize_operand(const uint8_t *value, uint32_t value_len, uint8_t *out, uint32_t bytesize)
{
    while (value_len > 0 && value[0] == 0) {
        value++;
        value_len--;
    }
    if (value_len > bytesize)
        return -EINVAL;
    memset(out, 0, bytesize);
    memcpy(out + bytesize - value_len, value, value_len);  /* potential underflow if value_len > bytesize */
    return 0;
}
```

**Wait--the check `if (value_len > bytesize) return -EINVAL;` prevents the underflow.** If `value_len > bytesize`, the function returns early. If `value_len <= bytesize`, then `bytesize - value_len` is safe. **Not an error.**

### 8. Missing Check for `rte_get_timer_hz() == 0` in `dequeue_one()`
**Location:** `dequeue_one()` function, line ~492

```c
uint64_t deadline = rte_get_timer_cycles() + rte_get_timer_hz();
```

If `rte_get_timer_hz()` returns 0 (on platforms without TSC), the deadline calculation is wrong and the timeout will trigger immediately. However, `rte_get_timer_hz()` is documented to always return a valid frequency on initialized EAL. **Not flagging this--DPDK guarantees non-zero after EAL init.**

---

## WARNINGS (Should Fix)

### 1. Suboptimal Use of `rte_malloc()` for Non-DMA Buffers
**Location:** `decode_hex()` function, line ~413

Temporary buffers allocated in `decode_hex()` for vector data (key, IV, message, signature, etc.) are not DMA'd. Use `malloc()` instead of `rte_malloc()` to avoid consuming hugepages.

**Fix:**
```c
decoded = malloc(*length);
if (decoded == NULL)
    return -ENOMEM;
...
free(decoded);  /* instead of rte_free() */
```

Apply this to all `decode_hex()` allocations and corresponding `free_vector()` calls. For buffers that ARE attached to crypto ops (digest, AAD), keep `rte_malloc()`.

### 2. Missing `--debug` Documentation in Release Notes
**Location:** `doc/guides/rel_notes/release_26_11.rst`

The release notes mention the new example but do not describe the `--debug` option. This is a user-facing feature and should be documented.

### 3. Inconsistent Error Handling in `process_dsa_p1363()`, `process_ecdh_ecpoint()`, `process_ecdsa_p1363()`
**Location:** Multiple locations in asymmetric processing functions

When `decode_hex()` fails for group-level parameters (DSA p/q/g/y, ECDSA wx/wy), the code frees already-allocated fields and increments `skipped_unsupported`, but does not call `debug_vector()` or `debug_file()` to explain why. This is inconsistent with the pattern used for per-test skips.

**Fix:** Add a debug message when group-level decode fails.

### 4. Hard-Coded `MAX_EC_BYTESIZE` May Be Insufficient
**Location:** Line 34: `#define MAX_EC_BYTESIZE 66`

This constant is sized for secp521r1 (66 bytes). If future EC curves with larger field sizes are added to DPDK, this will silently fail. Consider adding a runtime check or increasing the constant with a comment.

### 5. No Check for `env.hash_dev_id == env.dev_id` Before Attempting Fallback
**Location:** `app_init()` hashing device selection, line ~291

The code searches for a separate hashing device only if the target device lacks SHA256 capability. However, if the search fails (no sym-capable device found), `env.hash_dev_id` remains set to `env.dev_id` without `env.hash_dev_own` being set. Later asymmetric tests will call `compute_hash()` on a device that may not support SHA, causing runtime failures. This is not a leak or crash, but the error path is unclear.

**Fix:** Check whether a suitable hash device was found and warn the user if asymmetric tests will be unavailable due to missing hash support.

---

## INFO (Consider)

### 1. Use of `strcmp()` Result Directly in Conditions
**Location:** Throughout, e.g., line ~714: `if (strcmp(vector->result, "acceptable") == 0)`

This is **correct** per DPDK style--the code uses explicit comparison against 0. No issue.

### 2. Long Lines in `main.c`
**Location:** Multiple locations, e.g., line ~642

Some lines exceed 100 characters (e.g., function calls with many arguments). DPDK allows up to 100 characters; consider breaking very long lines for readability.

### 3. JSON Library Dependency
**Location:** `meson.build`, `Makefile`

The example requires `jansson` (JSON library). The Makefile checks for its presence, but the error message could be more helpful by suggesting installation commands.

**Suggestion:**
```makefile
ifneq ($(shell $(PKGCONF) --exists jansson && echo 0),0)
$(error "Jansson JSON library is required. Install with: apt-get install libjansson-dev (Debian/Ubuntu) or yum install jansson-devel (RHEL/CentOS)")
endif
```

---

## SUMMARY

**Critical issues (must fix before merge):**
1. Integer overflow risk in `decode_hex()` boundary check (off-by-one on 64-bit systems)

**Recommended fixes:**
1. Use `malloc()` instead of `rte_malloc()` for non-DMA temporary buffers (performance optimization)
2. Document `--debug` option in release notes
3. Add debug messages for group-level decode failures in asymmetric tests
4. Consider increasing `MAX_EC_BYTESIZE` or adding runtime validation
5. Clarify error path when no hash-capable device is available for asymmetric tests

**Overall assessment:**
The patch is well-structured and demonstrates good DPDK coding practices (bounded timeouts, proper cleanup paths, capability checks). The primary correctness issue is the `decode_hex()` boundary check, which must be fixed. The other items are optimizations or clarity improvements.