|WARNING| net/bnxt: fix truflow out-of-bounds accesses

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: net/bnxt: fix truflow out-of-bounds accesses

## Summary

This patch hardens several TruFlow paths against out-of-bounds accesses
from untrusted firmware-supplied indices and improves PCI device name parsing
in session attach. Overall the patch addresses real correctness issues.
A few items require clarification or adjustment.

---

## Errors

### 1. Incomplete signedness cast in bounds checks

**File:** `drivers/net/bnxt/tf_core/dpool.c`

The bounds checks cast `start` (an `int`) to `uint32_t` to compare against `dpool->size`:

```c
if (start < 0 || (uint32_t)start >= dpool->size)
```

This is incorrect when `start` is negative. The `start < 0` check executes first,
but if the code is reordered or the negative check is removed in a future change,
`(uint32_t)start` will produce a large positive value (wraparound) and pass the check.

**Why it matters:**
The pattern invites a future bug if someone removes the redundant-looking `start < 0` check.

**Suggested fix:**
Use a single comparison that handles both bounds:

```c
if ((uint32_t)start >= dpool->size)
    return -1;
```

When `start` is negative, the cast produces a value >= 2^31, which fails the check against `dpool->size`
(assuming `dpool->size` is reasonable). This is safer and more concise.

Applies to all three locations:
- `dpool_free()` first bounds check
- `dpool_free()` second bounds check (line with `size == 0`)
- `dpool_set_entry_data()` bounds check

### 2. Missing upper bound check in `dpool_free()` before flag read

**File:** `drivers/net/bnxt/tf_core/dpool.c`, function `dpool_free()`

The function checks `start < 0 || (uint32_t)start >= dpool->size` but then immediately reads
`dpool->entry[start].flags` inside the `if (DP_IS_START(...))` test:

```c
if (start < 0 || (uint32_t)start >= dpool->size)
    return -1;

if (DP_IS_START(dpool->entry[start].flags)) {  /* start is already validated here */
    size = DP_FLAGS_SIZE(dpool->entry[start].flags);
```

Wait -- the bounds check **does** precede the flag read. This is correct.

Actually, re-reading the patch: the new check `(uint32_t)start >= dpool->size`
is added on the same line as the existing `start < 0`. The dereference comes after.
This is fine -- no issue here.

Retract this item.

### 3. Second bounds check in `dpool_free()` redundant comparison

**File:** `drivers/net/bnxt/tf_core/dpool.c`, function `dpool_free()`

The patch adds:

```c
if (size > dpool->max_alloc_size || size == 0 ||
    (uint32_t)start + size > dpool->size)
    return -1;
```

The addition `(uint32_t)start + size` can overflow when `start` and `size` are both large,
producing a small value that passes the check.

**Example:**
- `dpool->size = 1000`
- `start = UINT32_MAX - 500` (already passed earlier bounds check by being < 1000... no wait, that can't happen)

Actually, `start` is already validated to be `< dpool->size`, so `start` is a small value.
But `size` comes from the pool entry flags and could be corrupted.

If `start = 999` and `size = UINT32_MAX`, then `(uint32_t)start + size` wraps to 998,
which is `< dpool->size`, and the check passes incorrectly.

**Suggested fix:**

```c
if (size > dpool->max_alloc_size || size == 0 ||
    size > dpool->size - start)  /* avoids overflow */
    return -1;
```

This rearranges the comparison to prevent overflow.
When `size > dpool->size - start`, the range `[start, start+size)` exceeds the pool bounds.

### 4. TF_FLOW_SCALE_QUERY index validation off-by-one

**File:** `drivers/net/bnxt/tf_core/tf_em_hash_internal.c`, function `tf_em_hash_delete_int_entry()`

```c
if ((parms->index - pool->start_index) < pool->size) {
    size = DP_FLAGS_SIZE(pool->entry[parms->index - pool->start_index].flags);
```

The check validates `(parms->index - pool->start_index) < pool->size`,
which is correct for an unsigned comparison.
But `parms->index - pool->start_index` is computed as a signed subtraction
(if `parms->index < pool->start_index`, the result is negative).

The code does not cast or check for negative values before the comparison.
If `parms->index < pool->start_index`, the result wraps and the comparison may pass incorrectly.

**Suggested fix:**

```c
int idx = parms->index - pool->start_index;
if (idx >= 0 && (uint32_t)idx < pool->size) {
    size = DP_FLAGS_SIZE(pool->entry[idx].flags);
```

Or, more concisely:

```c
if ((uint32_t)(parms->index - pool->start_index) < pool->size) {
```

If `parms->index < pool->start_index`, the subtraction produces a negative signed value,
which when cast to `uint32_t` becomes a large positive value (>= 2^31) that fails the check.
This matches the pattern used in `dpool_free()` and `dpool_set_entry_data()`.

---

## Warnings

None. The PCI name parsing fallback logic is correct and matches the pattern in `tf_open_session()`.

---

## Info

### 1. Consider extracting PCI name parsing to a helper function

The 4-field + fallback-to-3-field PCI name parsing pattern now appears in three places:
`tf_open_session()` (already present), `tf_attach_session()` ctrl_chan_name (this patch),
and `tf_attach_session()` attach_chan_name (this patch).

Extracting a helper function would reduce duplication and make the pattern easier to maintain:

```c
static int
parse_pci_name(const char *name, unsigned int *domain, unsigned int *bus,
               unsigned int *slot, unsigned int *device)
{
    int rc;

    rc = sscanf(name, "%x:%x:%x.%u", domain, bus, slot, device);
    if (rc == 4)
        return 0;

    /* PCI Domain not provided (optional in DPDK) */
    *domain = 0;
    rc = sscanf(name, "%x:%x.%u", bus, slot, device);
    if (rc == 3)
        return 0;

    return -EINVAL;
}
```

This is a suggestion for code quality, not a blocker.

---

## Final Check Summary

**Correctness bugs identified:**
- Overflow in `(uint32_t)start + size` comparison (Error #3)
- Potential signedness issue in TF_FLOW_SCALE_QUERY index validation (Error #4)
- Redundant negative check pattern in dpool bounds checks (Error #1, style improvement)

**No style or process false positives.**