|WARNING| [5/5] common/cnxk: fix OOB write in rep count copy

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-11-02

# DPDK Patch Review

Reviewing patch series containing 5 patches fixing resource management issues in common/cnxk driver.

---

## PATCH 1/5: common/cnxk: fix macsec teardown use-after-free

### Errors

**Use-after-free race condition fixed correctly:**
The patch adds proper locking around the MCS device lookup and callback invocation to prevent the interrupt handler from accessing freed memory during teardown. The implementation is correct:
- Lock held across lookup + callback in interrupt handler
- Lock held during list insertion in init
- Lock held during TAILQ_REMOVE in fini, ensuring no in-flight interrupt can see a dangling pointer

No errors in this patch.

### Warnings

None.

### Info

The lock granularity is appropriate for the access pattern. The interrupt handler snapshots the `mcs` pointer under lock before calling `mcs_event_cb_process()`, which is safe because the callback list is walked without holding the lock (callbacks are expected to be stable once registered).

---

## PATCH 2/5: common/cnxk: fix TM schq alloc OOB write

### Errors

**Bounds check and error propagation:**
The patch correctly addresses two related issues:
1. Clamping `rsp->schq[lvl]` and `rsp->schq_contig[lvl]` to `MAX_TXSCHQ_PER_FUNC` prevents over-reading `schq_list[lvl][]` and `schq_contig_list[lvl][]` arrays in the response structure
2. Validating each `schq id < NIX_TM_MAX_HW_TXSCHQ` before calling `plt_bitmap_set()` prevents out-of-bounds writes to the bitmap

The error propagation from `nix_tm_copy_rsp_to_nix()` is correctly implemented in both call sites (`nix_tm_alloc_txschq()` and `roc_nix_tm_prealloc_res()`).

No errors in this patch.

### Warnings

None.

### Info

The use of `PLT_MIN()` macro for clamping is idiomatic DPDK. The explicit cast to `uint16_t` in `PLT_MIN(rsp->schq[lvl], (uint16_t)MAX_TXSCHQ_PER_FUNC)` matches the types involved (both are `uint16_t` in the response structure).

---

## PATCH 3/5: common/cnxk: fix null callback deref in CPT IRQ

### Errors

**NULL check on function pointer before call:**
The patch correctly snapshots `inl_dev->work_cb` once and checks it before dispatch:

```c
work_cb = inl_dev->work_cb;
/* ... loop processing entries ... */
if (work_cb)
    work_cb(gw, sa, cq_type, (void *)cq_s, port_id);
```

This prevents the NULL function pointer dereference if the callback is unregistered or not yet registered. The snapshot pattern ensures the test and call use the same pointer value.

No errors in this patch.

### Warnings

None.

### Info

The CQ entries are still drained and the interrupt acknowledged even when `work_cb` is NULL, which is correct: the hardware state must be cleared to avoid stalling the queue.

---

## PATCH 4/5: common/cnxk: fix OOB access in RQ FC config

### Errors

**Bounds check and NULL check before array access:**
The patch adds two checks before indexing `nix->rqs[]` and dereferencing the result:

```c
if (fc_cfg->rq_cfg.rq >= nix->nb_rx_queues)
    return NIX_ERR_QUEUE_INVALID_RANGE;

rq = nix->rqs[fc_cfg->rq_cfg.rq];
if (rq == NULL)
    return NIX_ERR_PARAM;
```

This prevents:
1. Out-of-bounds read from `nix->rqs[]` if `rq >= nb_rx_queues`
2. NULL pointer dereference if the queue slot is uninitialized

The error codes match those used in sibling ROC helpers (`roc_nix_rq_init()` uses `NIX_ERR_QUEUE_INVALID_RANGE` for the same check).

No errors in this patch.

### Warnings

None.

---

## PATCH 5/5: common/cnxk: fix OOB write in rep count copy

### Errors

**Bounds check on mailbox response count:**
The patch validates `rsp->rep_cnt` against `MAX_PFVF_REP` before copying into the fixed-size `roc_nix->rep_pfvf_map[MAX_PFVF_REP]` array:

```c
if (rsp->rep_cnt > MAX_PFVF_REP) {
    plt_err("Invalid rep_cnt %u in get_rep_cnt rsp", rsp->rep_cnt);
    rc = -EIO;
    goto exit;
}
```

This prevents buffer overflow when copying `rsp->rep_pfvf_map[i]` entries. The error code `-EIO` is appropriate for a malformed hardware/firmware response.

No errors in this patch.

### Warnings

None.

---

## Series-Wide Observations

### Style and Formatting

All patches comply with DPDK C style:
- Proper comparison against NULL (`if (idev == NULL)`, `if (rq == NULL)`)
- Explicit comparison in conditionals (`if (work_cb)` is acceptable for function pointers, though the snapshot pattern is used correctly)
- Error messages use `plt_err()` with appropriate format strings
- No forbidden tokens or deprecated API usage
- Proper use of `goto` for error cleanup in PATCH 1/5 and PATCH 5/5

### Documentation

Missing release notes updates: These are bug fixes that should be documented in the current release notes file under a "Fixed Issues" or "Bugs Fixed" section. Each fix addresses a potentially severe issue (use-after-free, buffer overflow, NULL deref) that applications could encounter.

**Expected documentation location:** `doc/guides/rel_notes/release_XX_YY.rst` (where XX_YY is the current release version)

**Expected content:**
```rst
* **Fixed use-after-free in cnxk MACsec teardown.**

  Added locking to prevent race between MCS interrupt handler and device teardown.

* **Fixed out-of-bounds access in cnxk TM scheduler allocation.**

  Added bounds checking on hardware-supplied scheduler IDs to prevent bitmap corruption.

* **Fixed NULL pointer dereference in cnxk CPT interrupt handler.**

  Added NULL check before invoking completion callback.

* **Fixed out-of-bounds access in cnxk RQ flow control configuration.**

  Added queue index validation before accessing queue array.

* **Fixed buffer overflow in cnxk representor count handling.**

  Added bounds checking on firmware-supplied representor count.
```

### Testing

The patches do not include test additions, but these are fixes for existing code paths. The bugs are all in error/race paths that would be difficult to trigger reliably in unit tests:
- Interrupt timing races
- Malformed firmware responses
- Invalid user-supplied indices

Manual testing or fault injection would be required to verify the fixes.

---

## Summary

All five patches correctly address the stated bugs with appropriate bounds checks, error propagation, and locking. The fixes are minimal and focused on the specific issues. The code follows DPDK style and uses appropriate error codes.

**Required fix:** Add release notes documentation for all five fixes.

Review-Result: WARNING