|SUCCESS| [1/1] net/bnxt: fix string bounds checks and format specifiers

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163281

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-24

## Review of net/bnxt: fix string bounds checks and format specifiers

commit 1776401825 (assuming based on DKIM timestamp)
Author: Denis Lyulin <lyulin.2003 at mail.ru>
Subject: net/bnxt: fix string bounds checks and format specifiers

This patch converts sprintf calls to snprintf for memzone name creation
and updates format specifiers to use explicit width qualifiers.

> diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> index b677f9491d..bb02a2579b 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -652,8 +652,9 @@ static int bnxt_init_fc_ctx_mem(struct bnxt *bp)
>  
>  	max_fc = bp->flow_stat->max_fc;
>  
> -	sprintf(type, "bnxt_rx_fc_in_" PCI_PRI_FMT, pdev->addr.domain,
> -		pdev->addr.bus, pdev->addr.devid, pdev->addr.function);
> +	snprintf(type, RTE_MEMZONE_NAMESIZE, "bnxt_rx_fc_in_" PCI_PRI_FMT,
> +		pdev->addr.domain, pdev->addr.bus,
> +		pdev->addr.devid, pdev->addr.function);

The conversion to snprintf is good, but does this code handle truncation
properly? If the resulting string exceeds RTE_MEMZONE_NAMESIZE-1, the
name will be silently truncated. Should the function check snprintf's
return value and fail gracefully if the name is too long?

The same concern applies to all similar conversions in this function.

[ ... ]

> @@ -3252,7 +3256,7 @@ bnxt_fw_version_get(struct rte_eth_dev *dev, char *fw_version, size_t fw_size)
>  	uint8_t fw_rsvd = bp->fw_ver & 0xff;
>  	int ret;
>  
> -	ret = snprintf(fw_version, fw_size, "%d.%d.%d.%d",
> +	ret = snprintf(fw_version, fw_size, "%hhu.%hhu.%hhu.%hhu",
>  			fw_major, fw_minor, fw_updt, fw_rsvd);

Is the format specifier change necessary here? The variables fw_major,
fw_minor, fw_updt, and fw_rsvd are all uint8_t. Using %d will correctly
format these values due to integer promotion, while %hhu adds complexity.
The coding style guide typically recommends simpler format strings when
the output is identical.

[ ... ]

> @@ -5199,13 +5203,13 @@ static int bnxt_alloc_ctx_mem_blk(struct bnxt *bp,
>  		RTE_ALIGN_MUL_CEIL(mem_size, BNXT_PAGE_SIZE) / BNXT_PAGE_SIZE;
>  	rmem->page_size = BNXT_PAGE_SIZE;
>  
> -	snprintf(name, RTE_MEMZONE_NAMESIZE, "bnxt_ctx_pg_arr%s_%x_%d",
> +	snprintf(name, RTE_MEMZONE_NAMESIZE, "bnxt_ctx_pg_arr%s_%hx_%hu",
>  		 suffix, idx, bp->eth_dev->data->port_id);

What are the types of idx and bp->eth_dev->data->port_id? The change to
%hx and %hu implies they are uint16_t, but without seeing the variable
declarations, can you confirm these format specifiers match the actual
types? Mismatched format specifiers can cause undefined behavior.

The same question applies to similar changes throughout this function.

[ ... ]

> @@ -5393,7 +5397,7 @@ int bnxt_alloc_ctx_pg_tbls(struct bnxt *bp)
>  		for (i = 0; i < w && rc == 0; i++) {
>  			char name[RTE_MEMZONE_NAMESIZE] = {0};
>  
> -			sprintf(name, "_%d_%d", i, type);
> +			snprintf(name, RTE_MEMZONE_NAMESIZE, "_%d_%hu", i, type);

This snprintf conversion is good, but the format specifier change
requires verification. What is the type of the 'type' variable? Does
it match uint16_t (%hu)? Also, does this code check for truncation?
If the resulting name exceeds the buffer, should an error be returned?

[ ... ]

> @@ -6977,7 +6981,7 @@ static int bnxt_rep_port_probe(struct rte_pci_device *pci_dev,
>  		}
>  
>  		/* representor port net_bdf_port */
> -		snprintf(name, sizeof(name), "net_%s_representor_%d",
> +		snprintf(name, sizeof(name), "net_%s_representor_%hu",
>  			 pci_dev->device.name, eth_da->representor_ports[i]);

What is the type of eth_da->representor_ports[i]? Does it match uint16_t
(%hu)? If this is actually a larger type like uint32_t, the format
specifier mismatch could silently truncate values or cause undefined
behavior.

> diff --git a/drivers/net/bnxt/bnxt_hwrm.c b/drivers/net/bnxt/bnxt_hwrm.c
> index 0c82935de9..5c51108b65 100644
> --- a/drivers/net/bnxt/bnxt_hwrm.c
> +++ b/drivers/net/bnxt/bnxt_hwrm.c
> @@ -1693,7 +1693,8 @@ int bnxt_hwrm_ver_get(struct bnxt *bp, uint32_t timeout)
>  	     (dev_caps_cfg &
>  	      HWRM_VER_GET_OUTPUT_DEV_CAPS_CFG_SHORT_CMD_REQUIRED)) ||
>  	    bp->hwrm_max_ext_req_len > HWRM_MAX_REQ_LEN) {
> -		sprintf(type, "bnxt_hwrm_short_" PCI_PRI_FMT,
> +		snprintf(type, RTE_MEMZONE_NAMESIZE,
> +			"bnxt_hwrm_short_" PCI_PRI_FMT,
>  			bp->pdev->addr.domain, bp->pdev->addr.bus,
>  			bp->pdev->addr.devid, bp->pdev->addr.function);

Same concern as earlier: does this code handle truncation? Should the
function verify that snprintf did not truncate the name before
proceeding?

[ ... ]

> @@ -6573,7 +6575,7 @@ static int bnxt_alloc_all_ctx_pg_info(struct bnxt *bp)
>  		if (ctxm->instance_bmap)
>  			n = hweight32(ctxm->instance_bmap);
>  
> -		sprintf(name, "bnxt_ctx_pgmem_%d_%d",
> +		snprintf(name, RTE_MEMZONE_NAMESIZE, "bnxt_ctx_pgmem_%hu_%hu",
>  			bp->eth_dev->data->port_id, type);

What is the type of 'type' variable here? Does it match uint16_t? Also,
the snprintf conversion is good, but should truncation be checked?

[ ... ]

> @@ -7535,7 +7537,7 @@ int bnxt_hwrm_cfa_pair_exists(struct bnxt *bp, struct bnxt_representor *rep_bp)
>  	}
>  
>  	HWRM_PREP(&req, HWRM_CFA_PAIR_INFO, BNXT_USE_CHIMP_MB);
> -	snprintf(req.pair_name, sizeof(req.pair_name), "%svfr%d",
> +	snprintf(req.pair_name, sizeof(req.pair_name), "%svfr%hu",
>  		 bp->eth_dev->data->name, rep_bp->vf_id);

Does the type of rep_bp->vf_id match uint16_t? This should be verified
to ensure the format specifier is correct.

The same question applies to the similar changes in
bnxt_hwrm_cfa_pair_alloc and bnxt_hwrm_cfa_pair_free.