|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

# Wycheproof Validation Example - DPDK Code Review

## ERRORS

**1. Missing error check on `rte_cryptodev_asym_session_create()` (line 1414, 1531, 1803)**

The code checks only if the return value is `< 0` or if `session == NULL`, then treats it as `-ENOTSUP`. However, `rte_cryptodev_asym_session_create()` returns an `int` status code, not a pointer. The `session` parameter is an output pointer that is written to. The pattern should check the return value for error, not compare `session` to NULL as a separate condition.

```c
/* Current (incorrect) */
if (rte_cryptodev_asym_session_create(env.dev_id, &xform, env.asym_session_pool,
        &session) < 0 || session == NULL) {
    ret = -ENOTSUP;
    goto out;
}

/* Correct */
ret = rte_cryptodev_asym_session_create(env.dev_id, &xform, env.asym_session_pool,
    &session);
if (ret < 0 || session == NULL) {
    ret = -ENOTSUP;
    goto out;
}
```

The current pattern discards the return value in the conditional, making `ret = -ENOTSUP;` always execute regardless of what `rte_cryptodev_asym_session_create()` actually returned.

**2. Device selection for hash_dev_id modifies global state but never restores it on error (lines 261-292)**

When searching for a hash-capable device distinct from `env.dev_id`, the code configures, sets up a queue pair, and starts the device at index `d`. If any of these operations fail, the code calls `rte_cryptodev_close(d)` and continues searching. However:

- `rte_cryptodev_configure(d, &config)` was called and may have partially initialized device `d`.
- If `rte_cryptodev_queue_pair_setup()` fails, the device is in a configured-but-not-setup state.
- Calling `rte_cryptodev_close()` without first calling `rte_cryptodev_stop()` if the device was started is incorrect cleanup order.

The cleanup should be:
```c
if (rte_cryptodev_start(d) < 0) {
    rte_cryptodev_queue_pair_setup(...);  /* may have succeeded */
    rte_cryptodev_close(d);
    continue;
}
```
should be:
```c
if (rte_cryptodev_start(d) < 0) {
    rte_cryptodev_stop(d);  /* in case partial start */
    rte_cryptodev_close(d);
    continue;
}
```

But more importantly: if `rte_cryptodev_configure(d, ...)` succeeds but later steps fail, the device is left in an inconsistent state. The code should call `rte_cryptodev_stop()` before `rte_cryptodev_close()` in the error path.

**3. Potential buffer overflow in CCM AAD setup (line 654)**

```c
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);
```

The allocation size is `RTE_ALIGN_CEIL(vector->aad_len + 18, 16)`. If `vector->aad_len` is `UINT32_MAX - 17`, then `vector->aad_len + 18` wraps to 1, `RTE_ALIGN_CEIL(1, 16)` becomes 16, but then `memcpy(aad + 18, vector->aad, UINT32_MAX - 17)` writes far beyond the 16-byte allocation.

While `vector->aad_len` is parsed from JSON and unlikely to be that large in practice, there is no explicit bounds check before the addition. Add a check:

```c
if (vector->aad_len > UINT32_MAX - AES_CCM_AAD_OFFSET - 16)
    goto unsupported;
```

**4. `normalize_operand()` can be called with overlapping source and destination (lines 1475, 1795)**

In `run_ecdh_ecpoint()` at line 1475:
```c
if (normalize_operand(priv, priv_len, priv_buf, bytesize) != 0)
    return -EINVAL;
```
and in `run_ecdsa_verify()` at lines 1795-1796:
```c
if (normalize_operand(group->wx, group->wx_len, wx_buf, group->bytesize) != 0 ||
        normalize_operand(group->wy, group->wy_len, wy_buf, group->bytesize) != 0)
```

The function `normalize_operand()` at line 1368 does:
```c
while (value_len > 0 && value[0] == 0) {
    value++;
    value_len--;
}
...
memcpy(out + bytesize - value_len, value, value_len);
```

If `value` and `out` overlap (e.g., caller passes the same buffer as source and destination), the `memcpy()` has undefined behavior. While the current callers do not pass overlapping buffers, the function does not document or enforce this precondition. Either document the requirement or use `memmove()`.

**5. Wedged device comment claims "op still owned by the PMD" but code returns without freeing session (multiple locations)**

At lines 704, 817, 925, 1056, 1423, 1540, and 1812, the code has:
```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;
```

However, the `session` was allocated and is never freed on this path. The comment says "without freeing in-flight state" but that applies only to the `op` and `mbuf`, not to the session which is not in-flight. The session should be freed:

```c
if (completed == NULL) {
    ret = -ETIMEDOUT;
    goto out;  /* will free session */
}
```

As written, every wedged-device timeout leaks the session.

---

## WARNINGS

**1. `hash_dev_id` device selected but not validated as capable of all required hash algorithms (lines 261-292)**

The code searches for a device that advertises `RTE_CRYPTO_AUTH_SHA256`, assigns it to `env.hash_dev_id`, and later uses `env.hash_dev_id` to compute digests for DSA and ECDSA which may require SHA-1, SHA-224, SHA-384, or SHA-512 (lines 1196-1210, 1219). The device selected for SHA256 capability may not support all hash algorithms needed by the vectors. The code will fail later with `-ENOTSUP` when attempting to create a session for an unsupported hash, but the device selection logic should either:
- Check for a broader set of hash algorithms, or
- Document that the hash device is selected opportunistically and may not support all vectors.

**2. Release notes claim "covering AEAD, MAC, DSA, ECDH and ECDSA" but the example skips many algorithm variants (doc/guides/rel_notes/release_26_11.rst line 5)**

The release notes say the example "covers" these families, but the code explicitly skips AES-CBC, AES-CTR, SEED, SM4-CCM, RSA, and many DSA/ECDH/ECDSA encoding formats (process_file() lines 2046-2050). "Covers" implies comprehensive support; "validates selected algorithms from" would be more accurate. This is a documentation clarity issue, not a code bug.

**3. `dequeue_one()` timeout is 1 second, may be too short for slow software crypto (line 550)**

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

On systems with `rte_get_timer_hz()` returning cycles-per-second, this is a 1-second timeout. For a software cryptodev on a heavily loaded system, or for large asymmetric operations (RSA/DSA), 1 second may be insufficient. Consider making the timeout configurable or increasing it to 5-10 seconds for asymmetric operations.

**4. `env.mbuf_data_room` defaults to 16384 but vectors are skipped if message exceeds `env.mbuf_data_room - RTE_PKTMBUF_HEADROOM` (lines 887, 1192)**

The default 16KB data room is reasonable, but the skip condition `vector->msg_len > env.mbuf_data_room - RTE_PKTMBUF_HEADROOM` means messages slightly under 16KB (accounting for headroom) will be skipped even though they are valid test cases. The skip message says "message length exceeds mbuf" but this is a limitation of the tool's default configuration, not the vector. Consider either increasing the default or making the skip debug message say "message length exceeds configured mbuf data room".

**5. Application does not set `enable_drivers=crypto/openssl` in documented meson command but then requires it (doc/guides/sample_app_ug/wycheproof_validation.rst line 25)**

The build command shows:
```
meson setup build -Dexamples=crypto_validation/wycheproof_validation \
    -Denable_drivers=crypto/openssl
```
but then the run command also uses `--vdev crypto_openssl`. If `enable_drivers` is not set during build, the `crypto_openssl` PMD will not be built, and the vdev will fail at runtime. The documentation should clarify that `-Denable_drivers` is required to build the PMD, or omit it and explain how to ensure a crypto PMD is available.

**6. `parse_uint32()` does not distinguish "0" from invalid input when `errno` is already non-zero (line 100)**

```c
errno = 0;
parsed = strtoul(text, &end, 10);
if (errno != 0 || *text == '\0' || *end != '\0' || parsed > UINT32_MAX)
    return -EINVAL;
```

If `errno` is non-zero before the call (leftover from a previous operation), `strtoul()` does not clear it, and valid input "0" will be rejected. The pattern is correct (setting `errno = 0` before the call), but the comment or a more defensive check would clarify this. This is borderline; the code is correct as written.

---

## INFO

**1. `struct app_env` field `hash_dev_id` and `hash_dev_own` are not freed if `app_init()` fails after hash device allocation (pre-existing pattern, not introduced by this patch)**

If `app_init()` fails after the hash device is selected and started (line 292), but before the function completes, `app_uninit()` will not be called, and the hash device will not be stopped or closed. This is a pre-existing pattern in DPDK examples (setup functions that fail partway through), but it means a failure in the main body of `app_init()` after line 292 will leave the hash device running.

The fix would be to add cleanup in the `error:` label:
```c
error:
    if (env.hash_dev_own) {
        rte_cryptodev_stop(env.hash_dev_id);
        rte_cryptodev_close(env.hash_dev_id);
    }
    rte_mempool_free(env.asym_op_pool);
    /* ... rest of cleanup ... */
```

**2. `decode_hex()` allocates with `rte_malloc()` but no alignment requirement is specified (line 381)**

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

The third parameter is alignment; `0` means default (which is cache-line on most platforms). For buffers that will be DMA'd to hardware (keys, IVs, messages), cache-line alignment is appropriate. For temporary buffers that are only CPU-accessed, `malloc()` would suffice and consume less hugepage memory. See the "Appropriate Use of rte_malloc()" section in AGENTS.md. This is an Info note because the current usage is not incorrect, just heavier than necessary.

**3. Pre-existing: MAINTAINERS diff shows FIPS validation files moved but the MAINTAINERS entry for "FIPS validation example" still points to `examples/fips_validation/` (MAINTAINERS line 2029)**

The files were moved to `examples/crypto_validation/fips_validation/` but the MAINTAINERS entry was not updated to match. The correct path after this patch should be:
```
F: examples/crypto_validation/fips_validation/
```

This is a minor consistency issue, not a functional bug.

---

Review-Result: ERROR