|WARNING| net/bnxt: fix potential OOB string writes in debug helpers

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review - net/bnxt: fix potential OOB string writes in debug helpers

## Summary

This patch adds bounds checking to debug-dump string formatting routines to prevent out-of-bounds writes. The core change introduces `tfc_str_append()`, a bounds-checked `snprintf()` wrapper, and uses it throughout `tfc_mpc_debug.c` and `tfc_tcam_debug.c` to replace unsafe string accumulation patterns.

---

## Correctness Issues

### Errors

**1. Resource leak on buffer allocation failure**

In `em_show()` and `wc_tcam_show()`, four 256-byte buffers are allocated via `rte_malloc()`, but if any allocation fails, the function returns early without freeing already-allocated buffers.

```c
/* tfc_mpc_debug.c:605 */
line1 = rte_malloc("line1", TFC_STRING_LENGTH_256, 0);
line2 = rte_malloc("line2", TFC_STRING_LENGTH_256, 0);
line3 = rte_malloc("line3", TFC_STRING_LENGTH_256, 0);
line4 = rte_malloc("line4", TFC_STRING_LENGTH_256, 0);

if (!line1 || !line2 || !line3 || !line4) {
    PMD_DRV_LOG_LINE(ERR, "Memory alloc failed during EM dump");
    return;  /* BUG: earlier allocations leak */
}
```

**Fix:** Free all previously allocated buffers before returning. Use a `goto cleanup` pattern or free in reverse order on the error path:

```c
if (!line1 || !line2 || !line3 || !line4) {
    PMD_DRV_LOG_LINE(ERR, "Memory alloc failed during EM dump");
    rte_free(line4);
    rte_free(line3);
    rte_free(line2);
    rte_free(line1);
    return;
}
```

Same issue exists in `wc_tcam_show()` at line 1452.

---

**2. Unnecessary use of `rte_malloc()` in control path**

Both `em_show()` and `wc_tcam_show()` allocate 256-byte line buffers with `rte_malloc()`, but these are short-lived local buffers in a debug/control path that never need DMA access or process sharing. Standard `malloc()` is preferred here (faster, doesn't consume hugepage resources).

```c
/* BAD - control path using rte_malloc */
line1 = rte_malloc("line1", TFC_STRING_LENGTH_256, 0);

/* GOOD */
line1 = malloc(TFC_STRING_LENGTH_256);
```

---

## Style and Process Issues

### Warnings

**1. Missing error check on `malloc()` / `rte_malloc()` return values reintroduced**

The patch converts `em_show()` and `wc_tcam_show()` from using unchecked `char line[256]` stack arrays to dynamically allocated buffers, and correctly checks for allocation failure. However, if the recommendation to switch to `malloc()` is followed, the error handling remains correct -- no additional issue here. This is actually well-handled in the patch as written.

**2. `rte_free()` used instead of `free()` if switching allocator**

If the allocations are changed to `malloc()` as recommended above, the corresponding `rte_free()` calls must become `free()`. The patch uses `rte_free()` consistently with `rte_malloc()`, which is correct for the current code.

---

### Info (Suggestions)

**1. `tfc_str_append()` return value handling could be more defensive**

In `tfc_str_append()`, when `vsnprintf()` returns a negative value (encoding error), the function returns 0. This is safe, but the caller's offset tracking will silently stop accumulating. Consider whether a warning log or explicit marker would help debug such cases, though this is unlikely in practice.

**2. Consider stack allocation for line buffers**

The 256-byte line buffers in `em_show()` and `wc_tcam_show()` are small enough for stack allocation, which would eliminate the allocation failure path entirely and simplify the code. The original code used stack arrays; the switch to dynamic allocation may have been unnecessary.

```c
/* Original (simpler, no alloc failure path) */
char line1[TFC_STRING_LENGTH_256];
char line2[TFC_STRING_LENGTH_256];
/* ... */
```

If stack allocation is undesirable (e.g., due to stack size constraints in the calling context), the dynamic allocation is justified, but then use `malloc()` not `rte_malloc()`.

---

## Positive Aspects

- **Correct bounds-checking approach:** The `tfc_str_append()` wrapper correctly clamps `snprintf()`'s return value to the available buffer space, preventing offset overflow.
- **Thorough conversion:** All unsafe `strcat()` and raw `snprintf()` + offset accumulation patterns are replaced consistently across both files.
- **Truncation marker:** The `"<truncated>"` suffix on overflow is a good debugging aid.
- **Proper fix for the reported issue:** The patch directly addresses the CVE-level out-of-bounds write vulnerability.

---

## Final Recommendations

1. **Must fix (Error):** Add cleanup of allocated buffers on allocation failure in both `em_show()` and `wc_tcam_show()`.
2. **Should fix (Warning):** Replace `rte_malloc()` with `malloc()` for these control-path buffers (or revert to stack allocation).
3. **Consider:** Whether 256-byte stack allocation is acceptable here, which would eliminate the allocation failure path entirely.

---

## Patch Compliance Summary

- **Correctness:** 1 resource leak error, 1 inappropriate `rte_malloc()` usage
- **Style:** Compliant (no forbidden tokens, proper naming, formatting correct)
- **Documentation:** Not required for internal debug helpers
- **Testing:** Not applicable (debug dump functions)