|WARNING| [v3, 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/170121

_AI Code Review_

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

# DPDK Patch Review

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

### Errors

**Out-of-bounds array access before range check**

The patch moves the range check ahead of the vf_info[] lookup, which is correct. However, there is a logic issue in the initialization of `fwd_cmd` and `req_len`:

```c
struct input *fwd_cmd = NULL;
uint16_t req_len = 0;
```

These initializations are correct and necessary for the error path. However, the error message at the reject label:

```c
fwd_cmd ? rte_le_to_cpu_16(fwd_cmd->req_type) : 0xFFFF
```

This is good defensive coding. No error here.

**Missing NULL check after initialization**

In `bnxt_hwrm_reject_fwd_resp()`, the guard added:

```c
if (encaped)
    memcpy(req.encap_request, encaped, ec_size);
```

This introduces a silent behavior change. If `encaped` is NULL, the function now sends an uninitialized `req.encap_request` buffer to firmware. The original code would have copied from NULL (undefined behavior on most platforms, but caught by address sanitizer). The fix should either:
1. Return an error if `encaped` is NULL, or
2. Zero the buffer when `encaped` is NULL

```c
if (encaped)
    memcpy(req.encap_request, encaped, ec_size);
else
    memset(req.encap_request, 0, sizeof(req.encap_request));
```

### Info

The core fix (moving the range check before the array access) is correct and addresses the stated vulnerability.

---

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

### Warnings

**Potential integer overflow in addition**

The fix for `max_l2_ctx` addition:

```c
uint32_t l2_ctx = bp->max_l2_ctx + bp->max_rx_em_flows;
bp->max_l2_ctx = (uint16_t)RTE_MIN(l2_ctx, (uint32_t)BNXT_MAX_L2_CTX);
```

This is correct: widening to 32-bit before the addition prevents the 16-bit overflow, and the clamp ensures the result fits back into 16 bits. No issue here.

**Accessor mismatch fixed correctly**

The change from `rte_le_to_cpu_32()` to `rte_le_to_cpu_16()` in `bnxt_hwrm_func_resc_qcaps()` correctly matches the field's actual type in the response structure. The removal of the clamp on this path is correct because a properly-read 16-bit value cannot exceed 65535. No issue here.

### Info

The patch correctly addresses the big-endian read bug identified in v3 and the overflow in the `max_l2_ctx` calculation.

---

## Patch 3/5: net/bnxt: fix VNIC filter list walk stopping early

### Errors

**Incorrect macro used for safe list iteration**

The code uses:

```c
while (!STAILQ_EMPTY(&vnic->filter)) {
    filter = STAILQ_FIRST(&vnic->filter);
    rc = bnxt_clear_one_vnic_filter(bp, filter);
    STAILQ_REMOVE_HEAD(&vnic->filter, next);
    bnxt_free_filter(bp, filter);
}
```

This is a correct pattern for draining a list from the head. However, the commit message mentions trying `STAILQ_FOREACH_SAFE()` but finding it unavailable. This is correct: glibc's `sys/queue.h` does not provide `STAILQ_FOREACH_SAFE`. The chosen pattern is functionally equivalent and correct.

**Return value handling**

The loop continues even if `bnxt_clear_one_vnic_filter()` returns an error. The function returns `rc`, but only the last error is preserved. If multiple filters fail to clear, only the final error is returned. This may or may not be acceptable depending on whether partial cleanup is valid. The original code had the same behavior, so this is not a regression, but it's worth noting.

### Info

The fix correctly addresses the early termination bug where only the first filter was cleared.

---

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

### Info

The patch correctly adds `rte_free(vnic_ids)` on the error path where it was missing. Simple and correct.

---

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

### Warnings

**Pointer assignment order matters**

The patch moves `bp->pf->vf_info = vf_info;` earlier so that `bnxt_free_vf_info()` can see it on the error path. This is correct.

However, there is a subtle issue: if any allocation fails in the loop and we jump to `err:`, `bnxt_free_vf_info()` will iterate over `bp->pf->max_vfs` entries (set just before the loop), but only `i` entries are initialized. For entries `i` to `max_vfs-1`, `vf_info[i].vlan_table` and `vf_info[i].mac_addr` are uninitialized pointers. `bnxt_free_vf_info()` will call `rte_free()` on uninitialized pointers unless those fields are zeroed by `rte_zmalloc()`.

The original allocation:

```c
vf_info = rte_zmalloc("bnxt_vf_info", sizeof(vf_info[0]) * max_vfs, 0);
```

`rte_zmalloc()` zeros the memory, so `vf_info[i].vlan_table` and `vf_info[i].mac_addr` start as NULL for all `i`. `rte_free(NULL)` is safe (it does nothing), so the error path does not corrupt memory. However, this relies on `rte_zmalloc()` zeroing memory, which is documented behavior, so this is not an error.

### Info

The fix is correct. The early assignment of `bp->pf->vf_info` ensures the cleanup path can free partially-initialized entries.

---

## Summary

### Patch 1: Errors
- `bnxt_hwrm_reject_fwd_resp()` should zero `req.encap_request` when `encaped` is NULL instead of leaving it uninitialized.

### Patch 2: No issues

### Patch 3: Warnings
- Multiple errors in `bnxt_clear_one_vnic_filter()` calls: only the last error is returned (pre-existing behavior, not a regression).

### Patch 4: No issues

### Patch 5: No issues

### General
All patches correctly address resource leaks and out-of-bounds accesses as described. The code style is consistent with DPDK conventions. The use of `rte_zmalloc()` for zeroing memory is appropriate.