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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-17

# DPDK Patch Review: Wycheproof Validation Example

## Summary

This patch adds a comprehensive Wycheproof JSON test vector validation example for DPDK cryptodev PMDs. The code is generally well-structured, but there are several correctness issues and style violations that must be addressed.

---

## Errors (Must Fix)

### 1. Resource Leak on Hash Device Failure Path

**File:** `examples/wycheproof_validation/main.c`  
**Function:** `app_init()`  
**Lines:** ~247-282

The hash device discovery loop configures, sets up queue pairs, and starts devices (`d`), but if a later device succeeds, the earlier failed devices remain configured/started without cleanup. When `env.hash_dev_own = true` is set for device `d`, all previously attempted devices that were partially initialized are leaked.

**Impact:** Resource leak of configured cryptodev instances.

**Fix:**
```c
for (d = 0; d < count; d++) {
    struct rte_cryptodev_qp_conf hash_qp = { 128, 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_close(d);  /* Clean up after configure */
        continue;
    }
    if (rte_cryptodev_start(d) < 0) {
        rte_cryptodev_close(d);  /* Clean up after queue setup */
        continue;
    }
    env.hash_dev_id = d;
    env.hash_dev_own = true;
    break;
}
```

---

### 2. Missing Error Propagation in `compute_hash()`

**File:** `examples/wycheproof_validation/main.c`  
**Function:** `compute_hash()`  
**Lines:** ~1177

When `rte_cryptodev_sym_session_create()` returns `NULL`, the function jumps to `out:` with `ret = -ENOMEM`. However, if the session creation failed due to unsupported parameters (after the capability check passed), this should be distinguished from a true memory allocation failure. The current code correctly returns `-ENOMEM`, but the caller in `run_dsa_verify()` / `run_ecdsa_verify()` treats any non-zero return as a fatal error, which may be too harsh.

**Impact:** Error path handling is correct but could be clearer. This is a minor issue; the current behavior (returning `-ENOMEM` and having the caller handle it) is acceptable. However, the comment in the code should clarify that session creation failure after capability check indicates a PMD issue, not lack of memory.

**Recommendation (not an error):** Add a comment clarifying the expected failure mode.

---

### 3. Undefined Behavior: Cast Away `const` in Crypto Operations

**File:** `examples/wycheproof_validation/main.c`  
**Functions:** Multiple (`run_dsa_verify`, `run_ecdsa_verify`, `run_ecdh_ecpoint`)  
**Examples:**
- Line ~1262: `op->asym->dsa.r.data = (uint8_t *)(uintptr_t)r;`
- Line ~1264: `op->asym->dsa.s.data = (uint8_t *)(uintptr_t)s;`
- Line ~1500: `op->asym->ecdh.pub_key.x.data = (uint8_t *)(uintptr_t)pub_x;`
- Line ~1653: `op->asym->ecdsa.r.data = (uint8_t *)(uintptr_t)r;`

**Issue:** The code casts away `const` from read-only buffers (`r`, `s`, `pub_x`, `pub_y`) when assigning to `rte_crypto_op` fields. While the asymmetric crypto API may not actually modify these buffers, casting away `const` invites undefined behavior if the PMD implementation attempts to write to them.

**Impact:** Potential undefined behavior if the PMD writes to input buffers. At minimum, this violates strict aliasing rules and const correctness.

**Fix:** If the DPDK API requires non-const pointers for input data, the buffers should be allocated as non-const from the start. If the API accepts const pointers, remove the cast. If the API is incorrectly typed (should accept `const uint8_t *` for input-only fields), this is an API design issue, but the example should not work around it by casting away const.

**Recommended approach:**
- Verify the `rte_crypto_asym_op` API contract for these fields
- If they are truly input-only, allocate mutable copies before assigning
- Do not use `(uintptr_t)` to hide const-cast warnings

Example fix:
```c
/* Allocate mutable copies for API requirements */
uint8_t *r_buf = rte_malloc(NULL, r_len, 0);
uint8_t *s_buf = rte_malloc(NULL, s_len, 0);
if (r_buf == NULL || s_buf == NULL) {
    rte_free(r_buf);
    rte_free(s_buf);
    ret = -ENOMEM;
    goto out;
}
memcpy(r_buf, r, r_len);
memcpy(s_buf, s, s_len);
op->asym->dsa.r.data = r_buf;
op->asym->dsa.r.length = r_len;
op->asym->dsa.s.data = s_buf;
op->asym->dsa.s.length = s_len;
/* ... free r_buf, s_buf in cleanup path ... */
```

---

### 4. Buffer Overflow Risk in `decode_hex()`

**File:** `examples/wycheproof_validation/main.c`  
**Function:** `decode_hex()`  
**Lines:** ~358-364

The check `hex_length / 2 > UINT32_MAX` is insufficient on 64-bit systems where `size_t` can exceed `uint32_t` range. If `hex_length == UINT32_MAX * 2 + 2`, the division `hex_length / 2 == UINT32_MAX + 1`, which exceeds `UINT32_MAX`, but the prior check `(hex_length & 1) != 0` passes. The subsequent cast to `uint32_t` truncates.

**Impact:** Integer overflow could lead to undersized allocation if `hex_length` is crafted (though unlikely in practice with JSON parsing).

**Fix:**
```c
if ((hex_length & 1) != 0)
    return -EINVAL;
if (hex_length > UINT32_MAX * 2)  /* Check before division */
    return -EINVAL;

*length = hex_length / 2;
```

---

### 5. Missing Cleanup in `run_aead()` on `mbuf` Allocation Failure

**File:** `examples/wycheproof_validation/main.c`  
**Function:** `run_aead()`  
**Lines:** ~572-575

If `rte_pktmbuf_alloc()` returns `NULL`, the function jumps to `out:` but `operation_op` has already been allocated and is not freed on this path (it's only freed if `operation_op != NULL`, which it is at this point).

**Wait, checking again:** The cleanup path at `out:` has `rte_crypto_op_free(operation_op)` unconditionally. `rte_crypto_op_free()` handles `NULL` gracefully (it's a standard DPDK API that accepts `NULL`). So this is **not** an error.

**Correction:** Not an issue. `rte_crypto_op_free(NULL)` is safe.

---

### 6. Potential Integer Overflow in `normalize_operand()`

**File:** `examples/wycheproof_validation/main.c`  
**Function:** `normalize_operand()`  
**Lines:** ~1406-1415

The expression `bytesize - value_len` could underflow if `value_len > bytesize` after stripping leading zeros. The check `if (value_len > bytesize)` catches this, so this is **not** an error.

**Correction:** Not an issue. The function is correctly implemented.

---

### 7. Missing Bounds Check on `env.mbuf_data_room` Parameter

**File:** `examples/wycheproof_validation/main.c`  
**Function:** `parse_args()`  
**Lines:** ~151-157

The check ensures `mbuf_data_room >= RTE_PKTMBUF_HEADROOM` and `<= UINT16_MAX`, which is correct for mbuf sizing. However, the subsequent use in `run_aead()` line ~579 checks `input_len > rte_pktmbuf_tailroom(mbuf)`, which depends on the actual allocated mbuf's tailroom, not just the requested data room size. This is **correct** -- the tailroom check is the right place to catch oversized payloads.

**Correction:** Not an error. The implementation is safe.

---

### 8. Race Condition in `dequeue_one()` on Shared `completed` Variable?

**File:** `examples/wycheproof_validation/main.c`  
**Function:** `dequeue_one()`  
**Lines:** ~471-482

The `completed` variable is local to the function, and the application is single-threaded (no evidence of multiple threads using the same queue pair). The timeout logic using `rte_get_timer_cycles()` is safe for single-threaded polling.

**Correction:** Not a race condition. This is single-threaded code.

---

## Warnings (Should Fix)

### 1. Hardcoded Magic Numbers for Digest Sizes

**File:** `examples/wycheproof_validation/main.c`  
**Function:** `map_sha()`, `map_curve()`  
**Lines:** ~1052-1073, ~1292-1307

Digest sizes (20, 28, 32, 48, 64) and EC curve byte sizes (28, 32, 48, 66) are hardcoded. While these are fixed by the algorithm standards, using named constants or `RTE_CRYPTO_AUTH_*` / `RTE_CRYPTO_EC_*` related macros would improve readability.

**Suggestion:** Define macros at file scope:
```c
#define SHA1_DIGEST_LEN 20
#define SHA224_DIGEST_LEN 28
/* ... */
```

---

### 2. Overly Long Functions

**Functions:** `process_aead()`, `process_hmac()`, `process_gmac()`, `run_aead()`, `run_ecdh_ecpoint()`, etc.

Several functions exceed 100 lines and contain deeply nested control structures. Consider refactoring into smaller helper functions for readability and maintainability.

---

### 3. Missing Release Notes Entry

The patch adds a new example application but does not include a release notes update. According to AGENTS.md, new applications should have release notes.

**Required:** Add a section to `doc/guides/rel_notes/release_<version>.rst` documenting the new example.

---

### 4. Inconsistent Error Logging

Some functions use `if (env.debug)` with `printf()`, others use `debug_vector()` or `debug_file()`. The inconsistency makes it harder to grep for all debug output points.

**Suggestion:** Standardize on a single debug logging pattern, perhaps wrapping all debug output through a single macro or function.

---

### 5. `rte_malloc()` Without Size Validation

Multiple calls to `rte_malloc()` / `rte_zmalloc()` allocate buffers with sizes derived from user-controlled JSON input (e.g., `vector->tag_len`, `vector->msg_len`). While the current checks (`> UINT32_MAX`, `> rte_pktmbuf_tailroom()`) provide some bounds, there's no explicit cap on individual allocation sizes beyond `UINT32_MAX`.

**Suggestion:** Add a sanity check for maximum reasonable sizes (e.g., 64KB) for individual buffers to prevent memory exhaustion from malformed vectors.

---

### 6. Missing Jansson Version Check

The code depends on Jansson but does not verify the minimum required version. If Jansson <2.7 is present, the `json_array_foreach` macro may not be available.

**Suggestion:** Document the minimum Jansson version in the README or build files, or add a compile-time check.

---

### 7. Potential `NULL` Dereference After `json_object_get()`

**Example:** Line ~1219 in `process_dsa_p1363()`:
```c
sha = json_string_value(json_object_get(group, "sha"));
if (sha == NULL || map_sha(sha, &dsa.hash, &dsa.digest_len) != 0) {
```

If `json_object_get(group, "sha")` returns `NULL`, `json_string_value(NULL)` is called. According to Jansson documentation, `json_string_value(NULL)` returns `NULL`, so this is safe. However, the pattern is repeated throughout and could be clearer.

**Suggestion:** Add an explicit `NULL` check after `json_object_get()` before passing to `json_string_value()` for clarity.

---

## Style and Process Issues

### 1. Function Naming: `run_*` vs `process_*`

The codebase mixes `run_aead()` (executes a single operation) and `process_aead()` (iterates over vectors). Consider a more consistent naming scheme: `execute_*` for single operations, `process_*` for batch processing.

---

### 2. Inconsistent Use of `rte_malloc()` Alignment

Some calls use `RTE_CACHE_LINE_SIZE` alignment (e.g., line ~582 for digest), others use `0` (e.g., line ~371 for hex decoding). For crypto operations, DMA-safe alignment is preferred.

**Suggestion:** Use `RTE_CACHE_LINE_SIZE` consistently for all crypto-related buffers.

---

### 3. Missing Copyright Holder

The SPDX headers in the new files specify `Copyright(c) 2026` without a copyright holder (e.g., "Intel Corporation"). The patch is submitted by an Intel employee (`kai.ji at intel.com`), so the copyright holder should likely be Intel.

**Note:** Per AGENTS.md, copyright format is not subject to AI review, but this is a notable omission for the maintainer to address.

---

### 4. Documentation: Missing Algorithm Support Table

The `wycheproof_validation.rst` file lists algorithms in prose but would benefit from a table mapping Wycheproof test families to DPDK algorithm enums for clarity.

---

### 5. `MAX_EC_BYTESIZE` and `MAX_IV_LEN` Definitions

These constants are defined at file scope but are specific to the Wycheproof test suite's expected ranges (e.g., secp521r1 = 66 bytes). Consider documenting *why* these values were chosen.

---

### 6. `env` Global Variable

The `env` global is acceptable for an example application but makes the code harder to test or reuse. Consider passing `env` as a parameter to functions if the example is intended as a reference implementation.

---

### 7. Debug Output to `stdout` Instead of `stderr`

Debug messages and errors are printed to `stdout` via `printf()`. Standard practice is to use `stderr` for diagnostic output.

**Suggestion:** Use `fprintf(stderr, ...)` for debug and error messages.

---

### 8. Missing `--help` Option

The `usage()` function is only called on parse failure. Adding a `--help` / `-h` option would improve usability.

---

### 9. Makefile: `rmdir -p` Failure Ignored

Line 40 of the Makefile:
```make
test -d build && rmdir -p build || true
```

This silently ignores `rmdir` failure, which is fine, but the `|| true` is unnecessary given the `test -d` guard.

**Suggestion:**