|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

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

This patch moves the FIPS validation sample from `examples/` to `app/` and updates documentation references.

### ERRORS

**Error 1: Missing Windows check already present**
The patch removes `allow_experimental_apis = true` from `app/fips_validation/meson.build` but does not verify that no experimental APIs are used in the moved code. If the FIPS validation application calls any `__rte_experimental` functions, this will cause build failures.
```meson
# BAD - removed without verification
-allow_experimental_apis = true
```
The experimental API allowance must be retained if any experimental APIs are called, or the code must be audited to confirm none are used.

**Error 2: Release notes updated for wrong release**
The patch modifies `doc/guides/rel_notes/release_22_11.rst`, which is a historical release from 2022. Release notes are immutable after publication and must not be changed in new patches. This appears to be updating an old reference rather than adding a new entry.
```rst
# At doc/guides/rel_notes/release_22_11.rst line 273
-  See the :doc:`/sample_app_ug/fips_validation` for more details.
+  See the :doc:`/sample_app_ug/crypto_validation` for more details.
```
Historical release notes must not be modified. If this is a backport, it belongs in a separate stable-branch patch.

### WARNINGS

**Warning 1: Makefile deletion without release notes**
The patch deletes `examples/fips_validation/Makefile` (the legacy build system support), which is a user-facing change. This should be documented in release notes for the current release (26.11 based on patch 2/2), but no such entry appears in this patch.

**Warning 2: Missing app registration**
The patch adds `'fips_validation'` to `app/meson.build` but does not document this change in the current release notes. Moving an example to an application is a significant change that users should be notified about.

**Warning 3: Documentation does not match new app location**
The RST file is renamed from `fips_validation.rst` to `crypto_validation.rst` and the internal structure is changed (adding sections), but the patch does not update the build command from `examples/` to `app/`:
```rst
+    From the DPDK source tree, build the application with Meson:
+
+    .. code-block:: console
+
+       meson setup build -Denable_apps=fips_validation
+       meson compile -C build
```
This is correct for the new location, so no issue here. However, the old `make` instructions are removed without noting this is a breaking change for users on the legacy build system.

---

## Patch 2/2: app: add Wycheproof validation app

This patch adds a new cryptodev validation application using Wycheproof JSON test vectors.

### ERRORS

**Error 1: Key material not zeroed before free (multiple instances)**
The code allocates and copies private keys but does not zero them before freeing:

In `run_ecdh_ecpoint()` at line 1641:
```c
uint8_t priv_buf[MAX_EC_BYTESIZE];
...
if (normalize_operand(priv, priv_len, priv_buf, bytesize) != 0)
    return -EINVAL;
...
xform.ec.pkey.data = priv_buf;
...
out:
    /* The private key was copied into the stack buffer; wipe it. */
    rte_memzero_explicit(priv_buf, sizeof(priv_buf));
```
This case is correct. However, the caller in `process_ecdh_ecpoint()` allocates the original `priv` buffer and frees it with `rte_free_sensitive()` (line 1724), which is also correct. No issue here.

**Error 2: Resource leak on error path in `run_aead()`**
In `run_aead()`, if the second `run_aead()` call in `validate_aead_vector()` fails after the first succeeded, the first call's allocated `output` and `digest` are leaked:
```c
static int
validate_aead_vector(...)
{
    uint8_t *output = NULL;
    uint8_t *digest = NULL;
    ...
    if (strcmp(vector->result, "valid") == 0) {
        ret = run_aead(vector, algorithm, RTE_CRYPTO_AEAD_OP_ENCRYPT, &output, &digest,
            &status);
        ...
        rte_free(output);
        rte_free(digest);
        output = NULL;
        digest = NULL;
        ret = run_aead(vector, algorithm, RTE_CRYPTO_AEAD_OP_DECRYPT, &output, &digest,
            &status);
        if (ret == -ETIMEDOUT)
            return -ETIMEDOUT;  /* LEAK: first output/digest not freed */
```
The early `return -ETIMEDOUT` on line 896 does not free the first allocation. The variables are reset to NULL after the first free, so the cleanup at the end won't catch them.

**Error 3: Missing error check on `rte_zmalloc()` in `run_aead()`**
```c
aad = rte_zmalloc(NULL,
    RTE_ALIGN_CEIL(vector->aad_len + AES_CCM_AAD_OFFSET, 16), 0);
if (aad == NULL)
    goto out;
```
This check is present. No issue.

**Error 4: `rte_malloc_virt2iova()` on stack buffer**
In `run_ecdh_ecpoint()`, the code sets physical addresses for stack-allocated buffers:
```c
uint8_t priv_buf[MAX_EC_BYTESIZE];
...
op->asym->ecdh.priv_key.data = priv_buf;
op->asym->ecdh.priv_key.phys_addr = rte_malloc_virt2iova(priv_buf);
```
Wait--this line is not present in the code. The actual code at line 1654 does not set a `phys_addr` for `priv_key`. The structure is passed in the xform, not the op. Let me re-check.

Actually, looking at the ECDH code more carefully:
```c
xform.ec.pkey.data = priv_buf;
xform.ec.pkey.length = bytesize;
...
op->asym->ecdh.priv_key.data = priv_buf;
op->asym->ecdh.priv_key.length = bytesize;
```
The `rte_crypto_asym_op` structure does not have a `phys_addr` field for the private key--it's a data pointer only. The PMD will handle address translation if needed. No issue here.

**Error 5: Device wedge handling in `dequeue_one()` leaks in-flight state**
The comment at each timeout site says "Abort without freeing in-flight state; teardown stops the device." The code correctly returns `-ETIMEDOUT` and the main loop will call `app_uninit()`, which stops the device. However, the calling function may still hold allocated buffers (e.g., `output`, `digest`, `mbuf`, `op`) that are not freed on the timeout path.

For example, in `run_aead()`:
```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;
```
The function returns immediately without reaching the `out:` label, so `aad`, `mbuf`, `operation_op`, and `session` are leaked. The `op` itself is owned by the PMD and cannot be freed, but the other resources are not in-flight and should be cleaned up.

However, this is a device-wedge condition--an exceptional failure where the PMD has stopped responding. The program will exit shortly after. The comment indicates this is intentional: the authors chose not to attempt cleanup when the device is in an unknown state. This is a design choice rather than a bug, though it could be improved. I will not flag it as an error since the rationale is documented.

**Error 6: Verification in `validate_aead_vector()` missing timeout cleanup**
As noted in Error 2, the `-ETIMEDOUT` path does not free the first call's allocations. This is a correctness bug.

### WARNINGS

**Warning 1: Missing release notes entry for the jansson dependency**
The new app requires the Jansson library, which is an external dependency. The meson.build checks for it:
```meson
if not dpdk_conf.has('RTE_HAS_JANSSON')
    build = false
    message('Skipping wycheproof_validation: Jansson is required')
```
The release notes mention the new application but do not document that users must install Jansson to build it. This should be noted in the "New Features" or "Known Issues" section.

**Warning 2: No tests added**
The guidelines state "New API functions must have hooks in `app/testpmd` and tests in the functional test suite." This is an application, not a library, so testpmd is not applicable. However, there are no automated tests for the application itself. The app is a test tool, so this is acceptable, but a note about how to validate the app's own behavior would be useful.

**Warning 3: `MAX_EC_BYTESIZE` constant without explanation**
The constant `MAX_EC_BYTESIZE` is 66, which corresponds to secp521r1 (66 bytes for a 521-bit curve field element). A comment explaining this would improve maintainability:
```c
#define MAX_EC_BYTESIZE 66  /* secp521r1: ceil(521 / 8) = 66 bytes */
```

### INFO

**Info 1: Pre-existing: FIPS validation app does not zero keys on all error paths**
This patch does not touch the FIPS validation code, but as a note for future work: the moved FIPS code allocates keys with `rte_malloc()` and frees with `rte_free()` rather than `rte_free_sensitive()` in many places. This is a pre-existing issue in the code being moved, not introduced by this patch.

**Info 2: Hardcoded device IDs in environment struct**
The `app_env` struct stores `dev_id` and `hash_dev_id` as `uint8_t`. While cryptodev IDs are typically `uint8_t`, the application could be more defensive by validating the range in `parse_args()`, which it does:
```c
if (parse_uint32(optarg, &value) != 0 || value > UINT8_MAX ||
        !rte_cryptodev_is_valid_dev(value))
    return -EINVAL;
```
No issue.

---

## Summary of Findings

### Patch 1/2

**Errors:**
1. `allow_experimental_apis` removed without verifying no experimental APIs are used
2. Historical release notes (22.11) modified instead of adding entry to current release

**Warnings:**
1. Makefile deletion not documented in release notes
2. Example-to-app move not documented in release notes

### Patch 2/2

**Errors:**
1. Resource leak in `validate_aead_vector()` when second `run_aead()` returns `-ETIMEDOUT` (first call's allocations not freed)

**Warnings:**
1. Jansson dependency not documented in release notes
2. `MAX_EC_BYTESIZE` constant lacks explanatory comment

---

Review-Result: ERROR