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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Analysis

This is a 2-patch series that moves the FIPS validation application from `examples/` to `app/` (patch 1/2) and adds a new Wycheproof validation application (patch 2/2).

### Patch 1/2: Move FIPS validation

This is a pure move with documentation updates. The files are renamed but their content is unchanged (100% similarity). The key changes are:
- Move from `examples/fips_validation/` to `app/fips_validation/`
- Update MAINTAINERS, meson.build, and documentation references
- Rename the guide from `fips_validation.rst` to `crypto_validation.rst`

No code changes, so correctness review focuses on whether references are updated correctly and the new structure is consistent.

### Patch 2/2: Add Wycheproof validation

This adds a new application that validates cryptodev implementations against Wycheproof JSON test vectors. The application is ~2100 lines of new code in `app/wycheproof_validation/main.c`, covering AEAD (GCM, CCM, ChaCha20-Poly1305, SM4-GCM), MAC (HMAC-*, AES-GMAC, AES-CMAC), and asymmetric algorithms (DSA, ECDH, ECDSA).

---

## ERRORS

### Error 1: Key material not zeroed before free (app/wycheproof_validation/main.c:396)

**Line 396:**
```c
rte_free_sensitive(vector->key);
```

The `free_vector` function uses `rte_free_sensitive()` for the key, which is correct. However, examining the `parse_vector` and `parse_mac_vector` functions that allocate the key (lines 426-499), the key is allocated with plain `rte_malloc()`:

**Line 446 (within `decode_hex`):**
```c
decoded = rte_malloc(NULL, *length, 0);
```

This is called for all fields including the key. When `decode_hex` fails partway through parsing a vector, the error path at line 491 calls `free_vector(&vector)`, which will call `rte_free_sensitive(vector->key)` on a key that was allocated with `rte_malloc()`. This is correct.

However, there is a second concern: the error paths in the various `run_*` functions. For example, in `run_aead` (lines 642-765), when constructing the xform:

**Lines 661-665:**
```c
xform.aead.key.data = vector->key;
xform.aead.key.length = vector->key_len;
```

The key pointer from the vector is passed directly to the xform. When the operation completes, the key is still owned by the vector and will be freed by `free_vector`. This is correct.

But in `run_hmac` (lines 772-868), `run_gmac` (lines 870-961), `run_dsa_verify` (lines 1232-1316), and `run_ecdsa_verify` (lines 1745-1843), the same pattern applies: the key is referenced, not copied, so `rte_free_sensitive` in `free_vector` is the only free. This is correct.

Wait--on closer inspection of `run_ecdh_ecpoint` (lines 1586-1714), there is a different issue:

**Lines 1640-1641:**
```c
if (normalize_operand(priv, priv_len, priv_buf, bytesize) != 0)
    return -EINVAL;
```

The private key is copied into a local buffer `priv_buf` which is stack-allocated at line 1607:

**Line 1607:**
```c
uint8_t priv_buf[MAX_EC_BYTESIZE];
```

This buffer holds a copy of the private key (the ECDH secret). The function returns at line 1714 without zeroing `priv_buf`. The `normalize_operand` function (lines 1530-1543) copies the key material into this buffer, and the buffer is then used in the xform and operation, but when `run_ecdh_ecpoint` returns, the stack frame is reclaimed without the buffer being wiped.

**Fix:** Add `rte_memzero_explicit(priv_buf, sizeof(priv_buf));` before every return in `run_ecdh_ecpoint` (lines 1641, 1656, 1671, 1678, 1683, 1709, and the normal exit at 1714). Same issue in `run_ecdsa_verify` with the `wx_buf` and `wy_buf` that are derived from the public key (lines 1789-1790), though those are public and do not need wiping. However, `priv_buf` in `run_ecdh_ecpoint` is derived from the private key and must be wiped.

Re-checking: `wx_buf` and `wy_buf` in `run_ecdsa_verify` hold public key components (from `group->wx` and `group->wy`, which come from the JSON's "publicKey" object). These are public and do not need wiping. Only `priv_buf` in `run_ecdh_ecpoint` is secret.

**Correct statement of the issue:**

In `run_ecdh_ecpoint` (lines 1586-1714), the private key is copied into a stack-allocated buffer `priv_buf` (line 1607) via `normalize_operand` at line 1640. This buffer is used in the xform and operation but is not wiped before the function returns. The buffer contains secret key material and must be zeroed with `rte_memzero_explicit(priv_buf, sizeof(priv_buf));` before every return statement in the function (lines 1641, 1656, 1671, 1678, 1683, 1709, 1714).

---

### Error 2: Missing queue configuration and start for hash device (app/wycheproof_validation/main.c:308-326)

**Lines 308-326:**
```c
if (rte_cryptodev_sym_capability_get(env.dev_id, &hash_idx) == NULL) {
    for (d = 0; d < count; d++) {
        struct rte_cryptodev_qp_conf hash_qp = {
            .nb_descriptors = NUM_QP_DESCRIPTORS,
            .mp_session = NULL,
        };

        if (d == env.dev_id)
            continue;
        if (rte_cryptodev_sym_capability_get(d, &hash_idx) == NULL)
            continue;
        if (rte_cryptodev_configure(d, &config) < 0)
            continue;
        hash_qp.mp_session = env.session_pool;
        if (rte_cryptodev_queue_pair_setup(d, 0, &hash_qp,
                rte_socket_id()) < 0 ||
                rte_cryptodev_start(d) < 0) {
            rte_cryptodev_close(d);
            continue;
        }
```

The code configures, sets up queue pair 0, and starts device `d` if it has SHA256 capability. If any of these fail, it calls `rte_cryptodev_close(d)` and continues to the next device. However, if `rte_cryptodev_start(d)` fails, the device is in a configured and queue-pair-setup state but not started, and `rte_cryptodev_close` is called on it. This is correct: `rte_cryptodev_close` can be called on a configured but not-started device.

Wait--no, there's a different issue. If `rte_cryptodev_configure(d, &config)` succeeds but `rte_cryptodev_queue_pair_setup` fails, the code calls `rte_cryptodev_close(d)` and continues. But if the main device (env.dev_id) is still in the list and `d != env.dev_id`, and later we fail to find any suitable hash device, the main device has already been started at line 274:

**Line 274:**
```c
ret = rte_cryptodev_start(env.dev_id);
```

So the main device is started. If we fail to find a hash device, `env.hash_dev_id` is set to `env.dev_id` at line 295:

**Lines 295-296:**
```c
env.hash_dev_id = env.dev_id;
env.hash_dev_own = false;
```

This means the hash device is the same as the main device, and `env.hash_dev_own` is false, so `app_uninit` (lines 331-343) will not call `rte_cryptodev_close` on it. This is correct because the main device is closed separately.

So what's the error? Re-reading: the hash device search loop (lines 308-326) iterates over all devices, and for each one that has SHA256 capability, it configures, sets up a queue pair, and starts it. If all three succeed, it breaks and sets `env.hash_dev_id = d` and `env.hash_dev_own = true`. If any fail, it closes the device and continues.

The potential issue is: what if device `d` is configured but not started, and then `rte_cryptodev_close(d)` is called? According to the DPDK documentation, `rte_cryptodev_close` can be called on a device in any state. So this is not an error.

Wait--I'm looking for an actual error. Let me re-examine the hash device acquisition. The code at line 308 checks if the main device has SHA256 capability. If it does not, it searches for another device. If it finds one and all setup steps succeed, it sets `env.hash_dev_id = d` and `env.hash_dev_own = true` and breaks. If it fails to find any suitable device, `env.hash_dev_id` remains set to `env.dev_id` (from line 295) and `env.hash_dev_own` remains false.

The issue is: if the main device does not have SHA256 capability and we fail to find a suitable hash device, `env.hash_dev_id` is set to `env.dev_id` (line 295), and then `compute_hash` (lines 1126-1196) will try to use that device for SHA256 operations, which will fail because the device does not support it. The `compute_hash` function checks capability at lines 1152-1155:

**Lines 1152-1155:**
```c
capability = rte_cryptodev_sym_capability_get(env.hash_dev_id, &index);
if (capability == NULL || rte_cryptodev_sym_capability_check_auth(capability,
        0, digest_len, 0) != 0)
    return -ENOTSUP;
```

So `compute_hash` will return `-ENOTSUP`, and the DSA/ECDSA vectors that depend on it will be skipped. This is correct behavior: if no device supports SHA256, asymmetric signature verification that requires hashing is skipped.

So that's not an error either. Let me look for a real one.

Actually, wait--lines 308-326 only configure and start the hash device if it is not the main device (line 311: `if (d == env.dev_id) continue;`). So we never double-configure the main device. And if we find a suitable hash device that is not the main device, we configure, set up queue pair 0, and start it. The question is: does the main device already have queue pair 0 set up? Yes, at lines 269-271:

**Lines 269-271:**
```c
ret = rte_cryptodev_queue_pair_setup(env.dev_id, 0, &queue_pair,
    rte_socket_id());
```

So the main device has queue pair 0 set up. If we then find a different hash device and set up its queue pair 0, that's fine. Each device has its own queue pair 0.

I don't see a resource leak or configuration error here. Let me move on.

---

Actually, re-reading the error priorities in AGENTS.md: I should focus on correctness bugs at >=50% confidence. The `priv_buf` not being zeroed is a clear correctness bug (Error 1 above). Let me continue looking for others.

---

### Error 3: Non-constant-time digest comparison (app/wycheproof_validation/main.c:718, 857, 950)

**Line 718 (in `validate_aead_vector`):**
```c
if (ret != 0 || status != RTE_CRYPTO_OP_STATUS_SUCCESS ||
        (vector->ct_len != 0 &&
         memcmp(output, vector->ct, vector->ct_len) != 0) ||
        (vector->tag_len != 0 &&
         memcmp(digest, vector->tag, vector->tag_len) != 0))
    goto failed;
```

This compares the computed tag (`digest`) against the expected tag (`vector->tag`) using `memcmp`. The expected tag comes from the Wycheproof JSON test vector, which is attacker-controlled (the test vectors include both valid and invalid signatures/tags). Using `memcmp` to verify an authentication tag leaks timing information and allows an attacker to perform a timing attack to determine if their forged tag is correct.

**However**, this code is in the validation tool, not in the PMD. The validation tool is comparing the PMD's output against a known-good test vector. The comparison is happening in the tool, not in the cryptographic operation itself. The PMD has already either succeeded or failed its own constant-time comparison (inside the `RTE_CRYPTO_AEAD_OP_DECRYPT` operation), and the tool is just checking if the PMD's output matches the expected result.

So this is not a timing side-channel in the PMD; it's a comparison in the test harness. The concern about constant-time comparison applies to the PMD's implementation, not to the test tool. The test tool is not processing attacker-supplied data in a security-critical context; it's validating the PMD's correctness.

**Conclusion:** Not an error. The `memcmp` usage in the validation tool is acceptable because the tool is not performing cryptographic verification; it's testing that the PMD did.

The same reasoning applies to lines 857 (HMAC tag comparison) and 950 (GMAC tag comparison).

---

Let me continue looking for correctness bugs.

---

### Error 4: Missing error check on `json_array_foreach` iteration (multiple locations)

The `json_array_foreach` macro is used throughout the code (e.g., lines 987, 996, 1050, 1061, 1354, 1365, etc.). This macro does not return an error code; it simply iterates over the array. If the JSON structure is malformed (e.g., `"testGroups"` is not an array), the code checks this with `!json_is_array(groups)` before the loop and returns `-EINVAL`. This is correct.

Not an error.

---

### Error 5: `decode_hex` allocates with `rte_malloc` but sensitive data may need `rte_zmalloc` or special handling

**Line 446 (in `decode_hex`):**
```c
decoded = rte_malloc(NULL, *length, 0);
```

The `decode_hex` function is called for all fields: keys, IVs, AADs, messages, ciphertexts, tags, etc. Keys are sensitive and should be allocated with care. However, `rte_malloc` is not inherently wrong for keys; the issue is whether the memory is zeroed before being freed.

The code uses `rte_free_sensitive(vector->key)` in `free_vector` (line 396), which wipes the memory before freeing it. This is correct.

However, there's a subtle issue: what if `decode_hex` is called for a key, the allocation succeeds, but then a later `decode_hex` call (for IV, AAD, etc.) fails? The error path at line 491 calls `free_vector(&vector)`, which will free the partially-initialized vector. The key will be freed with `rte_free_sensitive`, which is correct. The other fields will be freed with plain `rte_free`, which is correct because they are not sensitive (or in the case of IVs, they are usually not secret).

Wait--actually, let me check the `decode_hex` error path. At line 452:

**Lines 448-453:**
```c
if (high < 0 || low < 0) {
    rte_free(decoded);warning: review has no Review-Result line, severity inferred from prose (response may be truncated)