Re: [PATCH v2 3/5] net/bnxt: harden sprintf bounds for device memory names

On Sun, 20 Sep 2026 20:24:18 -0600
Mohammad Shuab Siddique [off-list ref] wrote:

From: Keegan Freyhof <redacted>

sprintf() into fixed-size stack buffers such as
char type[RTE_MEMZONE_NAMESIZE] does not bound the write to the
buffer, so a long enough formatted string (e.g. from PCI address
fields) overflows it.

Add check_snprintf_rc(), a helper that logs and returns an error on a
failed snprintf() call and logs (without failing) a truncated one.
Convert sprintf() calls building a memzone/malloc name to snprintf()
plus this check, and add the same check to the existing snprintf()
calls building HWRM CFA pair_name request fields. Unlike a truncated
memzone/malloc label, a truncated pair_name would be sent to firmware
and could match the wrong pair or none at all, so
bnxt_hwrm_cfa_pair_exists()/_alloc()/_free() additionally reject a
truncated pair_name outright instead of proceeding.

Three bugs introduced by this change and fixed here: in
bnxt_hwrm_ver_get(), free bp->hwrm_short_cmd_req_addr (and null it)
before checking the new snprintf's return, instead of after, so an
early return on a snprintf failure doesn't leak the previous
allocation; also clear BNXT_FLAG_SHORT_CMD on that same early return,
since it may already have been set a few lines above and would
otherwise claim short-command support with no buffer allocated. In
bnxt_hwrm_cfa_pair_exists()/_alloc()/_free(), the new early-return
paths exited without releasing bp->hwrm_lock (held since the
preceding HWRM_PREP()), which would deadlock every later HWRM call;
added the missing HWRM_UNLOCK() before each return.

Signed-off-by: Keegan Freyhof <redacted>
Signed-off-by: Mohammad Shuab Siddique <redacted>

---

[PATCH v2 3/5] net/bnxt: harden sprintf bounds for device memory
names

Error: does not apply to main (see summary).

Warning: the rc < 0 branch of check_snprintf_rc() is unreachable.
snprintf() only fails on encoding errors, which cannot happen with
these formats. Every converted name except pair_name is an
rte_malloc()/rte_zmalloc_socket() type label. That label is
informational only, so truncation is harmless.

The only real overflow is a PCI domain above 0xffff with
"bnxt_hwrm_short_": 16 + 8 + 8 = 32 chars plus NUL into 32 bytes.
Plain snprintf() fixes that. Drop the helper and the early-return
paths, including the flag clearing and unlock handling added for
unreachable code.

For pair_name, rejecting truncation is reasonable. A single
"if (snprintf(...) >= sizeof(req.pair_name))" with HWRM_UNLOCK()
covers it.

Warning: the commit body carries review history ("Three bugs
introduced by this change and fixed here..."). Move it below ---.

Info: the flow xstat names can be written with snprintf() directly
into xstats_names[count].name, dropping buf and strlcpy(). "_%d_%d"
cannot exceed 32 bytes, so it needs no check.

Info: if the PCI domain overflow is the motivation, add Fixes: and
Cc: stable@dpdk.org.