|WARNING| [5/5] net/bnxt: fix TPA agg Rx descriptor and VNIC query bounds

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## PATCH 1/5: net/bnxt: fix stack exhaustion in flow stats

### Errors
None.

### Warnings
None.

### Info
This patch correctly replaces variable-length arrays (VLAs) on the stack with a simple arithmetic calculation. The original code allocated two arrays purely to compute their dimensions via `RTE_DIM()`, which was wasteful and could overflow the stack for large `bp->max_l2_ctx`. The replacement `2 * bp->max_l2_ctx` is mathematically equivalent and eliminates the stack allocation.

---

## PATCH 2/5: net/bnxt: fix bounds on TPA aggregation ID from completions

### Errors

**TPA aggregation ID bounds checks (bnxt_rxr.c):**

1. **Missing bounds check on `agg_id` before indexing `rxr->tpa_info[]`**

   In `bnxt_tpa_start()`, `bnxt_tpa_end()`, and `bnxt_rx_pkt()` (TPA abuf path), the aggregation ID extracted from the hardware completion is used to index `rxr->tpa_info[]` without verifying it is within bounds. The patch adds checks against `BNXT_TPA_MAX_AGGS(bp)` and schedules a ring reset on failure. This is correct.

2. **Incorrect endian conversion in abuf path**

   The original code used:
   ```c
   uint16_t agg_id = rte_cpu_to_le_16(rx_agg->agg_id);
   ```
   This is backward: `rx_agg->agg_id` is a little-endian value from the device, and should be converted **from** little-endian **to** CPU byte order using `rte_le_to_cpu_16()`, not the reverse. The patch corrects this to:
   ```c
   uint32_t agg_id = rte_le_to_cpu_16(rx_agg->agg_id);
   ```

   However, the variable type changed from `uint16_t` to `uint32_t`. This is acceptable (the bounds check compares against a 32-bit limit), but introduces a minor inconsistency: `bnxt_tpa_start()` and `bnxt_tpa_end()` both use `uint32_t agg_id`, while the original abuf path used `uint16_t`. This is not an error (widening to 32-bit is safe), but worth noting.

### Warnings
None.

### Info
The patch correctly adds bounds checks on untrusted hardware-supplied indices before using them to index driver arrays. The aggregation ID comes from the NIC completion and must be validated. Scheduling a ring reset on invalid ID is an appropriate response (the ring is in an inconsistent state if the NIC sends invalid completions).

---

## PATCH 3/5: net/bnxt: harden sprintf bounds for device memory names

### Errors

1. **Resource leak on early return in `bnxt_hwrm_ver_get()`**

   The patch adds:
   ```c
   rte_free(bp->hwrm_short_cmd_req_addr);
   bp->hwrm_short_cmd_req_addr = NULL;
   if (check_snprintf_rc(snp_rc, sizeof(type), "bnxt_hwrm_short_") < 0) {
       bp->flags &= ~BNXT_FLAG_SHORT_CMD;
       return snp_rc;
   }
   ```

   This is correct. The original code freed `bp->hwrm_short_cmd_req_addr` **after** the new snprintf check, so an early return on snprintf failure would leak the old allocation. The patch moves the `rte_free()` before the check. Additionally, it clears `BNXT_FLAG_SHORT_CMD` on the error path, which is necessary because the flag may have been set a few lines earlier and should not claim short-command support when the buffer allocation failed.

2. **Missing `HWRM_UNLOCK()` on early return in `bnxt_hwrm_cfa_pair_*()` functions**

   In `bnxt_hwrm_cfa_pair_exists()`, `bnxt_hwrm_cfa_pair_alloc()`, and `bnxt_hwrm_cfa_pair_free()`, the code does:
   ```c
   HWRM_PREP(&req, ..., BNXT_USE_CHIMP_MB);
   snp_rc = snprintf(req.pair_name, sizeof(req.pair_name), ...);
   if (check_snprintf_rc(snp_rc, sizeof(req.pair_name), "svfr") < 0) {
       HWRM_UNLOCK();
       return snp_rc;
   }
   if (snp_rc >= (int)sizeof(req.pair_name)) {
       HWRM_UNLOCK();
       return -EINVAL;
   }
   ```

   `HWRM_PREP()` acquires `bp->hwrm_lock`. The patch correctly adds `HWRM_UNLOCK()` before each early return. Without this, the lock would remain held, deadlocking every subsequent HWRM call.

3. **Redundant truncation check**

   The `check_snprintf_rc()` helper already logs when `rc >= max_size` (truncation). The `bnxt_hwrm_cfa_pair_*()` functions then add a second check:
   ```c
   if (snp_rc >= (int)sizeof(req.pair_name)) {
       HWRM_UNLOCK();
       return -EINVAL;
   }
   ```

   This is intentional: the commit message states that a truncated `pair_name` sent to firmware could match the wrong CFA pair or none at all, so the functions reject truncation outright (returning `-EINVAL`) instead of proceeding. This is correct behavior, though it duplicates the bounds comparison. The helper logs; the caller fails hard.

### Warnings
None.

### Info

The patch correctly replaces unbounded `sprintf()` with `snprintf()` plus return-value checking. The introduced `check_snprintf_rc()` helper validates both negative return (encoding error) and truncation (`rc >= max_size`). The patch also fixes three bugs it would have introduced: the leak in `bnxt_hwrm_ver_get()`, the missing lock releases in the CFA pair functions, and the missing `BNXT_FLAG_SHORT_CMD` clear.

---

## PATCH 4/5: net/bnxt: fix bounds in MAC pool index and flow parsing

### Errors

1. **Out-of-bounds access in `bnxt_mac_addr_add_op()`**

   The original code did:
   ```c
   struct bnxt_vnic_info *vnic = &bp->vnic_info[pool];
   ...
   if (!vnic) {
       PMD_DRV_LOG_LINE(ERR, "VNIC not found for pool %d!", pool);
       return -EINVAL;
   }
   ```

   This has two bugs:
   - `vnic` is the address of an array element (`&bp->vnic_info[pool]`), so the `if (!vnic)` check is always false (a non-null pointer).
   - `pool` is caller-supplied and indexed `bp->vnic_info[]` before any bounds check or validation that `bp->vnic_info` was allocated.

   The patch reorders:
   ```c
   if (!eth_dev->data->dev_started)
       return 0;
   if (bp->vnic_info == NULL)
       return 0;
   if (pool >= bp->max_vnics) {
       PMD_DRV_LOG_LINE(ERR, "Pool %u exceeds VNIC count %u!", pool, bp->max_vnics);
       return -EINVAL;
   }
   vnic = &bp->vnic_info[pool];
   ```

   This is correct. It checks `dev_started` first (if not started, VNIC array may not be allocated), then validates `bp->vnic_info != NULL`, then bounds-checks `pool` against `bp->max_vnics` before indexing.

2. **Unbounded loop in `bnxt_flow_non_void_item()` and `bnxt_flow_non_void_action()`**

   The original code did:
   ```c
   while (1) {
       if (cur->type != RTE_FLOW_ITEM_TYPE_VOID)
           return cur;
       cur++;
   }
   ```

   If the caller passed a pattern/actions array without a terminating `RTE_FLOW_ITEM_TYPE_END` or `RTE_FLOW_ACTION_TYPE_END`, this would walk off the end of the array indefinitely. The patch adds:
   ```c
   #define BNXT_MAX_FLOW_ITEMS 256
   ...
   int i = 0;
   if (!cur)
       return NULL;
   while (cur->type == RTE_FLOW_ITEM_TYPE_VOID && i < BNXT_MAX_FLOW_ITEMS) {
       cur++;
       i++;
   }
   return cur;
   ```

   This bounds the skip loop to 256 iterations. After the loop, `cur` points to the first non-VOID item or the item at position 256 (if the array was all VOID). The caller then checks `cur->type` for `END`, so malformed input is caught.

   However, the loop condition inverted the return logic. The original returned `cur` when `type != VOID`; the new code stops advancing when `type != VOID`, then returns `cur` after the loop. This is equivalent if the loop terminates normally, but if `i` hits 256 and the item is still VOID, it returns that VOID item instead of advancing further. The caller will then see `type == VOID` or `type == garbage` and should fail. This is acceptable (prevents infinite loop), though it changes the failure mode.

3. **Missing loop-skip in `bnxt_validate_and_parse_flow_type()`**

   The patch adds:
   ```c
   item = bnxt_flow_non_void_item(item + 1);
   ```
   at the end of the pattern-parsing loop, and similarly in `bnxt_filter_type_check()`. The original code did `item++`, which would process VOID items. Skipping VOIDs is correct and matches the start-of-loop logic (`item = bnxt_flow_non_void_item(pattern)`).

### Warnings
None.

### Info

The patch correctly fixes two out-of-bounds bugs: the unchecked `pool` index in `bnxt_mac_addr_add_op()`, and the unbounded VOID-skip loops in flow parsing. The loop bound (`BNXT_MAX_FLOW_ITEMS = 256`) is a reasonable sanity limit.

---

## PATCH 5/5: net/bnxt: fix TPA agg Rx descriptor and VNIC query bounds

### Errors

1. **Out-of-bounds write in `bnxt_rx_pkt()` TPA abuf path**

   The original code did:
   ```c
   tpa_info = &rxr->tpa_info[agg_id];
   RTE_ASSERT(tpa_info->agg_count < 16);
   tpa_info->agg_arr[tpa_info->agg_count++] = *rx_agg;
   ```

   `RTE_ASSERT()` is compiled out in release builds (when `NDEBUG` is defined), so the bounds check disappears. If firmware sent more aggregation segments than `TPA_MAX_NUM_SEGS` (16), `agg_count` would exceed the array size and the write would overflow `agg_arr[]`.

   The patch replaces the assert with:
   ```c
   if (unlikely(tpa_info->agg_count >= TPA_MAX_NUM_SEGS)) {
       PMD_DRV_LOG_LINE(ERR, "TPA abuf: agg_count %u exceeds max %u",
                        tpa_info->agg_count, TPA_MAX_NUM_SEGS);
       tpa_info->agg_count = 0;
       bnxt_sched_ring_reset(rxq);
       rc = -EINVAL;
       goto next_rx;
   }
   ```

   This is correct. The check always runs (not compiled out), logs the error, resets `agg_count` to prevent further writes, schedules a ring reset, and continues processing without writing to the array.

2. **Out-of-bounds access in `bnxt_rx_descriptor_status_op()`**

   The original code did:
   ```c
   cons = rxcmp->opaque;
   if (rxr->rx_buf_ring[cons])
       return RTE_ETH_RX_DESC_DONE;
   ```

   `rxcmp->opaque` is a firmware-supplied value that should be treated as an index into a ring. Ring indices must be masked to the ring size to handle wrap-around. The patch changes this to:
   ```c
   cons = RING_IDX(rxr->rx_ring_struct, rxcmp->opaque);
   if (rxr->rx_buf_ring[cons])
       return RTE_ETH_RX_DESC_DONE;
   ```

   `RING_IDX()` masks the index to the ring size. This is correct.

3. **Unclamped return value in `bnxt_hwrm_func_vf_vnic_query()`**

   The original code did:
   ```c
   rc = rte_le_to_cpu_32(resp->vnic_id_cnt);
   HWRM_UNLOCK();
   return rc;
   ```

   The caller uses the returned count to iterate over a VNIC ID buffer of size `bp->pf->total_vnics`. If firmware returned a count exceeding that size, the caller would read past the end of the buffer. The patch clamps the return value:
   ```c
   rc = RTE_MIN(rte_le_to_cpu_32(resp->vnic_id_cnt),
                (uint32_t)bp->pf->total_vnics);
   ```

   This is correct. It prevents the caller from iterating beyond the buffer.

### Warnings
None.

### Info

The patch correctly fixes three out-of-bounds issues: the TPA agg_count overflow, the unchecked ring index, and the unclamped VNIC count. The use of `unlikely()` in the agg_count check is appropriate (this condition should be rare if the NIC is functioning correctly).

---

## Summary

**Total Errors Found:** 10 (all corrected by the patch)
**Total Warnings:** 0

All five patches are correct and fix real bugs. Patch 3 additionally fixes three bugs it would have introduced (resource leak, missing unlocks, missing flag clear). No style, process, or documentation issues were identified.