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

[Mohammad Shuab Siddique]

From: Keegan Freyhof <keegan.freyhof at broadcom.com>

sprintf() into fixed-size stack buffers such as
char type[RTE_MEMZONE_NAMESIZE] does not bound the write to the
buffer. bnxt_hwrm_ver_get()'s "bnxt_hwrm_short_" PCI_PRI_FMT label
overflows a 32-byte RTE_MEMZONE_NAMESIZE buffer once the PCI domain
needs more than 4 hex digits: rte_pci_addr.domain is a uint32_t, and
PCI_PRI_FMT's %.4x is a minimum width, not a maximum, so an 8-digit
domain plus the 16-character prefix plus the rest of the PCI address
string plus the terminating NUL is 33 bytes into 32.

Convert this and the other sprintf() calls building memzone/malloc
names and HWRM CFA pair_name request fields to snprintf(), which
bounds the write and truncates instead of overflowing. The return
value is not checked: none of these format strings involve locale or
multibyte conversion, so a negative return is not reachable, and a
truncated debug label or pair_name is not itself a memory-safety
concern -- a truncated pair_name simply fails to match at the
firmware side, handled the same way as any other not-found pair.

Also write the per-flow xstat names directly into
xstats_names[count].name via snprintf() instead of through an
intermediate buf plus strlcpy(), since the destination is already
exactly sized for that format string.

Fixes: 79cc1efd99c6 ("net/bnxt: support extended HWRM request sizes")
Cc: stable at dpdk.org

Signed-off-by: Keegan Freyhof <keegan.freyhof at broadcom.com>
Signed-off-by: Mohammad Shuab Siddique <mohammad-shuab.siddique at broadcom.com>
---
v3:
* Dropped check_snprintf_rc() and the early-return paths on the CFA
  pair_name sites entirely -- Stephen Hemminger noted the rc < 0
  branch is unreachable and plain snprintf() is enough for the PCI
  domain overflow this patch actually targets. All ten snprintf()
  sites now behave the same way: bounded, unchecked, truncate on
  overflow. This also removes the three bugs (a leak, a stale flag,
  and missing HWRM_UNLOCK()s) v2's early-return paths had introduced
  and fixed, since there is no early-return path left to have them.

 drivers/net/bnxt/bnxt.h        |  1 +
 drivers/net/bnxt/bnxt_ethdev.c | 14 +++++++-------
 drivers/net/bnxt/bnxt_hwrm.c   | 10 +++++-----
 drivers/net/bnxt/bnxt_stats.c  | 15 ++++++---------
 4 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/drivers/net/bnxt/bnxt.h b/drivers/net/bnxt/bnxt.h
index 336de75da0..f3edfb9530 100644
--- a/drivers/net/bnxt/bnxt.h
+++ b/drivers/net/bnxt/bnxt.h
@@ -1285,6 +1285,7 @@ extern int bnxt_logtype_driver;
 						       BNXT_LINK_SPEEDS_V2_VF((bp))))
 #define BNXT_MAX_SPEED_LANES 8
 #define BNXT_SUPPORTS_TPA(bp)  (!BNXT_CHIP_P5_P7(bp) || (bp)->max_tpa_v2)
+
 extern const struct rte_flow_ops bnxt_ulp_rte_flow_ops;
 int32_t bnxt_ulp_port_init(struct bnxt *bp);
 void bnxt_ulp_port_deinit(struct bnxt *bp);
diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index 4d4349457c..11e8f7908d 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -647,12 +647,12 @@ static int bnxt_init_fc_ctx_mem(struct bnxt *bp)
 {
 	struct rte_pci_device *pdev = bp->pdev;
 	char type[RTE_MEMZONE_NAMESIZE];
-	uint16_t max_fc;
 	int rc = 0;
+	uint16_t max_fc;
 
 	max_fc = bp->flow_stat->max_fc;
 
-	sprintf(type, "bnxt_rx_fc_in_" PCI_PRI_FMT, pdev->addr.domain,
+	snprintf(type, sizeof(type), "bnxt_rx_fc_in_" PCI_PRI_FMT, pdev->addr.domain,
 		pdev->addr.bus, pdev->addr.devid, pdev->addr.function);
 	/* 4 bytes for each counter-id */
 	rc = bnxt_alloc_ctx_mem_buf(bp, type,
@@ -661,7 +661,7 @@ static int bnxt_init_fc_ctx_mem(struct bnxt *bp)
 	if (rc)
 		return rc;
 
-	sprintf(type, "bnxt_rx_fc_out_" PCI_PRI_FMT, pdev->addr.domain,
+	snprintf(type, sizeof(type), "bnxt_rx_fc_out_" PCI_PRI_FMT, pdev->addr.domain,
 		pdev->addr.bus, pdev->addr.devid, pdev->addr.function);
 	/* 16 bytes for each counter - 8 bytes pkt_count, 8 bytes byte_count */
 	rc = bnxt_alloc_ctx_mem_buf(bp, type,
@@ -670,7 +670,7 @@ static int bnxt_init_fc_ctx_mem(struct bnxt *bp)
 	if (rc)
 		return rc;
 
-	sprintf(type, "bnxt_tx_fc_in_" PCI_PRI_FMT, pdev->addr.domain,
+	snprintf(type, sizeof(type), "bnxt_tx_fc_in_" PCI_PRI_FMT, pdev->addr.domain,
 		pdev->addr.bus, pdev->addr.devid, pdev->addr.function);
 	/* 4 bytes for each counter-id */
 	rc = bnxt_alloc_ctx_mem_buf(bp, type,
@@ -679,7 +679,7 @@ static int bnxt_init_fc_ctx_mem(struct bnxt *bp)
 	if (rc)
 		return rc;
 
-	sprintf(type, "bnxt_tx_fc_out_" PCI_PRI_FMT, pdev->addr.domain,
+	snprintf(type, sizeof(type), "bnxt_tx_fc_out_" PCI_PRI_FMT, pdev->addr.domain,
 		pdev->addr.bus, pdev->addr.devid, pdev->addr.function);
 	/* 16 bytes for each counter - 8 bytes pkt_count, 8 bytes byte_count */
 	rc = bnxt_alloc_ctx_mem_buf(bp, type,
@@ -5232,8 +5232,8 @@ int bnxt_alloc_ctx_pg_tbls(struct bnxt *bp)
 {
 	struct bnxt_ctx_mem_info *ctx = bp->ctx;
 	struct bnxt_ctx_mem *ctx2;
-	uint16_t type;
 	int rc = 0;
+	uint16_t type;
 
 	ctx2 = &ctx->ctx_arr[0];
 	for (type = 0; type < ctx->types && rc == 0; type++) {
@@ -5254,7 +5254,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, sizeof(name), "_%d_%d", i, type);
 
 			if (ctxm->entry_multiple)
 				entries = bnxt_roundup(ctxm->max_entries,
diff --git a/drivers/net/bnxt/bnxt_hwrm.c b/drivers/net/bnxt/bnxt_hwrm.c
index 1615b36aae..361402b36f 100644
--- a/drivers/net/bnxt/bnxt_hwrm.c
+++ b/drivers/net/bnxt/bnxt_hwrm.c
@@ -1583,12 +1583,12 @@ int bnxt_hwrm_func_resc_qcaps(struct bnxt *bp)
 
 int bnxt_hwrm_ver_get(struct bnxt *bp, uint32_t timeout)
 {
-	int rc = 0;
 	struct hwrm_ver_get_input req = {.req_type = 0 };
 	struct hwrm_ver_get_output *resp = bp->hwrm_cmd_resp_addr;
 	uint32_t fw_version;
 	uint16_t max_resp_len;
 	char type[RTE_MEMZONE_NAMESIZE];
+	int rc = 0;
 	uint32_t dev_caps_cfg;
 
 	bp->max_req_len = HWRM_MAX_REQ_LEN;
@@ -1669,10 +1669,9 @@ 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, sizeof(type), "bnxt_hwrm_short_" PCI_PRI_FMT,
 			bp->pdev->addr.domain, bp->pdev->addr.bus,
 			bp->pdev->addr.devid, bp->pdev->addr.function);
-
 		rte_free(bp->hwrm_short_cmd_req_addr);
 
 		bp->hwrm_short_cmd_req_addr =
@@ -3542,7 +3541,7 @@ int bnxt_alloc_hwrm_resources(struct bnxt *bp)
 	struct rte_pci_device *pdev = bp->pdev;
 	char type[RTE_MEMZONE_NAMESIZE];
 
-	sprintf(type, "bnxt_hwrm_" PCI_PRI_FMT, pdev->addr.domain,
+	snprintf(type, sizeof(type), "bnxt_hwrm_" PCI_PRI_FMT, pdev->addr.domain,
 		pdev->addr.bus, pdev->addr.devid, pdev->addr.function);
 	bp->max_resp_len = BNXT_PAGE_SIZE;
 	bp->hwrm_cmd_resp_addr = rte_malloc(type, bp->max_resp_len, 0);
@@ -6792,7 +6791,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, sizeof(name), "bnxt_ctx_pgmem_%d_%d",
 			bp->eth_dev->data->port_id, type);
 		ctxm->pg_info = rte_malloc(name, sizeof(*ctxm->pg_info) * n,
 					   RTE_CACHE_LINE_SIZE);
@@ -7836,6 +7835,7 @@ int bnxt_hwrm_cfa_pair_free(struct bnxt *bp, struct bnxt_representor *rep_bp)
 	HWRM_PREP(&req, HWRM_CFA_PAIR_FREE, BNXT_USE_CHIMP_MB);
 	snprintf(req.pair_name, sizeof(req.pair_name), "%svfr%d",
 		 bp->eth_dev->data->name, rep_bp->vf_id);
+
 	req.pf_b_id = rep_bp->parent_pf_idx;
 	req.pair_mode = HWRM_CFA_PAIR_FREE_INPUT_PAIR_MODE_REP2FN_TRUFLOW;
 	req.vf_id = BNXT_REP_PF(rep_bp) ? rte_cpu_to_le_16(((uint16_t)-1)) :
diff --git a/drivers/net/bnxt/bnxt_stats.c b/drivers/net/bnxt/bnxt_stats.c
index 4074613eb0..05e8944f47 100644
--- a/drivers/net/bnxt/bnxt_stats.c
+++ b/drivers/net/bnxt/bnxt_stats.c
@@ -1165,17 +1165,14 @@ int bnxt_dev_xstats_get_names_op(struct rte_eth_dev *eth_dev,
 	    bp->fw_cap & BNXT_FW_CAP_ADV_FLOW_MGMT &&
 	    BNXT_FLOW_XSTATS_EN(bp)) {
 		for (i = 0; i < bp->max_l2_ctx; i++) {
-			char buf[RTE_ETH_XSTATS_NAME_SIZE];
-
-			sprintf(buf, "flow_%d_bytes", i);
-			strlcpy(xstats_names[count].name, buf,
-				sizeof(xstats_names[count].name));
+			snprintf(xstats_names[count].name,
+				sizeof(xstats_names[count].name),
+				"flow_%d_bytes", i);
 			count++;
 
-			sprintf(buf, "flow_%d_packets", i);
-			strlcpy(xstats_names[count].name, buf,
-				sizeof(xstats_names[count].name));
-
+			snprintf(xstats_names[count].name,
+				sizeof(xstats_names[count].name),
+				"flow_%d_packets", i);
 			count++;
 		}
 	}
-- 
2.47.3