[PATCH v2 0/2] wifi: brcmfmac: validate msgbuf flowring IDs

[PATCH v2 0/2] wifi: brcmfmac: validate msgbuf flowring IDs

[Can Peng]

Clean up the excess blank lines in msgbuf.c first, then validate
firmware supplied msgbuf flowring IDs before converting them to internal
flow IDs and using them as bit and array indexes.

Changes in v2:
- Add a preparatory cleanup to use a single blank line between functions
  and declarations in msgbuf.c.
- Rebase the flowring ID validation change on top of the cleanup.
- Link to v1:
  https://lore.kernel.org/all/20260723055618.550834-1-pengcan@kylinos.cn/

Can Peng (2):
  wifi: brcmfmac: remove extra blank lines in msgbuf.c
  wifi: brcmfmac: validate msgbuf flowring IDs before use

 .../broadcom/brcm80211/brcmfmac/msgbuf.c      | 84 +++++++++----------
 1 file changed, 39 insertions(+), 45 deletions(-)

-- 
2.53.0

[PATCH v2 1/2] wifi: brcmfmac: remove extra blank lines in msgbuf.c

[Can Peng]

msgbuf.c uses two blank lines in many places, including between
top-level declarations and functions.

Keep a single blank line instead to match common kernel coding style.

Signed-off-by: Can Peng <pengcan@kylinos.cn>
---
 .../broadcom/brcm80211/brcmfmac/msgbuf.c      | 40 -------------------
 1 file changed, 40 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
index ba1ce1552e0f..9c41e4a35815 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
@@ -24,7 +24,6 @@
 #include "bus.h"
 #include "tracepoint.h"
 
-
 #define MSGBUF_IOCTL_RESP_TIMEOUT		msecs_to_jiffies(2000)
 
 #define MSGBUF_TYPE_GEN_STATUS			0x1
@@ -291,7 +290,6 @@ struct brcmf_msgbuf_pktids {
 
 static void brcmf_msgbuf_rxbuf_ioctlresp_post(struct brcmf_msgbuf *msgbuf);
 
-
 static struct brcmf_msgbuf_pktids *
 brcmf_msgbuf_init_pktids(u32 nr_array_entries,
 			 enum dma_data_direction direction)
@@ -314,7 +312,6 @@ brcmf_msgbuf_init_pktids(u32 nr_array_entries,
 	return pktids;
 }
 
-
 static int
 brcmf_msgbuf_alloc_pktid(struct device *dev,
 			 struct brcmf_msgbuf_pktids *pktids,
@@ -362,7 +359,6 @@ brcmf_msgbuf_alloc_pktid(struct device *dev,
 	return 0;
 }
 
-
 static struct sk_buff *
 brcmf_msgbuf_get_pktid(struct device *dev, struct brcmf_msgbuf_pktids *pktids,
 		       u32 idx)
@@ -390,7 +386,6 @@ brcmf_msgbuf_get_pktid(struct device *dev, struct brcmf_msgbuf_pktids *pktids,
 	return NULL;
 }
 
-
 static void
 brcmf_msgbuf_release_array(struct device *dev,
 			   struct brcmf_msgbuf_pktids *pktids)
@@ -416,7 +411,6 @@ brcmf_msgbuf_release_array(struct device *dev,
 	kfree(pktids);
 }
 
-
 static void brcmf_msgbuf_release_pktids(struct brcmf_msgbuf *msgbuf)
 {
 	if (msgbuf->rx_pktids)
@@ -427,7 +421,6 @@ static void brcmf_msgbuf_release_pktids(struct brcmf_msgbuf *msgbuf)
 					   msgbuf->tx_pktids);
 }
 
-
 static int brcmf_msgbuf_tx_ioctl(struct brcmf_pub *drvr, int ifidx,
 				 uint cmd, void *buf, uint len)
 {
@@ -473,7 +466,6 @@ static int brcmf_msgbuf_tx_ioctl(struct brcmf_pub *drvr, int ifidx,
 	return err;
 }
 
-
 static int brcmf_msgbuf_ioctl_resp_wait(struct brcmf_msgbuf *msgbuf)
 {
 	return wait_event_timeout(msgbuf->ioctl_resp_wait,
@@ -481,14 +473,12 @@ static int brcmf_msgbuf_ioctl_resp_wait(struct brcmf_msgbuf *msgbuf)
 				  MSGBUF_IOCTL_RESP_TIMEOUT);
 }
 
-
 static void brcmf_msgbuf_ioctl_resp_wake(struct brcmf_msgbuf *msgbuf)
 {
 	msgbuf->ctl_completed = true;
 	wake_up(&msgbuf->ioctl_resp_wait);
 }
 
-
 static int brcmf_msgbuf_query_dcmd(struct brcmf_pub *drvr, int ifidx,
 				   uint cmd, void *buf, uint len, int *fwerr)
 {
@@ -526,14 +516,12 @@ static int brcmf_msgbuf_query_dcmd(struct brcmf_pub *drvr, int ifidx,
 	return 0;
 }
 
-
 static int brcmf_msgbuf_set_dcmd(struct brcmf_pub *drvr, int ifidx,
 				 uint cmd, void *buf, uint len, int *fwerr)
 {
 	return brcmf_msgbuf_query_dcmd(drvr, ifidx, cmd, buf, len, fwerr);
 }
 
-
 static int brcmf_msgbuf_hdrpull(struct brcmf_pub *drvr, bool do_fws,
 				struct sk_buff *skb, struct brcmf_if **ifp)
 {
@@ -560,7 +548,6 @@ brcmf_msgbuf_remove_flowring(struct brcmf_msgbuf *msgbuf, u16 flowid)
 	brcmf_flowring_delete(msgbuf->flow, flowid);
 }
 
-
 static struct brcmf_msgbuf_work_item *
 brcmf_msgbuf_dequeue_work(struct brcmf_msgbuf *msgbuf)
 {
@@ -578,7 +565,6 @@ brcmf_msgbuf_dequeue_work(struct brcmf_msgbuf *msgbuf)
 	return work;
 }
 
-
 static u32
 brcmf_msgbuf_flowring_create_worker(struct brcmf_msgbuf *msgbuf,
 				    struct brcmf_msgbuf_work_item *work)
@@ -647,7 +633,6 @@ brcmf_msgbuf_flowring_create_worker(struct brcmf_msgbuf *msgbuf,
 	return flowid;
 }
 
-
 static void brcmf_msgbuf_flowring_worker(struct work_struct *work)
 {
 	struct brcmf_msgbuf *msgbuf;
@@ -661,7 +646,6 @@ static void brcmf_msgbuf_flowring_worker(struct work_struct *work)
 	}
 }
 
-
 static u32 brcmf_msgbuf_flowring_create(struct brcmf_msgbuf *msgbuf, int ifidx,
 					struct sk_buff *skb)
 {
@@ -694,7 +678,6 @@ static u32 brcmf_msgbuf_flowring_create(struct brcmf_msgbuf *msgbuf, int ifidx,
 	return flowid;
 }
 
-
 static void brcmf_msgbuf_txflow(struct brcmf_msgbuf *msgbuf, u16 flowid)
 {
 	struct brcmf_flowring *flow = msgbuf->flow;
@@ -768,7 +751,6 @@ static void brcmf_msgbuf_txflow(struct brcmf_msgbuf *msgbuf, u16 flowid)
 	brcmf_commonring_unlock(commonring);
 }
 
-
 static void brcmf_msgbuf_txflow_worker(struct work_struct *worker)
 {
 	struct brcmf_msgbuf *msgbuf;
@@ -781,7 +763,6 @@ static void brcmf_msgbuf_txflow_worker(struct work_struct *worker)
 	}
 }
 
-
 static int brcmf_msgbuf_schedule_txdata(struct brcmf_msgbuf *msgbuf, u32 flowid,
 					bool force)
 {
@@ -796,7 +777,6 @@ static int brcmf_msgbuf_schedule_txdata(struct brcmf_msgbuf *msgbuf, u32 flowid,
 	return 0;
 }
 
-
 static int brcmf_msgbuf_tx_queue_data(struct brcmf_pub *drvr, int ifidx,
 				      struct sk_buff *skb)
 {
@@ -824,7 +804,6 @@ static int brcmf_msgbuf_tx_queue_data(struct brcmf_pub *drvr, int ifidx,
 	return 0;
 }
 
-
 static void
 brcmf_msgbuf_configure_addr_mode(struct brcmf_pub *drvr, int ifidx,
 				 enum proto_addr_mode addr_mode)
@@ -834,7 +813,6 @@ brcmf_msgbuf_configure_addr_mode(struct brcmf_pub *drvr, int ifidx,
 	brcmf_flowring_configure_addr_mode(msgbuf->flow, ifidx, addr_mode);
 }
 
-
 static void
 brcmf_msgbuf_delete_peer(struct brcmf_pub *drvr, int ifidx, u8 peer[ETH_ALEN])
 {
@@ -843,7 +821,6 @@ brcmf_msgbuf_delete_peer(struct brcmf_pub *drvr, int ifidx, u8 peer[ETH_ALEN])
 	brcmf_flowring_delete_peer(msgbuf->flow, ifidx, peer);
 }
 
-
 static void
 brcmf_msgbuf_add_tdls_peer(struct brcmf_pub *drvr, int ifidx, u8 peer[ETH_ALEN])
 {
@@ -852,7 +829,6 @@ brcmf_msgbuf_add_tdls_peer(struct brcmf_pub *drvr, int ifidx, u8 peer[ETH_ALEN])
 	brcmf_flowring_add_tdls_peer(msgbuf->flow, ifidx, peer);
 }
 
-
 static void
 brcmf_msgbuf_process_ioctl_complete(struct brcmf_msgbuf *msgbuf, void *buf)
 {
@@ -872,7 +848,6 @@ brcmf_msgbuf_process_ioctl_complete(struct brcmf_msgbuf *msgbuf, void *buf)
 	brcmf_msgbuf_rxbuf_ioctlresp_post(msgbuf);
 }
 
-
 static void
 brcmf_msgbuf_process_txstatus(struct brcmf_msgbuf *msgbuf, void *buf)
 {
@@ -899,7 +874,6 @@ brcmf_msgbuf_process_txstatus(struct brcmf_msgbuf *msgbuf, void *buf)
 			 skb, true);
 }
 
-
 static u32 brcmf_msgbuf_rxbuf_data_post(struct brcmf_msgbuf *msgbuf, u32 count)
 {
 	struct brcmf_pub *drvr = msgbuf->drvr;
@@ -977,7 +951,6 @@ static u32 brcmf_msgbuf_rxbuf_data_post(struct brcmf_msgbuf *msgbuf, u32 count)
 	return i;
 }
 
-
 static void
 brcmf_msgbuf_rxbuf_data_fill(struct brcmf_msgbuf *msgbuf)
 {
@@ -995,7 +968,6 @@ brcmf_msgbuf_rxbuf_data_fill(struct brcmf_msgbuf *msgbuf)
 	}
 }
 
-
 static void
 brcmf_msgbuf_update_rxbufpost_count(struct brcmf_msgbuf *msgbuf, u16 rxcnt)
 {
@@ -1005,7 +977,6 @@ brcmf_msgbuf_update_rxbufpost_count(struct brcmf_msgbuf *msgbuf, u16 rxcnt)
 		brcmf_msgbuf_rxbuf_data_fill(msgbuf);
 }
 
-
 static u32
 brcmf_msgbuf_rxbuf_ctrl_post(struct brcmf_msgbuf *msgbuf, bool event_buf,
 			     u32 count)
@@ -1079,7 +1050,6 @@ brcmf_msgbuf_rxbuf_ctrl_post(struct brcmf_msgbuf *msgbuf, bool event_buf,
 	return i;
 }
 
-
 static void brcmf_msgbuf_rxbuf_ioctlresp_post(struct brcmf_msgbuf *msgbuf)
 {
 	u32 count;
@@ -1089,7 +1059,6 @@ static void brcmf_msgbuf_rxbuf_ioctlresp_post(struct brcmf_msgbuf *msgbuf)
 	msgbuf->cur_ioctlrespbuf += count;
 }
 
-
 static void brcmf_msgbuf_rxbuf_event_post(struct brcmf_msgbuf *msgbuf)
 {
 	u32 count;
@@ -1099,7 +1068,6 @@ static void brcmf_msgbuf_rxbuf_event_post(struct brcmf_msgbuf *msgbuf)
 	msgbuf->cur_eventbuf += count;
 }
 
-
 static void brcmf_msgbuf_process_event(struct brcmf_msgbuf *msgbuf, void *buf)
 {
 	struct brcmf_pub *drvr = msgbuf->drvr;
@@ -1142,7 +1110,6 @@ static void brcmf_msgbuf_process_event(struct brcmf_msgbuf *msgbuf, void *buf)
 	brcmu_pkt_buf_free_skb(skb);
 }
 
-
 static void
 brcmf_msgbuf_process_rx_complete(struct brcmf_msgbuf *msgbuf, void *buf)
 {
@@ -1257,7 +1224,6 @@ brcmf_msgbuf_process_flow_ring_create_response(struct brcmf_msgbuf *msgbuf,
 	brcmf_msgbuf_schedule_txdata(msgbuf, flowid, true);
 }
 
-
 static void
 brcmf_msgbuf_process_flow_ring_delete_response(struct brcmf_msgbuf *msgbuf,
 					       void *buf)
@@ -1284,7 +1250,6 @@ brcmf_msgbuf_process_flow_ring_delete_response(struct brcmf_msgbuf *msgbuf,
 	brcmf_msgbuf_remove_flowring(msgbuf, flowid);
 }
 
-
 static void brcmf_msgbuf_process_msgtype(struct brcmf_msgbuf *msgbuf, void *buf)
 {
 	struct brcmf_pub *drvr = msgbuf->drvr;
@@ -1333,7 +1298,6 @@ static void brcmf_msgbuf_process_msgtype(struct brcmf_msgbuf *msgbuf, void *buf)
 	}
 }
 
-
 static void brcmf_msgbuf_process_rx(struct brcmf_msgbuf *msgbuf,
 				    struct brcmf_commonring *commonring)
 {
@@ -1365,7 +1329,6 @@ static void brcmf_msgbuf_process_rx(struct brcmf_msgbuf *msgbuf,
 		goto again;
 }
 
-
 int brcmf_proto_msgbuf_rx_trigger(struct device *dev)
 {
 	struct brcmf_bus *bus_if = dev_get_drvdata(dev);
@@ -1397,7 +1360,6 @@ int brcmf_proto_msgbuf_rx_trigger(struct device *dev)
 	return 0;
 }
 
-
 void brcmf_msgbuf_delete_flowring(struct brcmf_pub *drvr, u16 flowid)
 {
 	struct brcmf_msgbuf *msgbuf = (struct brcmf_msgbuf *)drvr->proto->pd;
@@ -1613,7 +1575,6 @@ int brcmf_proto_msgbuf_attach(struct brcmf_pub *drvr)
 	if (!msgbuf->flow)
 		goto fail;
 
-
 	brcmf_dbg(MSGBUF, "Feeding buffers, rx data %d, rx event %d, rx ioctl resp %d\n",
 		  msgbuf->max_rxbufpost, msgbuf->max_eventbuf,
 		  msgbuf->max_ioctlrespbuf);
@@ -1653,7 +1614,6 @@ int brcmf_proto_msgbuf_attach(struct brcmf_pub *drvr)
 	return -ENOMEM;
 }
 
-
 void brcmf_proto_msgbuf_detach(struct brcmf_pub *drvr)
 {
 	struct brcmf_msgbuf *msgbuf;
-- 
2.53.0

[PATCH v2 2/2] wifi: brcmfmac: validate msgbuf flowring IDs before use

[Can Peng]

Firmware messages carry flow_ring_id values which brcmfmac converts
to an internal flowid by subtracting
BRCMF_H2D_MSGRING_FLOWRING_IDSTART. The resulting value is used as
a bit index in txstatus_done_map and as an array index into
msgbuf->flowrings and the flowring state.

Validate the firmware supplied flow_ring_id before using it. This
prevents flow_ring_id values below BRCMF_H2D_MSGRING_FLOWRING_IDSTART
from underflowing and rejects values outside msgbuf->max_flowrings.

In the tx status path, complete the packet with an error after
removing a valid packet id so the skb is not leaked when the flow
ring id is invalid.

Fixes: 9a1bb60250d2 ("brcmfmac: Adding msgbuf protocol.")
Cc: stable@vger.kernel.org
Signed-off-by: Can Peng <pengcan@kylinos.cn>
---
 .../broadcom/brcm80211/brcmfmac/msgbuf.c      | 46 ++++++++++++++++---
 1 file changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
index 9c41e4a35815..959659bfea99 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
@@ -548,6 +548,28 @@ brcmf_msgbuf_remove_flowring(struct brcmf_msgbuf *msgbuf, u16 flowid)
 	brcmf_flowring_delete(msgbuf->flow, flowid);
 }
 
+static bool brcmf_msgbuf_get_flowid(struct brcmf_msgbuf *msgbuf,
+				    u16 flow_ring_id, u16 *flowid)
+{
+	u32 id = flow_ring_id;
+
+	if (id < BRCMF_H2D_MSGRING_FLOWRING_IDSTART) {
+		bphy_err(msgbuf->drvr, "invalid flowring id %u\n",
+			 flow_ring_id);
+		return false;
+	}
+
+	id -= BRCMF_H2D_MSGRING_FLOWRING_IDSTART;
+	if (id >= msgbuf->max_flowrings) {
+		bphy_err(msgbuf->drvr, "invalid flowring id %u\n",
+			 flow_ring_id);
+		return false;
+	}
+
+	*flowid = id;
+	return true;
+}
+
 static struct brcmf_msgbuf_work_item *
 brcmf_msgbuf_dequeue_work(struct brcmf_msgbuf *msgbuf)
 {
@@ -855,17 +877,23 @@ brcmf_msgbuf_process_txstatus(struct brcmf_msgbuf *msgbuf, void *buf)
 	struct msgbuf_tx_status *tx_status;
 	u32 idx;
 	struct sk_buff *skb;
+	u16 flow_ring_id;
 	u16 flowid;
 
 	tx_status = (struct msgbuf_tx_status *)buf;
 	idx = le32_to_cpu(tx_status->msg.request_id) - 1;
-	flowid = le16_to_cpu(tx_status->compl_hdr.flow_ring_id);
-	flowid -= BRCMF_H2D_MSGRING_FLOWRING_IDSTART;
+	flow_ring_id = le16_to_cpu(tx_status->compl_hdr.flow_ring_id);
 	skb = brcmf_msgbuf_get_pktid(msgbuf->drvr->bus_if->dev,
 				     msgbuf->tx_pktids, idx);
 	if (!skb)
 		return;
 
+	if (!brcmf_msgbuf_get_flowid(msgbuf, flow_ring_id, &flowid)) {
+		brcmf_txfinalize(brcmf_get_ifp(msgbuf->drvr, tx_status->msg.ifidx),
+				 skb, false);
+		return;
+	}
+
 	set_bit(flowid, msgbuf->txstatus_done_map);
 	commonring = msgbuf->flowrings[flowid];
 	atomic_dec(&commonring->outstanding_tx);
@@ -1204,13 +1232,16 @@ brcmf_msgbuf_process_flow_ring_create_response(struct brcmf_msgbuf *msgbuf,
 	struct msgbuf_flowring_create_resp *flowring_create_resp;
 	u16 status;
 	u16 flowid;
+	u16 flow_ring_id;
 
 	flowring_create_resp = (struct msgbuf_flowring_create_resp *)buf;
 
-	flowid = le16_to_cpu(flowring_create_resp->compl_hdr.flow_ring_id);
-	flowid -= BRCMF_H2D_MSGRING_FLOWRING_IDSTART;
+	flow_ring_id = le16_to_cpu(flowring_create_resp->compl_hdr.flow_ring_id);
 	status =  le16_to_cpu(flowring_create_resp->compl_hdr.status);
 
+	if (!brcmf_msgbuf_get_flowid(msgbuf, flow_ring_id, &flowid))
+		return;
+
 	if (status) {
 		bphy_err(drvr, "Flowring creation failed, code %d\n", status);
 		brcmf_msgbuf_remove_flowring(msgbuf, flowid);
@@ -1232,13 +1263,16 @@ brcmf_msgbuf_process_flow_ring_delete_response(struct brcmf_msgbuf *msgbuf,
 	struct msgbuf_flowring_delete_resp *flowring_delete_resp;
 	u16 status;
 	u16 flowid;
+	u16 flow_ring_id;
 
 	flowring_delete_resp = (struct msgbuf_flowring_delete_resp *)buf;
 
-	flowid = le16_to_cpu(flowring_delete_resp->compl_hdr.flow_ring_id);
-	flowid -= BRCMF_H2D_MSGRING_FLOWRING_IDSTART;
+	flow_ring_id = le16_to_cpu(flowring_delete_resp->compl_hdr.flow_ring_id);
 	status =  le16_to_cpu(flowring_delete_resp->compl_hdr.status);
 
+	if (!brcmf_msgbuf_get_flowid(msgbuf, flow_ring_id, &flowid))
+		return;
+
 	if (status) {
 		bphy_err(drvr, "Flowring deletion failed, code %d\n", status);
 		brcmf_flowring_delete(msgbuf->flow, flowid);
-- 
2.53.0

Re: [PATCH v2 0/2] wifi: brcmfmac: validate msgbuf flowring IDs

[Johannes Berg]

On Mon, 2026-08-03 at 15:12 +0800, Can Peng wrote:
> Clean up the excess blank lines in msgbuf.c first, then validate
> firmware supplied msgbuf flowring IDs before converting them to internal
> flow IDs and using them as bit and array indexes.
> 
> Changes in v2:
> - Add a preparatory cleanup to use a single blank line between functions
>   and declarations in msgbuf.c.
> - Rebase the flowring ID validation change on top of the cleanup.

Too late, I applied the other changes yesterday, and while I can rebase,
I'm not going to do that now for just some whitespace. Feel free to send
a cleanup on top of current wireless-next.

johannes