|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 introduces a new helper function `tfc_str_append()` to prevent out-of-bounds writes in debug dump routines that were using unchecked `snprintf()` return value accumulation or `strcat()`. The core fix is sound and addresses real OOB vulnerabilities, but there are implementation and style issues to address.

---

## Errors

### 1. tfc_str_append() treats truncation as partial success instead of an error

**Issue:** `tfc_str_append()` appends `"<truncated>"` when truncation occurs and then returns `size - 1`, indicating that `size - 1` bytes were "successfully" written. The calling code then increments the offset by this value and attempts to append more data at `buf + (size - 1)`, which is the position of the NUL terminator. The next call to `tfc_str_append()` with `size - remaining` will have only 1 byte of space (the NUL slot), causing it to immediately re-append `"<truncated>"` over itself on every subsequent call.

**Why it matters:** Once truncation occurs, all further appends silently fail. The function's design assumes callers will continue appending after truncation, but the arithmetic breaks: if the buffer is full, `remaining = size - off = 0`, and the next call gets `size = 0`, which the function treats as "do nothing." Worse, if a caller does `off += tfc_str_append(buf + off, size - off, ...)`, the truncation marker gets overwritten repeatedly.

**Fix:** Two design choices are viable:

1. **Stop-on-truncation model:** Make `tfc_str_append()` return 0 once truncation has occurred (check if the last byte is `'\0'` and the second-to-last is the start of the marker). Callers then naturally stop appending when `off` stops advancing.

2. **Error-propagation model:** Return `size` on truncation (not `size - 1`), so `off += ret` makes `off >= size`, and subsequent calls with `size - off` get `size = 0` or negative (clamp to 0).

Preferred fix (stop-on-truncation):

```c
size_t tfc_str_append(char *buf, size_t size, const char *fmt, ...)
{
	const char *mark = "<truncated>";
	size_t marklen = strlen(mark);
	va_list ap;
	int n;

	if (size == 0)
		return 0;

	/* Check if already truncated: last char is NUL and we can find the marker */
	if (size > marklen && buf[size - 1] == '\0' && 
	    strncmp(buf + size - 1 - marklen, mark, marklen) == 0)
		return 0;  /* Already truncated, refuse further writes */

	va_start(ap, fmt);
	n = vsnprintf(buf, size, fmt, ap);
	va_end(ap);

	if (n < 0)
		return 0;
	if ((size_t)n >= size) {
		/* Truncation: overwrite tail with marker if space permits */
		if (size > marklen)
			snprintf(buf + (size - 1 - marklen), marklen + 1, "%s", mark);
		return 0;  /* Signal truncation by returning 0 */
	}
	return (size_t)n;
}
```

Alternatively, simpler approach: just return `size - 1` on truncation (as written) but document that callers should check `if (off >= size - marklen)` before continuing. However, the current call sites don't do this, so they would need updating.

**Alternative simpler fix:** Remove the `"<truncated>"` marker entirely and just return `size - 1` on truncation. The buffer ends with a valid NUL, and the caller's loop naturally stops when `off + len >= size`. This is the minimal safe change.

---

### 2. `tfc_str_append()` casts negative return value from `vsnprintf()` to `size_t` (wraparound)

**Issue:** Line 1043:
```c
if ((size_t)n >= size) {
```
If `vsnprintf()` returns `-1` on encoding error, the cast to `size_t` produces `SIZE_MAX`, which is `>= size`, so the function correctly enters the truncation path. However, the truncation path then returns `size - 1`, which is a valid offset. A subsequent append at `buf + (size - 1)` would be at the NUL terminator, and the append would overwrite it.

**Fix:** Return 0 on encoding error instead of proceeding to truncation logic:

```c
if (n < 0)
	return 0;  /* Encoding error */
if ((size_t)n >= size) {
	/* Truncation */
	...
	return 0;  /* or size - 1, depending on chosen design */
}
return (size_t)n;
```

The patch already has `if (n < 0) return 0;` at line 1042, so this is correct. The issue is that the subsequent `if ((size_t)n >= size)` is redundant and confusing. Simplify to `if (n >= (int)size)` or just rely on the `n < 0` check being sufficient.

Actually, re-reading: the current code at lines 1042-1043 has:
```c
if (n < 0)
	return 0;
if ((size_t)n >= size) {
```
This is correct: if `n < 0`, we return 0; otherwise, `n` is non-negative, so the cast is safe. No issue here.

**Retraction:** This is not an error. The `n < 0` check is present. Delete this item.

---

### 3. Missing `#include <string.h>` for `strlen()` and `strncmp()`

**Issue:** `tfc_str_append()` calls `strlen(mark)` but `string.h` is not explicitly included in the new code added to `tfc_mpc_debug.c`. The file does have `#include <string.h>` at line 8, so this is fine. Not an error.

**Retraction:** Delete this item.

---

### 4. Integer overflow in offset arithmetic when appending to 256-byte buffers

**Issue:** Throughout the modified code, `size_t off1, off2, ...` are accumulated by:
```c
off1 += tfc_str_append(line1 + off1, TFC_STRING_LENGTH_256 - off1, "%s", tmp1);
```
If `tfc_str_append()` is broken (returns `size - 1` on truncation as discussed above), then `off1` can exceed `TFC_STRING_LENGTH_256`, and `TFC_STRING_LENGTH_256 - off1` underflows to a huge `size_t` value. However, `tfc_str_append()` then receives `buf = line1 + off1`, which is past the end of `line1`, causing OOB write.

**Why it matters:** If the function returns a value causing `off > size`, the next call's `size - off` underflows.

**Fix:** This is a consequence of issue #1. If `tfc_str_append()` returns 0 on truncation, the offset stops advancing and the underflow cannot occur. Alternatively, clamp the size calculation:

```c
size_t remaining = (off1 < TFC_STRING_LENGTH_256) ? (TFC_STRING_LENGTH_256 - off1) : 0;
off1 += tfc_str_append(line1 + off1, remaining, "%s", tmp1);
```

But the cleaner fix is to make `tfc_str_append()` return 0 on truncation.

---

## Warnings

### 1. Variable initialization not needed for `off1, off2, ...` when first use is assignment

**Issue:** Lines like:
```c
size_t off1, off2, off3, off4;
...
off1 = tfc_str_append(line1, TFC_STRING_LENGTH_256, ...);
```
The variables are declared uninitialized but the first use is an assignment, so no initialization is needed. This is correct as written. However, some later sections (e.g., `prof_tcam_show()` line 1006) have:
```c
size_t offh = 0, off1 = 0, ...
```
but the first use is:
```c
offh = tfc_str_append(lineh, TFC_STRING_LENGTH_256, ...);
```
The initialization to 0 is unnecessary.

**Fix:** Remove the `= 0` initializers when the first use is an assignment:
```c
size_t offh, off1, off2, off3, off4, off5;
```

**Note:** This is a minor style issue. Some developers prefer initializing to 0 defensively, and DPDK style does not forbid it. This is a suggestion, not a requirement.

---

### 2. `strncmp()` in suggested fix for issue #1 is not available

If you adopt the "stop-on-truncation" fix in Error #1 that uses `strncmp()`, note that `<string.h>` is included so `strncmp()` is available. No issue.

**Retraction:** Not applicable if the fix is simplified. Delete this item.

---

### 3. Truncation marker length is computed at runtime on every call

**Issue:** `tfc_str_append()` line 1034:
```c
size_t marklen = strlen(mark);
```
This computes the length of `"<truncated>"` on every call. Since `mark` is a string literal, `strlen(mark)` is a compile-time constant (12).

**Fix:** Make `marklen` a macro or use `sizeof("<truncated>") - 1`:
```c
#define TRUNCATION_MARK "<truncated>"
#define TRUNCATION_MARK_LEN (sizeof(TRUNCATION_MARK) - 1)
```
or inline:
```c
size_t marklen = sizeof("<truncated>") - 1;  /* 12 */
```

**Note:** Modern compilers optimize `strlen("literal")` to a constant, so this is a micro-optimization. However, using `sizeof() - 1` makes the intent explicit.

---

### 4. `snprintf()` at line 1044 checks for truncation but the marker append does not verify success

**Issue:** Line 1044-1046:
```c
if (size > marklen)
	snprintf(buf + (size - 1 - marklen), marklen + 1, "%s", mark);
```
If `snprintf()` fails (returns -1), the function still returns `size - 1`, claiming success.

**Fix:** Check the return value:
```c
if (size > marklen) {
	int ret = snprintf(buf + (size - 1 - marklen), marklen + 1, "%s", mark);
	if (ret < 0 || (size_t)ret >= marklen + 1)
		/* Marker append failed, buffer state uncertain */
		buf[0] = '\0';  /* Safeguard: clear buffer */
}
return size - 1;
```

**Counterargument:** `snprintf()` with a fixed format string `"%s"` and a static string `mark` cannot fail on any sane platform. The return value will be the length of `mark` (12). This check adds no value in practice.

**Verdict:** This is a defensive check that does not hurt. The function is in a debug/diagnostic path (not fast path), so the extra conditional is acceptable. However, this is a Warning, not an Error.

---

### 5. Inconsistent use of `tfc_str_append()` vs raw `snprintf()` in some blocks

**Issue:** In `prof_tcam_show()` and `wc_tcam_show()`, the first few lines use `tfc_str_append()`, but some later lines (e.g., `snprintf(tmp1, ...)`) are not followed by `tfc_str_append()` calls. Instead, they rely on `strcat()` being replaced with `tfc_str_append(..., "%s", tmp)`. This is correct.

However, in `mod_decode()` at line 793, the code does:
```c
snprintf(str, TFC_STRING_LENGTH_256, "%s: ", mod_data[vect].name);
mod_off += tfc_str_append(mod_str + mod_off, TFC_MOD_STRING_LENGTH - mod_off, "%s", str);
```
This could be simplified to:
```c
mod_off += tfc_str_append(mod_str + mod_off, TFC_MOD_STRING_LENGTH - mod_off, "%s: ", mod_data[vect].name);
```
avoiding the temporary `str` buffer.

**Fix:** This is an optimization, not a correctness issue. The code works as written. Simplification would reduce temporary buffer usage but is not required.

**Verdict:** Info-level suggestion. Not flagging.

---

### 6. `stat_decode()` has two separate `size_t str_off` increment blocks that could lead to confusion

**Issue:** In `stat_decode()` (line 1216), `str_off` is declared, set by the initial `tfc_str_append()`, then incremented in each `switch` case. If a case forgets to increment, the next append overwrites the previous one.

However, the code structure is clear: each case appends once and increments. The pattern is consistent. No issue.

**Retraction:** Delete this item.

---

## Info

### 1. Function prototype in header should document truncation behavior

**Suggestion:** Add Doxygen comment for `tfc_str_append()` in `tfc_debug.h` explaining:
- Returns number of characters appended (excluding NUL)
- Returns 0 on truncation or error
- Appends `"<truncated>"` marker if space permits
- Safe to chain calls: `off += tfc_str_append(buf + off, size - off, ...)`

Example:
```c
/**
 * Append formatted string to buffer with bounds checking.
 *
 * @param buf
 *   Destination buffer
 * @param size
 *   Size of destination buffer
 * @param fmt
 *   printf-style format string
 * @return
 *   Number of characters appended (excluding NUL terminator).
 *   Returns 0 if truncation occurs or on encoding error.
 *   If truncation occurs, appends "<truncated>" marker at end of buffer.
 */
size_t tfc_str_append(char *buf, size_t size, const char *fmt, ...);
```

---

### 2. Consider using a macro for repeated append patterns

**Suggestion:** The pattern:
```c
off += tfc_str_append(buf + off, SIZE - off, "%s", tmp);
```
appears dozens of times. A macro could reduce repetition:
```c
#define TFC_APPEND(buf, off, size, ...) \
	do { \
		off += tfc_str_append((buf) + (off), (size) - (off), __VA_ARGS__); \
	} while (0)

/* Usage */
TFC_APPEND(line1, off1, TFC_STRING_LENGTH_256, "%s", tmp1);
```

However, macros can hide control flow and make debugging harder. This is a style preference, not a requirement.

---

### 3. Debug code size is significant; consider dynamic allocation

**Observation:** The `*_show()` functions allocate multiple 256-byte stack arrays (`line1`, `line2`, etc.). For `prof_tcam_show()`, this is 6 x 256 = 1536 bytes of stack. The debug routines are not called in fast paths, so dynamic allocation (`malloc()`) might be more appropriate, or a single larger buffer could be reused.

However, this is debug code, and stack usage is unlikely to be a problem. No change required.

---

## Summary of Required Fixes

**Must fix:**
1. **Error #1:** `tfc_str_append()` truncation handling allows offset to exceed size, causing underflow in `size - off` and subsequent OOB writes. Fix by returning 0 on truncation so offset stops advancing.
2. **Error #4:** Offset arithmetic underflows when `off > size` due to broken `tfc_str_append()` return value. Fixed by issue #1.

**Should fix:**
1. **Warning #1