On Sun, 20 Sep 2026 20:24:18 -0600 Mohammad Shuab Siddique <[email protected]> wrote:
> From: Keegan Freyhof <[email protected]> > > 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 <[email protected]> > Signed-off-by: Mohammad Shuab Siddique <[email protected]> > > --- [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: [email protected].
