|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 `ulp_port_db_port_update()`. Overall, the patch addresses a real security concern (out-of-bounds access from untrusted FW data), but has several issues with error handling and code quality.

---

## Errors

### 1. Resource leak on error path after `drv_func_id` validation failure
After `func->func_valid` is set to `true` (line 170), the error path at line 219 returns without cleanup. If the `phy_port_id` bounds check fails, `func->func_valid` remains set but the port is not fully initialized, leaving inconsistent state.

**Fix:** Add cleanup before returning -EINVAL, or move all validation checks before setting `func_valid`.

```c
/* Preferred: validate all IDs upfront */
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;
}
if (intf->type == BNXT_ULP_INTF_TYPE_VF_REP) {
	intf->vf_func_id = bnxt_pmd_get_fw_func_id(port_id, BNXT_ULP_INTF_TYPE_VF_REP);
	if (intf->vf_func_id >= BNXT_PORT_DB_MAX_FUNC) {
		BNXT_DRV_DBG(ERR, "Invalid vf_func_id %d", intf->vf_func_id);
		return -EINVAL;
	}
}
/* Now set func_valid after all validation passes */
func->func_valid = true;
```

### 2. `phy_port_id` bounds check occurs after `func->phy_port_id` is already used
The validation at line 219 checks `func->phy_port_id >= port_db->phy_port_cnt` **after** the value has been assigned and potentially used. The assignment happens earlier in the function (not shown in the diff), and by the time the check executes, the bad value has already been stored. This is a **TOCTOU (time-of-check-time-of-use)** issue.

**Fix:** Move the `phy_port_id` bounds check immediately after the value is assigned to `func->phy_port_id`, before any use of that value.

```c
/* Immediately after func->phy_port_id is assigned: */
func->phy_port_id = bnxt_pmd_get_phy_port_id(port_id);
if (func->phy_port_id >= port_db->phy_port_cnt) {
	BNXT_DRV_DBG(ERR, "Invalid phy_port_id %d", func->phy_port_id);
	return -EINVAL;
}
```

### 3. Missing bounds check on `intf->drv_func_id` before first use
The bounds check is added at line 162, but the diff does not show where `intf->drv_func_id` is assigned. If the assignment happens **before** line 159 (the visible context start), the value could be used as an array index before the check executes. Verify that the bounds check occurs immediately after assignment and before any use.

---

## Warnings

### 4. Inconsistent error message formatting
The error messages use different capitalization of the ID name (`drv_func_id` vs `vf_func_id` vs `phy_port_id`). While this matches the variable names, consider making the user-facing message more consistent or descriptive.

**Suggestion:**
```c
BNXT_DRV_DBG(ERR, "Driver function ID %d exceeds maximum %d",
             intf->drv_func_id, BNXT_PORT_DB_MAX_FUNC - 1);
```

### 5. Return value inconsistency on validation failure
The function returns `-EINVAL` on validation failure, but the caller may not distinguish this from other error conditions. Consider using a more specific error code for bounds check failures (e.g., `-ERANGE` or a driver-specific code) to aid debugging.

### 6. Missing validation for the untrusted ID source
While the patch adds bounds checks, it does not address the root issue: why is firmware data trusted without validation? Consider adding a comment or assertion documenting that these IDs come from firmware and require validation.

**Suggestion:**
```c
/* Firmware-supplied ID - validate before use as array index */
if (intf->drv_func_id >= BNXT_PORT_DB_MAX_FUNC) {
	BNXT_DRV_DBG(ERR, "Invalid drv_func_id %d from FW", intf->drv_func_id);
	return -EINVAL;
}
```

---

## Info

### 7. Consider consolidating validation logic
The same bounds check pattern (`>= BNXT_PORT_DB_MAX_FUNC`) is repeated for `drv_func_id` and `vf_func_id`. Consider extracting this into a helper function or macro to reduce duplication and ensure consistency.

```c
static inline bool
ulp_is_valid_func_id(uint16_t func_id)
{
	return func_id < BNXT_PORT_DB_MAX_FUNC;
}
```

### 8. Release notes required
This patch fixes a security vulnerability (potential out-of-bounds access from untrusted firmware data). It should be mentioned in the release notes under "**Fixed Issues**" for the net/bnxt PMD.

---

## Style Observations (No Action Required)

- The code follows DPDK style conventions for indentation, braces, and naming
- Error logging uses the appropriate `BNXT_DRV_DBG(ERR, ...)` macro
- Return value is consistent with existing code style

---

## Final Recommendation

**Do not merge as-is.** The patch addresses a real issue but introduces new correctness bugs:
1. Resource leak on error path after `func_valid` is set
2. TOCTOU issue with `phy_port_id` check occurring after the value is used
3. Potential use-before-check of `drv_func_id` depending on code not shown in diff

These must be fixed before merging to stable branches.