[PATCH wireless] wifi: brcmfmac: validate msgbuf flowring IDs before use

[PATCH wireless] 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>
---
 .../wireless/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 ba1ce1552e0f..8146d50d6f07 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
@@ -560,6 +560,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)
@@ -880,17 +902,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);
@@ -1237,13 +1265,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);
@@ -1266,13 +1297,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 wireless] wifi: brcmfmac: validate msgbuf flowring IDs before use

[Arend van Spriel]

On Thu, 23 Jul 2026 13:56:17 +0800, Can Peng <pengcan@kylinos.cn> wrote:
> 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>
> ---
>  .../wireless/broadcom/brcm80211/brcmfmac/msgbuf.c  | 46 +++++++++++++++++++---
>  1 file changed, 40 insertions(+), 6 deletions(-)

One nit: there is a double blank line between brcmf_msgbuf_get_flowid()
and brcmf_msgbuf_dequeue_work() in the resulting code. This comes from
the pre-existing double blank line that was between
brcmf_msgbuf_remove_flowring() and brcmf_msgbuf_dequeue_work() -- worth
cleaning up to a single blank line.

Minor enough that I will take care of it while applying if you do not
send a v2.

To be applied to wireless tree.

Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>

Regards,
Arend

Re: [PATCH wireless] wifi: brcmfmac: validate msgbuf flowring IDs before use

[Arend van Spriel]

On 02/08/2026 10:39, Arend van Spriel wrote:
> On Thu, 23 Jul 2026 13:56:17 +0800, Can Peng <pengcan@kylinos.cn> wrote:
>> 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>
>> ---
>>   .../wireless/broadcom/brcm80211/brcmfmac/msgbuf.c  | 46 +++++++++++++++++++---
>>   1 file changed, 40 insertions(+), 6 deletions(-)
> 
> One nit: there is a double blank line between brcmf_msgbuf_get_flowid()
> and brcmf_msgbuf_dequeue_work() in the resulting code. This comes from
> the pre-existing double blank line that was between
> brcmf_msgbuf_remove_flowring() and brcmf_msgbuf_dequeue_work() -- worth
> cleaning up to a single blank line.
> 
> Minor enough that I will take care of it while applying if you do not
> send a v2.

As I do not apply anything anywhere maybe Johannes will, but feel free 
to send that v2.

Regards,
Arend