|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

**Inverted logic after switching to `rte_memeq_timingsafe()`**

The original code used `memcmp()` which returns 0 when buffers are equal:
```c
if (memcmp(mac, gen_mac, mac_len))  /* non-zero = different */
    op->status = RTE_CRYPTO_OP_STATUS_AUTH_FAILED;
else
    op->status = RTE_CRYPTO_OP_STATUS_SUCCESS;
```

After the change, `rte_memeq_timingsafe()` returns `true` (non-zero) when buffers are equal, so the logic is correctly inverted:
```c
if (rte_memeq_timingsafe(mac, gen_mac, mac_len))  /* true = equal */
    op->status = RTE_CRYPTO_OP_STATUS_SUCCESS;
else
    op->status = RTE_CRYPTO_OP_STATUS_AUTH_FAILED;
```

This is correct. No issue here.

### Correctness Review

- Authentication tag comparison now uses constant-time function, preventing timing side channel
- Logic correctly inverted to match `rte_memeq_timingsafe()` semantics
- No resource leaks or error path issues

### Summary for Patch 1/3

No issues found. The patch correctly replaces timing-vulnerable `memcmp()` with `rte_memeq_timingsafe()` for MAC verification and properly inverts the comparison logic.

---

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

### Errors

**Salt comparison is not a security-critical operation**

This patch replaces `memcmp()` with `rte_memeq_timingsafe()` in `softreq_copy_salt()`:

```c
addr = rte_crypto_op_ctod_offset(sr->op, uint8_t *, ctx->iv.offset);
-if (!memcmp(ctx->salt, addr, AES_GCM_SALT_SIZE))
+if (rte_memeq_timingsafe(ctx->salt, addr, AES_GCM_SALT_SIZE))
    return 0;

memcpy(ctx->salt, addr, AES_GCM_SALT_SIZE);
```

The function is checking whether the cached salt in the session context matches the salt in the current operation's IV. If they match, it returns early (optimization to skip redundant copy). If they don't match, it updates the cached salt.

This is NOT an authentication/verification operation that gates acceptance of untrusted data. The salt is not secret key material being verified against an attacker-supplied value. This is a performance optimization comparing two application-controlled values to avoid redundant work.

**Timing leakage concern is misplaced here.** An attacker cannot exploit timing differences in an internal equality check used purely to skip redundant copies. The salt values being compared are both set by the application, not computed vs. attacker-supplied.

The use of `rte_memeq_timingsafe()` is unnecessary overhead for this code path.

### Correctness Review

- Logic correctly inverted (`!memcmp()` becomes `rte_memeq_timingsafe()`)
- No resource leaks or error path issues
- Function behavior unchanged

### Summary for Patch 2/3

**Error:** Unnecessary use of constant-time comparison for non-security-critical operation. The salt comparison in `softreq_copy_salt()` is an optimization check (skip redundant copy if salt unchanged), not authentication. Both salt values are application-controlled, not attacker-supplied. Standard `memcmp()` is appropriate here. Constant-time comparison adds unnecessary overhead with no security benefit.

---

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

### Errors

**MAC address lookup is not a security-critical operation**

This patch replaces `memcmp()` with `rte_memeq_timingsafe()` in `nix_find_mac_addr()`:

```c
for (i = 0; i < dev->max_mac_entries; i++) {
-   if (!memcmp(&eth_dev->data->mac_addrs[i], addr, sizeof(*addr)))
+   if (rte_memeq_timingsafe(&eth_dev->data->mac_addrs[i], addr, sizeof(*addr)))
        return i;
}
```

`nix_find_mac_addr()` searches the device's configured MAC address table for a specific address and returns its index. This is a lookup/search operation, not authentication or verification of attacker-supplied data against a secret.

**Timing leakage concern is misplaced.** The MAC address table is application-configured (via `rte_eth_dev_mac_addr_add()` etc.) and is not secret. The lookup is comparing configuration data against configuration data. There is no attacker-controlled comparison value and no accept/reject security decision being made.

The function is used internally by MAC address management operations (`mac_addr_add`, `mac_addr_remove`, etc.). The timing of the search does not leak security-sensitive information.

The use of `rte_memeq_timingsafe()` is unnecessary overhead for this code path.

### Correctness Review

- Logic correctly inverted (`!memcmp()` becomes `rte_memeq_timingsafe()`)
- No resource leaks or error path issues
- Function behavior unchanged

### Summary for Patch 3/3

**Error:** Unnecessary use of constant-time comparison for non-security-critical operation. The MAC address lookup in `nix_find_mac_addr()` searches device configuration data (MAC address table) and is not an authentication or verification operation. Both the table and search key are application-controlled, not attacker-supplied. Standard `memcmp()` is appropriate here. Constant-time comparison adds unnecessary overhead with no security benefit.

---

## Overall Series Assessment

**Patch 1/3:** Correct fix for timing side channel in MAC verification.

**Patch 2/3 and 3/3:** Misapplied security fix. Not every use of `memcmp()` in crypto/network code requires constant-time comparison. Only comparisons that verify attacker-influenced data against computed secrets (MAC/digest/tag verification) need constant-time treatment.

The guideline states:
> "Do NOT flag memcmp on non-secret data: algorithm IDs, key lengths, capability/feature structs, lookup keys. Only flag comparisons that gate acceptance of attacker-influenced data."

Patches 2 and 3 flag comparisons that are clearly non-secret lookups/optimizations, not authentication.