|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 TruFlow code paths that trusted unvalidated indices from firmware responses, adding bounds checks to prevent out-of-bounds memory accesses in the dpool allocator and EM hash delete paths. It also fixes PCI name parsing in `tf_attach_session()` to accept DPDK's optional-domain format.

---

## Errors

### 1. Inconsistent bounds check logic: `(uint32_t)start >= dpool->size` vs `(parms->index - pool->start_index) < pool->size`

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

The patch adds `(uint32_t)start >= dpool->size` checks in `dpool_free()` and `dpool_set_entry_data()`,
but uses the inverted form `(parms->index - pool->start_index) < pool->size` in `tf_em_hash_delete_int_entry()`.
Both forms are logically equivalent (given `start = parms->index - pool->start_index`),
but using inconsistent patterns in the same patch when checking the same condition reduces readability
and suggests one may be wrong.

The subtraction `parms->index - pool->start_index` can underflow when `parms->index < pool->start_index`,
producing a large unsigned value. In the dpool functions, this is caught by the `start < 0` check first
(converting to signed then testing negative). In `tf_em_hash_delete_int_entry()`, the expression
`(parms->index - pool->start_index) < pool->size` performs unsigned arithmetic:
when `parms->index < pool->start_index`, the subtraction wraps,
and the comparison against `pool->size` is unlikely to catch it (depends on exact values).

**Recommended fix:** Use the same pattern as `dpool_free()` for clarity and correctness:

```c
/* tf_em_hash_delete_int_entry(): validate index before dereferencing pool->entry[] */
int offset = (int)(parms->index - pool->start_index);
if (offset < 0 || (uint32_t)offset >= pool->size)
	goto skip_usage_update;

size = DP_FLAGS_SIZE(pool->entry[offset].flags);
tf_em_usage_update(tfp, parms->dir, size, TF_RESC_FREE);
skip_usage_update:
```

This ensures underflow is caught by the `offset < 0` check rather than relying on unsigned wraparound.

---

### 2. Missing bounds check in `TF_FLOW_SCALE_QUERY` path remains a vulnerability

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

The patch adds a bounds check around the `pool->entry[]` dereference inside the `#ifdef TF_FLOW_SCALE_QUERY` block,
but it then **silently continues** to call `dpool_free(pool, parms->index)` even when the index is out of range.
`dpool_free()` now has an upper-bound check and will return `-1` on invalid index,
but that return value is **not checked** by the caller.

If firmware returns an out-of-bounds `parms->index`, the patch correctly skips the usage update
but then passes the bogus index to `dpool_free()`, which will fail silently (return `-1` is dropped).
The function then returns 0 (success), and the caller believes the entry was deleted when it was not.
This is a **logic error**: a corrupt firmware response can cause a resource leak
(the entry remains allocated but the application thinks it's freed).

**Recommended fix:** Check the return value of `dpool_free()` and propagate the error:

```c
int offset = (int)(parms->index - pool->start_index);
if (offset >= 0 && (uint32_t)offset < pool->size) {
	size = DP_FLAGS_SIZE(pool->entry[offset].flags);
	tf_em_usage_update(tfp, parms->dir, size, TF_RESC_FREE);
}

rc = dpool_free(pool, parms->index);
if (rc != 0) {
	TFP_DRV_LOG(ERR, "Failed to free EM entry index %u\n", parms->index);
	return -EINVAL;
}
return 0;
```

This prevents the function from reporting success when the index was invalid.

---

## Warnings

### 3. Implicit zero comparison: `size == 0`

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

```c
if (size > dpool->max_alloc_size || size == 0 ||
```

The existing code (not introduced by this patch) uses implicit zero comparison.
DPDK style requires explicit comparison:

```c
if (size > dpool->max_alloc_size || size == 0 ||
```

**Wait--this IS explicit.** No issue here. (Checking my notes: explicit `== 0` is correct. Disregard.)

---

### 4. `sscanf()` return value overwritten without checking intermediate results

**File:** `drivers/net/bnxt/tf_core/tf_core.c` lines 121 and 144

```c
rc = sscanf(parms->ctrl_chan_name, "%x:%x:%x.%u", &domain, &bus, &slot, &device);
if (rc != 4) {
	domain = 0;
	rc = sscanf(parms->ctrl_chan_name, "%x:%x.%u", &bus, &slot, &device);
	if (rc != 3) {
		TFP_DRV_LOG(ERR, "Failed to scan device ctrl_chan_name\n");
		return -EINVAL;
	}
}
```

When the first `sscanf()` returns `rc != 4`, the code immediately overwrites `rc` with the second `sscanf()` result.
This is acceptable because the first `rc` has already been tested (`!= 4` branch taken),
and the variable is reused for the second parse attempt.

However, the pattern is potentially fragile: if a third parse attempt were added,
it would be easy to forget to test `rc` before overwriting.
This is a **style observation**, not a bug--the current code is correct.

**No change required**, but consider using separate variables (`rc1`, `rc2`) or testing `rc` immediately
if this pattern is extended in the future.

---

### 5. Domain variable reused after failed 4-field parse

**File:** `drivers/net/bnxt/tf_core/tf_core.c` lines 115 and 141

When the 4-field `sscanf()` fails and the fallback 3-field parse is attempted,
`domain` is unconditionally set to `0` but `bus`, `slot`, and `device` are **not reset**.
If the first `sscanf()` partially succeeded (e.g., `rc == 2`),
those variables may contain stale values from the failed parse.

The fallback `sscanf()` writes to `bus`, `slot`, `device` again,
so if it succeeds (`rc == 3`), the stale values are overwritten.
But if the fallback also fails, the variables retain partial results from the first parse attempt.
Since the function returns `-EINVAL` on fallback failure, the stale values are never used.

**This is not a bug** because the error path does not use `bus`/`slot`/`device`.
However, initializing them to 0 before the first `sscanf()` would make the code more robust.

**Suggested improvement** (not required):

```c
unsigned int domain = 0, bus = 0, slot = 0, device = 0;
```

---

## Info

### 6. Consider extracting PCI name parsing into a helper function

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

The patch duplicates the 4-field-then-3-field `sscanf()` fallback pattern twice
(once for `ctrl_chan_name`, once for `attach_chan_name`).
The same pattern appears in `tf_open_session()` (mentioned in the commit message).

**Suggestion:** Extract this into a helper function:

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

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

	return -EINVAL;
}
```

Then use:

```c
if (parse_pci_name(parms->ctrl_chan_name, &domain, &bus, &slot, &device) != 0) {
	TFP_DRV_LOG(ERR, "Failed to scan device ctrl_chan_name\n");
	return -EINVAL;
}
```

This eliminates duplication and makes future changes (e.g., supporting extended PCI formats) easier.

---

### 7. Consider logging the invalid index when bounds check fails

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

When `dpool_free()` or `dpool_set_entry_data()` detect an out-of-range index,
they return `-1` but do not log the invalid value.
Adding a log statement would aid debugging firmware issues:

```c
if (start < 0 || (uint32_t)start >= dpool->size) {
	TFP_DRV_LOG(ERR, "dpool index %d out of range [0, %u)\n",
		    index, dpool->start_index + dpool->size);
	return -1;
}
```

---

### 8. `TF_FLOW_SCALE_QUERY` comment could be clearer

**File:** `drivers/net/bnxt/tf_core/tf_em_hash_internal.c` line 156

The added comment:

```c
/* Update usage state buffer for EM.
 * parms->index is populated from the (untrusted) HWRM response by
 * tf_msg_delete_em_entry(); validate it before indexing the pool.
 */
```

is helpful but could explicitly state what happens on validation failure:

```c
/* Update usage state buffer for EM.
 * parms->index originates from firmware (untrusted). Validate it before
 * indexing pool->entry[]; skip usage update if out of range.
 */
```

---

## Overall Assessment

The patch correctly identifies and mitigates out-of-bounds access vulnerabilities in TruFlow.
The dpool bounds checks are sound (modulo the style inconsistency noted),
and the PCI name parsing fix is correct.

**Critical issue:** Error propagation from `dpool_free()` is missing in the delete path,
allowing a corrupt firmware response to silently leak resources.
This should be fixed before merging.

**Style issue:** Inconsistent bounds-check pattern between dpool and em_hash_delete creates confusion;
unifying the approach improves clarity and catches underflow correctly.