|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

This patch series moves the FIPS validation application from `examples/` to `app/` and adds a new Wycheproof validation application. I will review both patches for correctness bugs, coding style compliance, API requirements, and adherence to DPDK guidelines.

---

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

This patch is primarily a code relocation with minimal changes. All moved files are identical renames.

### Code Changes

**app/fips_validation/meson.build (line 4):**
```meson
# meson file, for building this application as part of a main DPDK build.
```
Changed comment from "example" to "application" - appropriate.

**app/meson.build:**
Added `'fips_validation'` to the apps list.

**examples/meson.build:**
Removed `'fips_validation'` from examples list.

**Documentation updates:**
All reference changes correctly point to the new location and the renamed guide.

### Findings

None. This is a straightforward relocation with appropriate comment and reference updates. The patch does not introduce any code changes that could cause correctness bugs.

---

## PATCH 2/2: Add Wycheproof validation app

This patch adds a new cryptographic validation application. I will examine it for correctness bugs, resource management, coding style, and API compliance.

---

## ERRORS

### 1. Resource leak on early return from `parse_vector()`

**File:** `app/wycheproof_validation/main.c`  
**Lines:** 451-455

```c
ret = decode_hex(json_object_get(test, "key"), &vector->key, &vector->key_len);
if (ret != 0)
	goto error;
ret = decode_hex(json_object_get(test, "iv"), &vector->iv, &vector->iv_len);
if (ret != 0)
	goto error;
```

If any `decode_hex()` call after the first one fails, the previous allocations are leaked. For example, if the `iv` decode fails after `key` succeeded, `vector->key` is allocated but not freed by the error path in `parse_vector()`. The function calls `free_vector()` on error, which does free these fields, but the intermediate failure path between allocation steps does not guarantee cleanup.

**Impact:** Memory leak on malformed JSON vectors with partial field decoding failures.

**Fix:** The existing `goto error; ... error: free_vector(vector);` pattern is correct. On closer inspection, `free_vector()` zeros and frees all fields, so this is safe. However, the error label frees `vector` itself, and the function returns `ret` without ensuring all allocations up to that point are handled. The current implementation is actually correct - each allocation failure jumps to the error label, which calls `free_vector()`, and `free_vector()` uses `rte_free()` which is safe on NULL pointers. No error here upon re-inspection.

**Correction:** No error. `free_vector()` is safe on partially initialized `wycheproof_data`.

---

### 2. Use-after-free potential in `run_aead()` on `-ETIMEDOUT` return

**File:** `app/wycheproof_validation/main.c`  
**Lines:** 658-662

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

When `dequeue_one()` times out, the function returns `-ETIMEDOUT` without freeing `operation_op`, `mbuf`, `aad`, `*digest`, or calling `rte_cryptodev_sym_session_free()`. The comment states "abort without freeing in-flight state; teardown stops the device", acknowledging that the op is still owned by the PMD. However, `session`, `mbuf`, and `aad` are not in-flight - they are local resources that should be freed even on timeout.

**Impact:** Memory leak on device timeout. The session remains allocated, consuming resources from the session pool.

**Fix:** Free non-in-flight resources before returning `-ETIMEDOUT`:

```c
completed = dequeue_one(env.dev_id);
if (completed == NULL) {
	/* Device wedged: op still owned by the PMD. Do not free the op or
	 * mbuf attached to it, but clean up other local resources.
	 */
	rte_free(aad);
	rte_cryptodev_sym_session_free(env.dev_id, session);
	rte_free(*digest);
	*digest = NULL;
	return -ETIMEDOUT;
}
```

The same pattern appears in `run_hmac()` (line 752), `run_gmac()` (line 841), `compute_hash()` (line 1054), `run_dsa_verify()` (line 1154), `run_ecdh_ecpoint()` (line 1400), and `run_ecdsa_verify()` (line 1791) - all must be fixed.

---

### 3. Key material not zeroed in `run_ecdh_ecpoint()`

**File:** `app/wycheproof_validation/main.c`  
**Lines:** 1336-1337, 1408-1410

```c
uint8_t priv_buf[MAX_EC_BYTESIZE];
...
if (normalize_operand(priv, priv_len, priv_buf, bytesize) != 0)
	return -EINVAL;
```

The private key is copied into `priv_buf` and attached to the asymmetric operation. The function has multiple error paths that return without zeroing `priv_buf`. At line 1410, the function calls `rte_cryptodev_asym_session_free()` and returns, but `priv_buf` is not wiped.

**Impact:** Private key material remains on the stack after function return. An attacker with memory access (e.g., via another vulnerability or core dump) could recover the key.

**Fix:** Zero `priv_buf` on all exit paths:

```c
out:
	rte_free(out_x);
	rte_free(out_y);
	rte_crypto_op_free(op);
	if (session != NULL)
		rte_cryptodev_asym_session_free(env.dev_id, session);
	rte_memzero_explicit(priv_buf, sizeof(priv_buf));  /* Add this */
	return ret;
```

---

### 4. Key material not zeroed in `run_ecdsa_verify()`

**File:** `app/wycheproof_validation/main.c`  
**Lines:** 1740-1744, 1798-1801

```c
uint8_t wx_buf[MAX_EC_BYTESIZE];
uint8_t wy_buf[MAX_EC_BYTESIZE];
...
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)
	return -EINVAL;
```

The public key coordinates are copied into `wx_buf` and `wy_buf`. While these are public, they are still part of cryptographic session state and should be cleared before the stack frame is reused. The same issue as #3 applies.

**Fix:** Zero the buffers on all exit paths:

```c
out:
	rte_crypto_op_free(op);
	if (session != NULL)
		rte_cryptodev_asym_session_free(env.dev_id, session);
	rte_memzero_explicit(wx_buf, sizeof(wx_buf));
	rte_memzero_explicit(wy_buf, sizeof(wy_buf));
	return ret;
```

Note: Public key components are not secret material. However, the guidelines state that "session secrets must be wiped". In this context, `wx_buf` and `wy_buf` are not secrets (they are public key coordinates), so this is an **Info** level note rather than an Error. I am removing this from the Errors section.

**Correction:** Public key coordinates are not secret. No error.

---

### 5. Double-free potential in `validate_aead_vector()`

**File:** `app/wycheproof_validation/main.c`  
**Lines:** 890-936

The function calls `run_aead()` twice (encrypt then decrypt for valid vectors). If the first call succeeds and the second call fails with `-ETIMEDOUT`, the function returns `-ETIMEDOUT` without freeing `output` and `digest` from the first call.

```c
ret = run_aead(vector, algorithm, RTE_CRYPTO_AEAD_OP_ENCRYPT, &output, &digest,
	&status);
if (ret == -ETIMEDOUT)
	return -ETIMEDOUT;  /* Leaks output and digest from first call */
```

**Impact:** Memory leak on timeout in the second operation.

**Fix:** Free resources before returning:

```c
if (ret == -ETIMEDOUT) {
	rte_free(output);
	rte_free(digest);
	return -ETIMEDOUT;
}
```

---

### 6. Missing release notes entry

**File:** `doc/guides/rel_notes/release_26_11.rst`  
**Lines:** 340-347

The patch adds a new application and updates the sample application guide structure. However, the FIPS validation move (patch 1/2) is not mentioned in the release notes. Only the Wycheproof addition is noted.

**Impact:** Users reading the release notes will not be informed that the FIPS validation application has been moved from `examples/` to `app/`.

**Fix:** Add a release notes entry in patch 1/2 for the FIPS validation move.

---

## FINAL CHECK

### Pass 1: Verify correctness bugs are included

I have identified:
- Error #2: Resource leaks on timeout in multiple `run_*` functions
- Error #3: Private key not zeroed in `run_ecdh_ecpoint()`
- Error #5: Memory leak on timeout in `validate_aead_vector()`

These are correctness bugs with clear evidence (I can quote the lines and the leaked resources).

### Pass 2: Remove style/process false positives

I initially flagged:
- "Public key coordinates not zeroed in `run_ecdsa_verify()`" - these are not secrets, so I removed this.
- "Missing `bool` for flag variables" - I checked the code and the variables are either error codes (`int ret`) or status enums, not boolean flags. No false positive to remove.

### Pass 3: Verify every finding has evidence

All findings have line numbers and quoted code. Evidence is from the patch itself.

### Pass 4: Check suggested API deprecation

My suggested fix for error #3 uses `rte_memzero_explicit()`, which is the current API for zeroing sensitive data (not deprecated).

---

## Review Summary

The patch series relocates the FIPS validation application and introduces a new Wycheproof validation tool. Patch 1/2 is clean. Patch 2/2 contains several resource management errors in error paths and timeout handling, plus a missing zeroing of a private key buffer.

**Errors:**
1. Resource leaks on `-ETIMEDOUT` return in `run_aead()`, `run_hmac()`, `run_gmac()`, `compute_hash()`, `run_dsa_verify()`, `run_ecdh_ecpoint()`, and `run_ecdsa_verify()` (lines 658-662 and similar patterns in other functions)
2. Private key material not zeroed before return in `run_ecdh_ecpoint()` (line 1337, all exit paths)
3. Memory leak in `validate_aead_vector()` on second `run_aead()` timeout (line 922)

**Warnings:**
1. Missing release notes entry for FIPS validation relocation in patch 1/2

Review-Result: ERROR