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

[Stephen Hemminger <stephen@networkplumber.org>]

On Sun, 20 Sep 2026 20:24:18 -0600
Mohammad Shuab Siddique <mohammad-shuab.siddique@broadcom.com> wrote:

> From: Keegan Freyhof <keegan.freyhof@broadcom.com>
> 
> 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 <keegan.freyhof@broadcom.com>
> Signed-off-by: Mohammad Shuab Siddique <shuab.siddique@broadcom.com>
> 
> ---


[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.