|WARNING| net/bnxt: fix out-of-bounds read in TCAM get

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-05

# DPDK Patch Review: net/bnxt: fix out-of-bounds read in TCAM get

## Summary
This patch fixes a critical buffer overflow vulnerability in the TCAM get operation.
The fix adds proper bounds checking, corrects byte order handling, and ensures early return on error.

---

## Errors

**1. Missing byte order conversion in array index calculation**

The array indexing in the final memcpy operations uses `*key_size` and `*remap_size`,
which now contain host-byte-order converted values.
However, the bounds check validates `resp.dev_data` against converted sizes,
so the memcpy indexing is correct.

Actually, re-reading the code: the memcpy operations are correct.
After the conversions at lines 1173-1174, `*key_size` and `*remap_size` hold host byte order values,
and those are what is used for the memcpy lengths and offsets.
This is correct.

*(Self-correction: this is not an error. Omitting from output.)*

---

## Warnings

**1. Integer overflow in bounds check not prevented by cast alone**

Line 1159-1160:
```c
if ((size_t)rte_le_to_cpu_16(resp.key_size) * 2 +
    (size_t)rte_le_to_cpu_16(resp.result_size) > sizeof(resp.dev_data)) {
```

The cast to `size_t` occurs after `rte_le_to_cpu_16()` returns a 16-bit value.
If `resp.key_size` is 0x8000 or larger (from malicious firmware),
`key_size * 2` could overflow before the `size_t` cast,
though this would require a 16-bit overflow which wraps to a small value,
making the check pass when it should fail.

**Suggested fix:** Cast before multiplication:
```c
if ((size_t)rte_le_to_cpu_16(resp.key_size) * 2U +
    (size_t)rte_le_to_cpu_16(resp.result_size) > sizeof(resp.dev_data)) {
```
The `2U` ensures the multiplication is performed in `size_t` width.

**2. Inconsistent error message format**

Lines 1161-1162, 1167:
```c
PMD_DRV_LOG_LINE(ERR, "%s: FW sizes exceed TCAM bounds, rc:%d",
			 __func__, rc);
```
The message says "rc:%d" but `rc` is -EINVAL (a negative error code).
DPDK error messages typically express the error as a positive value
or use the symbolic name.

**Suggested fix:** Either use `%s` with `strerror(-rc)` or positive value:
```c
PMD_DRV_LOG_LINE(ERR, "%s: FW sizes exceed TCAM bounds: %s",
			 __func__, strerror(-rc));
```
Or drop the rc entirely since it's always -EINVAL in this context:
```c
PMD_DRV_LOG_LINE(ERR, "%s: FW sizes exceed TCAM bounds",
			 __func__);
```

---

## Info

**1. Consider adding defensive assert on sizeof(resp.dev_data)**

The bounds check assumes `resp.dev_data` is the data array.
If the response structure changes in the future,
the check could silently become incorrect.
Consider adding a compile-time assertion to document the expected size relationship.

**2. Comment explaining the dev_data layout would improve maintainability**

The calculation `key_size * 2 + result_size` encodes the layout
(key, mask, remap) but this is implicit.
A brief comment would help future reviewers:
```c
/* dev_data layout: [key][mask][remap] */
if ((size_t)rte_le_to_cpu_16(resp.key_size) * 2U + ...
```

---

## Positive Observations

- The patch correctly identifies and fixes a serious security vulnerability
  (out-of-bounds read, potential buffer overflow)
- Early return pattern on error prevents subsequent memcpy operations
  from using invalid lengths
- Byte order conversion is now correctly applied to all uses
  of firmware-supplied sizes
- The fix validates both buffer capacity (caller's buffers)
  and source bounds (firmware response buffer)
- The cast to `size_t` at line 1159 addresses the `-Wsign-compare` warning
  mentioned in the commit message

---

## Final Recommendation

The patch is a significant security improvement and should be merged
after addressing the integer overflow concern in Warning #1.
The message formatting in Warning #2 is a minor style issue.