|WARNING| net/bnxt: add bounds checks for FW-supplied IDs

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Summary
This patch adds bounds checking for firmware-supplied IDs used as array indices in the bnxt PMD. Overall, the patch addresses important correctness issues by validating array indices before use.

---

## Errors

### 1. Missing cleanup on error paths after initial allocation

The function allocates/initializes `func` at line 167 (`func->func_valid` is set), but the newly added error returns at lines 189 and 220 do not clean up this state. If `vf_func_id` or `phy_port_id` validation fails, the partially initialized `func` entry remains marked valid in `port_db->ulp_func_id_tbl[intf->drv_func_id]`, potentially causing stale data to be used.

**Fix:** Reset `func->func_valid = 0` before returning error, or restructure to validate all IDs before setting any state.

```c
/* After line 188: */
if (intf->vf_func_id >= BNXT_PORT_DB_MAX_FUNC) {
	BNXT_DRV_DBG(ERR, "Invalid vf_func_id %d", intf->vf_func_id);
	port_db->ulp_func_id_tbl[intf->drv_func_id].func_valid = 0;  /* cleanup */
	return -EINVAL;
}

/* Similarly after line 219: */
if (func->phy_port_id >= port_db->phy_port_cnt) {
	BNXT_DRV_DBG(ERR, "Invalid phy_port_id %d", func->phy_port_id);
	port_db->ulp_func_id_tbl[intf->drv_func_id].func_valid = 0;  /* cleanup */
	return -EINVAL;
}
```

### 2. Inconsistent bounds checking order

The `drv_func_id` validation occurs after it is already used as an array index at line 167 (`func = &port_db->ulp_func_id_tbl[intf->drv_func_id]`). The bounds check at line 162 comes too late--the out-of-bounds access has already occurred.

**Fix:** Move the bounds check to immediately after `drv_func_id` is obtained (before line 167).

```c
intf->drv_func_id = bnxt_pmd_get_fw_func_id(port_id,
					BNXT_ULP_INTF_TYPE_INVALID);

if (intf->drv_func_id >= BNXT_PORT_DB_MAX_FUNC) {
	BNXT_DRV_DBG(ERR, "Invalid drv_func_id %d", intf->drv_func_id);
	return -EINVAL;
}

func = &port_db->ulp_func_id_tbl[intf->drv_func_id];  /* now safe */
```

---

## Warnings

None.

---

## Info

### 1. Potential for additional validation

The patch checks indices against array size limits but does not validate that the firmware-supplied IDs are non-negative. If `bnxt_pmd_get_fw_func_id()` or related functions can return negative values to signal errors, the current `>= MAX` check alone is insufficient (negative values would pass the check but cause undefined behavior when used as array indices).

**Consider:** Add explicit checks for negative values if the getter functions document error returns, or document that they guarantee non-negative results.

```c
if (intf->drv_func_id < 0 || intf->drv_func_id >= BNXT_PORT_DB_MAX_FUNC) {
	/* ... */
}
```

---

## Positive Notes

- The patch correctly identifies a security-relevant issue (untrusted firmware data used as array index)
- Bounds checks use the appropriate constants (`BNXT_PORT_DB_MAX_FUNC`, `port_db->phy_port_cnt`)
- Error messages are clear and include the invalid value for debugging