|WARNING| [v1,3/3] net/cnxk: use timing-safe MAC address comparison

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-08-03

# DPDK Patch Review

## Patch 1/3: common/cpt: use timing-safe digest comparison

### Errors

**1. Inverted return value logic**

The conversion from `memcmp()` to `rte_memeq_timingsafe()` is incorrect. `memcmp()` returns 0 when buffers match, while `rte_memeq_timingsafe()` returns **true** (non-zero) when buffers match. The patch inverted the condition but did not account for this semantic difference.

```c
/* Current (WRONG) */
if (rte_memeq_timingsafe(mac, gen_mac, mac_len))
    op->status = RTE_CRYPTO_OP_STATUS_SUCCESS;
else
    op->status = RTE_CRYPTO_OP_STATUS_AUTH_FAILED;

/* Correct */
if (!rte_memeq_timingsafe(mac, gen_mac, mac_len))
    op->status = RTE_CRYPTO_OP_STATUS_AUTH_FAILED;
else
    op->status = RTE_CRYPTO_OP_STATUS_SUCCESS;
```

The original code rejected when `memcmp() != 0` (buffers differ). The patch now rejects when `rte_memeq_timingsafe() == 0` (buffers differ), which produces the **opposite result** -- authentication will fail when MACs match and succeed when they don't.

### Correctness Summary

This patch introduces a **critical authentication bypass bug**. All MACs will fail verification when they should succeed, and succeed when they should fail.

---

## Patch 2/3: crypto/nitrox: use timing-safe salt comparison

### Errors

**1. Incorrect return value handling**

The function `softreq_copy_salt()` returns 0 when the salts match (no copy needed) and performs a copy when they differ. The conversion is incorrect:

```c
/* Original (memcmp returns 0 when equal) */
if (!memcmp(ctx->salt, addr, AES_GCM_SALT_SIZE))
    return 0;  /* salts match, no copy needed */

/* Current (WRONG - rte_memeq_timingsafe returns true when equal) */
if (rte_memeq_timingsafe(ctx->salt, addr, AES_GCM_SALT_SIZE))
    return 0;  /* salts match, no copy needed */
```

The patch happens to be correct here because:
- Original: `!memcmp()` - returns true when salts match
- Patch: `rte_memeq_timingsafe()` - returns true when salts match

However, the reviewer should note that this is **not** the pattern used in Patch 1/3, creating inconsistency. The correct conversion pattern (mirroring Patch 1's intended logic) would be to negate the result if the original code checked `memcmp() == 0`, or leave it un-negated if the original checked `!memcmp()`.

### Warnings

**1. Salt comparison is not a timing-sensitive operation**

The AES-GCM salt in this context is used to detect whether the IV prefix has changed, requiring a session update. This is a **configuration check**, not a cryptographic verification against attacker-supplied data. Timing leaks here do not constitute a security vulnerability -- the salt value is not secret in this comparison (both values come from the session/request configuration).

The guidelines state to flag non-constant-time digest/tag/MAC comparisons only when "comparing a computed value against an attacker-supplied one to decide accept/reject." This is a control-flow optimization check (should we copy or skip), not a cryptographic verification gate.

Applying `rte_memeq_timingsafe()` here is unnecessary and adds overhead for no security benefit.

---

## Patch 3/3: net/cnxk: use timing-safe MAC address comparison

### Warnings

**1. MAC address lookup is not a timing-sensitive operation**

The function `nix_find_mac_addr()` searches the device's configured MAC address table to determine the index of a given address. This is a **lookup operation**, not a cryptographic verification. MAC addresses in this context are public configuration values, not secrets being validated against attacker input.

Timing-safe comparison is intended for cryptographic operations where an attacker can submit candidate values and measure how long verification takes to infer secret data. In this case:
- The MAC addresses being compared are the device's own configuration (public)
- The function returns an index for configuration purposes, not an accept/reject security decision
- There is no attacker-controlled input being validated against a secret

Applying `rte_memeq_timingsafe()` here is unnecessary and adds overhead for no security benefit.

---

## Series-Wide Issues

### Errors

**1. Inconsistent conversion patterns across patches**

The series applies three different interpretations of the `memcmp()` - `rte_memeq_timingsafe()` conversion:
- Patch 1: Inverted condition (WRONG semantics)
- Patch 2: Direct replacement of `!memcmp()` with `rte_memeq_timingsafe()` (CORRECT)
- Patch 3: Direct replacement of `!memcmp()` with `rte_memeq_timingsafe()` (semantically correct but unnecessary)

This indicates a misunderstanding of the return value semantics. The correct rule is:
- `memcmp(a, b, n) == 0` - `rte_memeq_timingsafe(a, b, n)` (both true when equal)
- `!memcmp(a, b, n)` - `rte_memeq_timingsafe(a, b, n)` (both true when equal)
- `memcmp(a, b, n) != 0` - `!rte_memeq_timingsafe(a, b, n)` (both true when different)

Patch 1 violates this rule.

### Warnings

**2. Bugzilla ID 1773 may be misfiled**

Only Patch 1/3 addresses a legitimate timing-side-channel vulnerability (authentication tag verification). Patches 2/3 apply timing-safe comparison to non-cryptographic lookups where timing leaks are not security-relevant. If Bugzilla 1773 requested "use constant-time comparison for all memcmp," the bug report should be refined to target only cryptographic verification sites.

---

## Summary

| Patch | Correctness | Necessity |
|-------|-------------|-----------|
| 1/3 (cpt digest) | **BROKEN** -- inverted logic causes auth bypass | Required fix (with correct logic) |
| 2/3 (nitrox salt) | Correct but unnecessary | Salt comparison is not timing-sensitive |
| 3/3 (cnxk MAC) | Correct but unnecessary | MAC lookup is not timing-sensitive |

**Action Required:**
1. **Patch 1 must be corrected** -- the current logic will cause all authentication to fail incorrectly
2. Patches 2 and 3 should be dropped or justified with a concrete threat model