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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Wycheproof Validation Example Review

## ERRORS

**Error: Missing release notes for moved FIPS validation example**

The patch moves `examples/fips_validation/` to `examples/crypto_validation/fips_validation/` but does not document this in the release notes. Moving an example to a new path is a user-visible change and must be noted in `doc/guides/rel_notes/release_26_11.rst`.

**Error: Resource leak on queue pair setup failure**

`main.c:215` - If `rte_cryptodev_queue_pair_setup()` fails, the function jumps to `error:` without calling `rte_cryptodev_configure()` cleanup. The device remains configured but unusable, and the session/op/mbuf pools allocated earlier are not freed.

```c
ret = rte_cryptodev_queue_pair_setup(env.dev_id, 0, &queue_pair,
    rte_socket_id());
if (ret < 0)
    goto error;
```

The `error:` label only frees pools; it does not call `rte_cryptodev_close()`.

**Error: Resource leak on device start failure**

`main.c:219` - If `rte_cryptodev_start()` fails, the function jumps to `error:` without closing the cryptodev. The device remains in a partially initialized state and is not freed.

**Error: Session pool not freed on early configure failure**

`main.c:195` - If `rte_cryptodev_configure()` fails, the function jumps to `error:` before `env.session_pool` is assigned. At the `error:` label, `rte_mempool_free(env.session_pool)` is called on an uninitialized pointer (contains whatever was in the BSS or stack). This is a NULL pointer dereference or use of uninitialized memory.

The assignment `env.session_pool = ...` happens after `rte_cryptodev_configure()` returns, but the error path assumes the pool pointer is valid.

**Error: Integer overflow in hex decode**

`main.c:391` - The expression `hex_length / 2` is performed on a `size_t`, but the result is assigned to `*length` which is `uint32_t`. If `hex_length` is larger than `2 * UINT32_MAX`, the check `hex_length / 2 > UINT32_MAX` is performed after the division has already truncated. The check should be `hex_length > 2 * UINT32_MAX` before the division.

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

However, since `size_t` is at least 32 bits and often 64 bits, `hex_length / 2` computed as `size_t` may be larger than `UINT32_MAX` before the comparison. The assignment `*length = hex_length / 2` truncates silently if `hex_length / 2` exceeds `UINT32_MAX`.

**Error: Use of uninitialized `env` fields on error path**

`main.c:297-305` - The `error:` label in `app_init()` calls `rte_mempool_free()` on `env.asym_op_pool`, `env.asym_session_pool`, etc., but these fields are only conditionally assigned (asymmetric pools are "best-effort"). If pool creation fails or is skipped, the pointers remain uninitialized (or contain prior values if `env` is not zeroed), and `rte_mempool_free()` is called on garbage.

The code sets the pointers to `NULL` after freeing at the end of the error path, but does not initialize them to `NULL` before attempting allocation. The global `env` structure may be zero-initialized by the compiler, but this is not guaranteed for all fields in all compilation modes.

**Error: Hash device not stopped on failure in DSA/ECDSA paths**

`main.c:265-292` - The code starts `env.hash_dev_id` with `rte_cryptodev_start(d)` if a separate hash device is selected, but if this succeeds and a later operation fails (e.g., asymmetric pool creation fails on line 242), the function jumps to `error:` without stopping or closing `env.hash_dev_id`. The device remains started and consumes resources.

The `app_uninit()` function only stops and closes `env.hash_dev_id` if `env.hash_dev_own` is true, but `env.hash_dev_own` is set to true before starting the device. If the device is started but the function then fails, `app_uninit()` is never called, so the device leaks.

**Error: Missing check for device validity in hash device selection loop**

`main.c:269` - The loop that searches for a hash-capable device calls `rte_cryptodev_configure(d, &config)`, `rte_cryptodev_queue_pair_setup(d, 0, &hash_qp, ...)`, and `rte_cryptodev_start(d)` without first checking that device `d` is not already in use. If device `d` is already configured and started (e.g., it is `env.dev_id` or another device the application might use later), configuring it again could fail or break existing state.

The code does check `if (d == env.dev_id)` and `continue`, but it does not check `rte_cryptodev_is_valid_dev(d)` or whether the device is already started.

**Error: Memory leak when session create fails in DSA/ECDSA/ECDH paths**

`main.c:1271-1276, 1575-1580, 1783-1788` - In `run_dsa_verify()`, `run_ecdh_ecpoint()`, and `run_ecdsa_verify()`, if `rte_cryptodev_asym_session_create()` returns failure or NULL, the function sets `ret = -ENOTSUP` and jumps to `out:`. However, the `out:` label only frees the session if `session != NULL`. If session creation fails, `session` may be non-NULL but invalid (some PMDs may return a non-NULL pointer on failure), or it may leak internal resources. The code should unconditionally attempt to free the session if the create call was made.

Actually, re-reading the code: `session` is initialized to `NULL`, and `rte_cryptodev_asym_session_create()` returns `< 0` on error. The code checks `< 0 || session == NULL`, so if the function returns success but `session` is NULL, that is caught. If the function returns error, `session` remains NULL and the free is skipped, which is correct. However, the function signature of `rte_cryptodev_asym_session_create()` is `int rte_cryptodev_asym_session_create(uint8_t dev_id, struct rte_crypto_asym_xform *xforms, struct rte_mempool *mp, void **session)`. It returns `int` and writes to `*session`. If it returns < 0, `*session` may be undefined. The code should zero `session` before the call and only free if non-NULL, which it does. This is correct.

On closer inspection, the code is correct here. Withdrawn.

**Error: Potential NULL dereference in AEAD AAD handling**

`main.c:665-667` - If `algorithm == RTE_CRYPTO_AEAD_AES_CCM` and `vector->aad_len != 0`, the code allocates `aad` with `rte_zmalloc()` and checks `if (aad == NULL) goto out;`. However, if `vector->aad_len == 0`, the `memcpy(aad + AES_CCM_AAD_OFFSET, vector->aad, vector->aad_len)` inside the `if` is skipped, and `aad` is never allocated. Later, `sym_op->aead.aad.data = aad;` is set to an uninitialized pointer. The `else` branch sets `sym_op->aead.aad.data = vector->aad;`, but the CCM path always uses `aad`, even when `vector->aad_len == 0`.

Correction: re-reading the code:

```c
if (algorithm == RTE_CRYPTO_AEAD_AES_CCM) {
    aad = rte_zmalloc(NULL,
        RTE_ALIGN_CEIL(vector->aad_len + AES_CCM_AAD_OFFSET, 16), 0);
    if (aad == NULL)
        goto out;
    if (vector->aad_len != 0)
        memcpy(aad + AES_CCM_AAD_OFFSET, vector->aad, vector->aad_len);
    sym_op->aead.aad.data = aad;
    ...
}
```

The allocation happens unconditionally for CCM, even when `vector->aad_len == 0`. The allocation size is `RTE_ALIGN_CEIL(0 + 18, 16) = 32`, so a buffer is allocated. The `if (aad == NULL)` check is correct. This is correct.

Withdrawn.

**Error: Unbounded loop in directory traversal**

`main.c:2047-2063` - The `readdir()` loop in `process_path()` has no bound on the number of entries processed. A malicious or corrupt filesystem with a circular directory structure or an extremely large number of entries could cause the loop to run indefinitely or consume excessive memory.

However, `readdir()` returns `NULL` when the directory is exhausted, and POSIX filesystems do not have circular directories (`.` and `..` are handled by the filesystem, not returned as separate entries by `readdir()`). A large directory is a performance issue, not a correctness bug.

This is not a bug. Withdrawn.

---

## WARNINGS

**Warning: Missing functional tests**

The patch adds a new example application but does not add any tests to `app/test/`. New applications that exercise cryptodev should include unit tests in the test framework.

**Warning: Missing testpmd integration**

The patch adds a new cryptodev example but does not add hooks in `app/testpmd/`. New cryptodev validation code should be integrated into testpmd for broader testing.

**Warning: Error handling in `decode_hex` silently succeeds on zero-length input**

`main.c:387-397` - If `hex_length == 0`, the function sets `*length = 0` and returns `0` without allocating `*buffer`. The caller must check `if (*length == 0)` before using `*buffer`, but not all callers do. For example, `parse_vector()` calls `decode_hex()` for `key`, `iv`, `aad`, etc., and if any of these are zero-length, the corresponding `vector->key` pointer remains uninitialized (or NULL if the structure was zeroed). The code should either allocate a zero-length buffer or document that `*buffer` is undefined when `*length == 0`.

The callers do check lengths before using the buffers (e.g., `if (vector->msg_len != 0) memcpy(...)`), so this is not a bug, but it is a subtle API contract that should be documented.

**Warning: `compute_hash()` does not validate that `digest_len` matches the algorithm's natural output size**

`main.c:1165-1174` - The function accepts `digest_len` as a parameter and passes it to `xform.auth.digest_length`, but does not check that it matches the algorithm's expected output size. If the caller passes a `digest_len` that is too small, the PMD may write beyond the buffer or fail unpredictably.

The callers (`run_dsa_verify`, `run_ecdsa_verify`) pass `group->digest_len` which is derived from `map_sha()`, so it is correct. However, the function should validate the parameter for robustness.

**Warning: `normalize_operand()` does not check for all-zero input**

`main.c:1513-1524` - If `value` is all zeros, the loop strips all bytes and `value_len` becomes 0. The function then sets `out` to all zeros and returns 0. The caller may not expect a zero operand. For elliptic curve operations, a zero private key or public key coordinate is invalid and should be rejected.

The DPDK asymmetric API may reject this at the PMD level, but the application should validate it earlier to provide a clearer error message.

**Warning: `snprintf` truncation not checked consistently**

`main.c:2056-2060` - The code checks if `snprintf()` returned >= `sizeof(file_path)` and returns `-ENAMETOOLONG`, but in other parts of the code (e.g., `usage()` at line 79), `printf()` is used without checking for truncation. This is consistent within the path-building code but inconsistent across the application.

For the path-building case, the check is correct. For diagnostic messages, truncation is not critical.

**Warning: `dequeue_one()` busy-waits without backoff**

`main.c:542-551` - The function spins on `rte_cryptodev_dequeue_burst()` until a completed operation is available or a deadline is reached. It calls `rte_pause()` on each iteration, which is a hint to the CPU, but does not implement exponential backoff or yield to the OS. On a heavily loaded system, this could waste CPU cycles.

However, `rte_pause()` is the recommended DPDK idiom for spin-wait loops, and the 1-second deadline ensures the loop exits even if the device is wedged. This is acceptable for a test application.

**Warning: `app_uninit()` does not check for NULL pointers before freeing**

`main.c:314-321` - The function calls `rte_mempool_free(env.asym_op_pool)`, etc., without checking if the pointer is NULL. `rte_mempool_free()` is documented to accept NULL and be a no-op, so this is correct. However, the code does not explicitly initialize the global `env` structure to zero, so if `app_uninit()` is called before `app_init()` completes, the pointers may contain garbage.

The `env` structure is declared as `static struct app_env env = { .mbuf_data_room = DEFAULT_MBUF_DATA_ROOM };` at line 72. Static structures are zero-initialized by the compiler for all members not explicitly initialized. The mempool pointers are zero-initialized, so this is correct.

This is correct. Withdrawn.

**Warning: Session pool sized for "largest sym session across all devices" may over-allocate**

`main.c:205-215` - The code iterates over all cryptodevs to find the largest session size and uses that for the session pool. If the target device requires a small session but another device on the system requires a large session, the pool allocates more memory than necessary. This is a robustness trade-off: the pool can be used with the hash device, which may differ from the target device.

The comment explains this is intentional, so this is not a bug. It is a design choice.

**Warning: Hardcoded 1-second timeout in `dequeue_one()`**

`main.c:543` - The timeout is `rte_get_timer_hz()`, which is 1 second. A slow device might take longer to complete an operation, causing a false timeout. A faster device might benefit from a shorter timeout to detect wedges more quickly.

The 1-second timeout is reasonable for a test application and allows the tool to detect a stuck PMD without hanging indefinitely. This is acceptable.

**Warning: Error messages in `process_file()` do not distinguish between PMD errors and vector errors**

`main.c:760-762` - When a vector fails validation, the message prints `ret` and `status`, but if `ret` is an error code from a DPDK API (e.g., `-ETIMEDOUT`, `-EMSGSIZE`), the message does not indicate which API failed. This makes debugging harder.

The debug output is sufficient for a test tool. Adding more detail would make the code more complex.

**Warning: `check_aead_capability()` returns cryptodev error codes but no other capability check function does**

`main.c:558-573` - The function returns the result of `rte_cryptodev_sym_capability_check_aead()`, which is `0` on success or a negative error code. Other capability-check helpers (e.g., in `run_hmac()`, `run_gmac()`) return `-ENOTSUP` when the capability is absent and do not propagate the detailed error. This inconsistency couldwarning: review has no Review-Result line, severity inferred from prose (response may be truncated)