|WARNING| [v11,2/2] app: add Wycheproof validation app

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-11-06

# Review of DPDK Patch Series: FIPS/Wycheproof Validation Application Refactoring

## Patch 1/2: Move FIPS validation under app/

### ERRORS

None.

### WARNINGS

**Missing release notes update for FIPS validation move:**

The patch moves `fips_validation` from `examples/` to `app/` and updates documentation references, but does not add a release note entry for this structural change. While the change to `doc/guides/rel_notes/release_22_11.rst` updates an existing reference to the new path, the move itself should be documented in the current release notes (26.11) as a significant organizational change.

MAINTAINERS shows this targets the 26.11 development cycle, but `doc/guides/rel_notes/release_26_11.rst` is not modified. The change affects users building or referencing FIPS validation: the example is now a first-class application and the build invocation changes from `examples/` to `app/`.

Add a section in `doc/guides/rel_notes/release_26_11.rst` under "Other Changes" or similar:

```rst
* **Moved FIPS validation from examples/ to app/.**

  The FIPS cryptographic validation tool has been promoted from a sample
  application (``examples/fips_validation/``) to an official application
  (``app/fips_validation/``). Build with ``-Denable_apps=fips_validation``
  instead of enabling examples. Documentation has been updated accordingly.
```

**Removal of `allow_experimental_apis` without verification:**

In `app/fips_validation/meson.build`, the line `allow_experimental_apis = true` is removed. This is correct only if the FIPS validation code no longer calls any `__rte_experimental` API. The patch does not modify the C sources themselves (they are pure file moves), so if those sources call experimental API, removing this flag will cause a build failure.

Verify that the FIPS validation sources do not use experimental API, or retain `allow_experimental_apis = true` if they do.

---

## Patch 2/2: Add Wycheproof validation app

### ERRORS

**`rte_malloc()` used for ordinary control-path allocations:**

`app/wycheproof_validation/main.c` uses `rte_malloc()` and `rte_zmalloc()` extensively for allocating control-path structures, test vectors, and intermediate buffers. `rte_malloc()` allocates from hugepage memory, which is a limited resource and is not necessary for these allocations. Standard `malloc()` is faster and appropriate for general-purpose memory.

Lines where this occurs (examples):
- Line 408: `decoded = rte_malloc(NULL, *length, 0);` (hex decode buffer, non-DMA)
- Line 624: `*digest = rte_malloc(NULL, vector->tag_len, RTE_CACHE_LINE_SIZE);` (digest buffer; DMA-related but could use `malloc` if PMD accepts phys_addr from non-hugepage)
- Line 691: `aad = rte_zmalloc(NULL, RTE_ALIGN_CEIL(...), 0);` (CCM AAD, DMA-related)
- Line 806, 896, 997, 1307, 1389, etc.: similar patterns for digest, output, operand buffers

Review each `rte_malloc()`/`rte_zmalloc()` call:
- If the buffer is used for DMA (passed to `phys_addr` fields), hugepage allocation may be required.
- For control-path metadata (hex decode buffers, parsed vector fields, local copies not seen by hardware), use `malloc()`.

The queue descriptor ring and internal PMD structures are correctly allocated via the session/op pools and are not the issue. The issue is the numerous small test-vector and intermediate-result allocations.

**Missing `rte_free_sensitive()` for private key buffers:**

At line 1719, `rte_memzero_explicit(priv_buf, sizeof(priv_buf));` correctly wipes the stack buffer holding the private key after use. However, at lines 1519 and 1631, the private key is heap-allocated via `decode_hex()` (which calls `rte_malloc()`) and later freed with `rte_free_sensitive()`, which is correct. The stack-buffer case is fine. No error here on second review; the pattern is correct.

**Non-constant-time comparison of auth tag/MAC in test validation:**

At line 1000 (HMAC validation), the code uses:
```c
(vector.tag_len != 0 && memcmp(digest, vector.tag, vector.tag_len) != 0)
```
This is a plain `memcmp()` on an authentication tag, comparing the computed digest against the expected tag. This leaks timing information about which byte differs and is a side-channel vulnerability.

Use `rte_memeq_timingsafe(digest, vector.tag, vector.tag_len)` instead. Note the inverted return semantics: `rte_memeq_timingsafe()` returns true on equal, whereas `memcmp()` returns 0 on equal.

Similar pattern at line 1126 (GMAC validation):
```c
(vector.tag_len != 0 && memcmp(digest, vector.tag, vector.tag_len) != 0)
```

And in `validate_aead_vector()` at line 892 (AEAD):
```c
(vector->tag_len != 0 && memcmp(digest, vector->tag, vector->tag_len) != 0)
```

Replace all `memcmp()` calls on digests, tags, MACs, or authentication outputs with `rte_memeq_timingsafe()`. The code structure must change from:
```c
if (memcmp(a, b, len) != 0)
    fail();
```
to:
```c
if (!rte_memeq_timingsafe(a, b, len))
    fail();
```

**Potential resource leak on session-create failure in asymmetric paths:**

At line 1313 (`run_dsa_verify`), if `rte_cryptodev_asym_session_create()` fails and returns `< 0` or sets `session == NULL`, the function sets `ret = -ENOTSUP` and jumps to `out`, which attempts `rte_cryptodev_asym_session_free(env.dev_id, session);` on a NULL or invalid session pointer. The API documentation for `rte_cryptodev_asym_session_free()` does not guarantee NULL-safety. If the function was created but initialization failed, `session` could be a non-NULL invalid pointer.

The same pattern appears at:
- Line 1489 (`run_ecdh_ecpoint`)
- Line 1718 (`run_ecdsa_verify`)

In each case, `session` is initialized to `NULL` at the start, and the session-create call is:
```c
if (rte_cryptodev_asym_session_create(..., &session) < 0 || session == NULL) {
    ret = -ENOTSUP;
    goto out;
}
```
Then at `out:`:
```c
if (session != NULL)
    rte_cryptodev_asym_session_free(env.dev_id, session);
```

If the create call returns `< 0` but does not set `session` to NULL and does not initialize it, the conditional free at `out` could call free on an uninitialized pointer. However, the code initializes `session = NULL` at declaration, so this is safe. The check `if (session != NULL)` before free is correct. No leak on this path.

Re-examining: the pattern is safe. `session` is initialized to NULL, and the free is guarded. The session-create failure does not leak because the session was never created. Not an error.

---

### WARNINGS

**New public API `dpdk-wycheproof_validation` introduced without `__rte_experimental` or versioning:**

The application `dpdk-wycheproof_validation` is not a library API, so this guideline does not apply. Applications do not export symbols or require experimental tagging. No issue.

**Missing functional tests for Wycheproof validation logic:**

The guidelines require that new applications have tests. `app/wycheproof_validation/` is a test application itself (it validates PMDs against test vectors), so requiring it to have its own functional tests would be circular. This is appropriate as-is. No issue.

**Hardcoded hash algorithm table could use a lookup function:**

At lines 1166-1183 and 1935-1958, the `map_sha()` and `map_curve()` functions use a chain of `strcmp()` calls to map string names to enum values. This is acceptable for a small set of values, but a table-driven approach would be more maintainable. This is a code-quality suggestion, not a compliance issue. Informational only.

**DSA/ECDH/ECDSA validation silently requires a separate hash device:**

The code at lines 263-290 searches for a separate hash-capable device when the target device does not advertise SHA256 auth capability. If no such device is found, `env.hash_dev_id` remains set to `env.dev_id`, and DSA/ECDSA vectors that call `compute_hash()` will fail with `-ENOTSUP` and be reported as "PMD capability" skipped.

This is correct behavior, but the user gets no early warning that the PMD they selected will skip all DSA/ECDSA vectors due to missing hash support. Consider adding a startup message when the hash device differs from the target device, or when no hash device is found and the user is running with a PMD that advertises asymmetric support but no SHA256. This is an Info-level observation, not an error.

**`env.hash_dev_own` cleanup could fail silently:**

At line 325, the cleanup path calls:
```c
if (env.hash_dev_own) {
    rte_cryptodev_stop(env.hash_dev_id);
    rte_cryptodev_close(env.hash_dev_id);
}
```
Neither `rte_cryptodev_stop()` nor `rte_cryptodev_close()` have return values checked. If either fails, the error is silently ignored. This is acceptable in a cleanup path where the application is about to exit, but could leave the device in an inconsistent state. Best practice would be to log failures here, but it is not an error to skip the check in an exit path. Informational only.

**`rte_pause()` busy-wait in `dequeue_one()` could be more efficient:**

At line 519, `dequeue_one()` busy-waits with `rte_pause()` until a completion is dequeued or the deadline is reached. This consumes CPU cycles. A more efficient approach would be to use a longer sleep between polls (e.g., `usleep(1)` or `rte_delay_us_block(1000)`), trading latency for CPU efficiency. However, in a test application where latency matters and the device is expected to complete quickly, this pattern is acceptable. Informational only.

---

### INFO

**Pre-existing patterns not introduced by this patch:**

None. All code in patch 2/2 is new.

**Debugging output controlled by `--debug` flag:**

The application correctly gates verbose output behind the `env.debug` flag. This is good practice and does not require a finding.

**Use of `rte_memeq_timingsafe()` is available since DPDK 25.03:**

The function `rte_memeq_timingsafe()` was added in a recent release. Verify that the target DPDK version supports it before using. If this code is intended to backport to an earlier release, use `OPENSSL_memcmp()` or a similar constant-time comparison from the cryptography library the PMD depends on. For mainline DPDK, `rte_memeq_timingsafe()` is correct.

---

## Summary

### Pass 1: Correctness bugs included
All identified correctness bugs are listed above with line numbers and code quotes.

### Pass 2: Style/process false positives removed
No style or process items were determined to be correct and then listed as errors. All items above are actual issues.

### Pass 3: Evidence verification
Every finding quotes the line or describes the exact location in the patch. No findings rest on code not present in the patch.

---

Review-Result: ERROR