|WARNING| [v2, 5/5] net/bnxt: fix VF info alloc error path memory leak

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-21

# DPDK Patch Review

## Patch 1/5: net/bnxt: add VF ID boundary check before usage

### Errors

**Out-of-bounds array access via firmware-controlled index** (Correctness bug)
The patch correctly identifies and fixes an out-of-bounds access: `fw_vf_id` (from firmware) was used to compute `vf_id` and index `bp->pf->vf_info[]` before validating that `fw_vf_id` falls within the active VF range. An attacker controlling firmware could supply an out-of-range `fw_vf_id`, causing `vf_info[vf_id]` to read/write memory outside the array bounds.

The fix (moving the range check before the computation and dereference) is correct.

### Warnings

None.

---

## Patch 2/5: net/bnxt: fix bounds on firmware-reported resource counts

### Errors

None.

### Warnings

**Integer overflow in addition then truncation to 16-bit** (Correctness bug, fixed by this patch)
The original code added two `uint16_t` values (`bp->max_l2_ctx + bp->max_rx_em_flows`) in a 16-bit context, which could overflow, then assigned the truncated result back to the 16-bit `bp->max_l2_ctx`. The patch correctly widens the addition to `uint32_t` and clamps the result to `BNXT_MAX_L2_CTX` (65535) before the cast back to 16-bit, preventing silent truncation.

The fix is correct.

---

## Patch 3/5: net/bnxt: fix use-after-free in VNIC filter cleanup

### Errors

**Use-after-free in `STAILQ_FOREACH` loop body** (Correctness bug, fixed by this patch)
`STAILQ_FOREACH(filter, &vnic->filter, next)` dereferences `filter->next` at the end of each iteration to advance to the next node. The loop body calls `bnxt_free_filter(bp, filter)`, freeing `filter` before the macro reads `filter->next` for the next iteration. This is a classic use-after-free in a list traversal that modifies (removes/frees) nodes.

The patch correctly rewrites the loop to use `STAILQ_FIRST()` and `STAILQ_REMOVE_HEAD()`, removing each filter from the list before freeing it so no freed memory is dereferenced.

The fix is correct.

---

## Patch 4/5: net/bnxt: fix memory leak in VF VNIC query error path

### Errors

**Memory leak on error path** (Correctness bug, fixed by this patch)
`bnxt_hwrm_func_vf_vnic_query_and_config()` allocates `vnic_ids` via `rte_malloc()`, then calls `bnxt_hwrm_func_vf_vnic_query()`. If that call fails (`num_vnic_ids < 0`), the original code returned immediately without freeing `vnic_ids`, leaking the allocation.

The patch adds `rte_free(vnic_ids)` on the error path before `return`, correctly plugging the leak.

The fix is correct.

---

## Patch 5/5: net/bnxt: fix VF info alloc error path memory leak

### Errors

**Memory leak on error path due to NULL pointer in cleanup** (Correctness bug, fixed by this patch)
In `bnxt_alloc_vf_info()`, the original code allocated `vf_info`, performed nested allocations in a loop (each of which could fail and `goto err`), and only assigned `bp->pf->vf_info = vf_info` at the very end. If a nested allocation failed, the cleanup path (`bnxt_free_vf_info(bp)`) ran while `bp->pf->vf_info` was still `NULL`, so `bnxt_free_vf_info()` could not free the partially-initialized `vf_info` array or its nested allocations, causing a leak.

The patch moves the assignment `bp->pf->vf_info = vf_info` to before the nested allocations, so the cleanup function can always find and free the allocation on any error path.

The fix is correct.

---

## Summary

All five patches are **correctness bug fixes**:
- **Patch 1**: Prevents out-of-bounds array access via untrusted firmware input (critical security/correctness issue).
- **Patch 2**: Prevents integer overflow and truncation in resource count calculations.
- **Patch 3**: Fixes use-after-free in linked list traversal.
- **Patch 4**: Plugs memory leak on error path (missing free).
- **Patch 5**: Plugs memory leak on error path (cleanup function couldn't find allocation).

**No style, format, or process violations were identified.** The patches follow DPDK coding conventions:
- Bracing and indentation are correct.
- `NULL` checks are explicit (`if (encaped)` in patch 1 is correct for guarding `memcpy` against a NULL source).
- Error propagation is correct.
- `Fixes:` and `Cc: stable at dpdk.org` tags are present (LTS backport compliance).
- Commit messages are clear and describe the bug and fix accurately.

**All patches are recommended for acceptance.**