* [PATCH wireless 1/2] wifi: brcmfmac: fwsignal: bound the AMPDU reorder indices
2026-11-15 18:58 [PATCH wireless 0/2] wifi: brcmfmac: fwsignal: bound indices from firmware signals Maoyi Xie
@ 2026-11-15 18:58 ` Maoyi Xie
2026-11-15 18:58 ` [PATCH wireless 2/2] wifi: brcmfmac: fwsignal: bound the MACDESC_ADD interface index Maoyi Xie
1 sibling, 0 replies; 3+ messages in thread
From: Maoyi Xie @ 2026-11-15 18:58 UTC (permalink / raw)
To: Arend van Spriel
Cc: linux-wireless, brcm80211, brcm80211-dev-list.pdl, linux-kernel,
stable
brcmf_fws_rxreorder() allocates max_idx + 1 slots when a reorder flow
starts. It then uses the indices of each frame without a check.
rfi->pktslots[rfi->cur_idx] = pkt;
A cur_idx above the allocated maximum makes the driver write and read
past pktslots[]. An exp_idx above max_idx makes
brcmf_rxreorder_get_skb_list() read past the array or loop forever.
Store the allocated maximum in the flow. Bound max_idx by it and the
other indices by max_idx. Deliver a frame that fails the check without
reordering.
This was found with a static analysis tool that we develop. A userspace
harness that compiles the fwsignal.c functions unchanged runs under
AddressSanitizer. It shows the out-of-bounds accesses and the endless
loop before this patch, but not after it. No hardware was available for
testing.
Fixes: 87edd8916ee2 ("brcmfmac: add AMPDU reordering functionality")
Cc: stable@vger.kernel.org
Signed-off-by: Maoyi Xie <maoyixie.tju@gmail.com>
---
.../broadcom/brcm80211/brcmfmac/core.h | 2 ++
.../broadcom/brcm80211/brcmfmac/fwsignal.c | 24 ++++++++++++++++---
2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
index 399b6810e394de..19cecb987a87bc 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
@@ -52,6 +52,7 @@
* @cur_idx: last AMPDU index from firmware.
* @exp_idx: expected next AMPDU index.
* @max_idx: maximum amount of packets per AMPDU.
+ * @alloc_max_idx: highest index of @pktslots, set when the flow is created.
* @pend_pkts: number of packets currently in @pktslots.
* @pktslots: array for ordering AMPDU packets.
*/
@@ -60,6 +61,7 @@ struct brcmf_ampdu_rx_reorder {
u8 cur_idx;
u8 exp_idx;
u8 max_idx;
+ u8 alloc_max_idx;
u8 pend_pkts;
struct sk_buff *pktslots[];
};
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
index a43f1a38b0e30e..e81105154d6543 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
@@ -1721,8 +1721,16 @@ void brcmf_fws_rxreorder(struct brcmf_if *ifp, struct sk_buff *pkt)
ifp->drvr->reorder_flows[flow_id] = rfi;
rfi->max_idx = max_idx;
+ rfi->alloc_max_idx = max_idx;
}
if (flags & BRCMF_RXREORDER_NEW_HOLE) {
+ cur_idx = reorder_data[BRCMF_RXREORDER_CURIDX_OFFSET];
+ exp_idx = reorder_data[BRCMF_RXREORDER_EXPIDX_OFFSET];
+ max_idx = reorder_data[BRCMF_RXREORDER_MAXIDX_OFFSET];
+ if (max_idx > rfi->alloc_max_idx || cur_idx > max_idx ||
+ exp_idx > max_idx)
+ goto bad_idx;
+
if (rfi->pend_pkts) {
brcmf_rxreorder_get_skb_list(rfi, rfi->exp_idx,
rfi->exp_idx,
@@ -1731,9 +1739,9 @@ void brcmf_fws_rxreorder(struct brcmf_if *ifp, struct sk_buff *pkt)
} else {
__skb_queue_head_init(&reorder_list);
}
- rfi->cur_idx = reorder_data[BRCMF_RXREORDER_CURIDX_OFFSET];
- rfi->exp_idx = reorder_data[BRCMF_RXREORDER_EXPIDX_OFFSET];
- rfi->max_idx = reorder_data[BRCMF_RXREORDER_MAXIDX_OFFSET];
+ rfi->cur_idx = cur_idx;
+ rfi->exp_idx = exp_idx;
+ rfi->max_idx = max_idx;
rfi->pktslots[rfi->cur_idx] = pkt;
rfi->pend_pkts++;
brcmf_dbg(DATA, "flow-%d: new hole %d (%d), pending %d\n",
@@ -1741,6 +1749,8 @@ void brcmf_fws_rxreorder(struct brcmf_if *ifp, struct sk_buff *pkt)
} else if (flags & BRCMF_RXREORDER_CURIDX_VALID) {
cur_idx = reorder_data[BRCMF_RXREORDER_CURIDX_OFFSET];
exp_idx = reorder_data[BRCMF_RXREORDER_EXPIDX_OFFSET];
+ if (cur_idx > rfi->max_idx || exp_idx > rfi->max_idx)
+ goto bad_idx;
if ((exp_idx == rfi->exp_idx) && (cur_idx != rfi->exp_idx)) {
/* still in the current hole */
@@ -1811,6 +1821,8 @@ void brcmf_fws_rxreorder(struct brcmf_if *ifp, struct sk_buff *pkt)
} else {
/* explicitly window move updating the expected index */
exp_idx = reorder_data[BRCMF_RXREORDER_EXPIDX_OFFSET];
+ if (exp_idx > rfi->max_idx)
+ goto bad_idx;
brcmf_dbg(DATA, "flow-%d (0x%x): change expected: %d -> %d\n",
flow_id, flags, rfi->exp_idx, exp_idx);
@@ -1830,6 +1842,12 @@ void brcmf_fws_rxreorder(struct brcmf_if *ifp, struct sk_buff *pkt)
__skb_unlink(pkt, &reorder_list);
brcmf_netif_rx(ifp, pkt);
}
+ return;
+
+bad_idx:
+ bphy_err(drvr, "flow-%d: invalid reorder index, ignore this packet\n",
+ flow_id);
+ brcmf_netif_rx(ifp, pkt);
}
void brcmf_fws_hdrpull(struct brcmf_if *ifp, s16 siglen, struct sk_buff *skb)
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH wireless 2/2] wifi: brcmfmac: fwsignal: bound the MACDESC_ADD interface index
2026-11-15 18:58 [PATCH wireless 0/2] wifi: brcmfmac: fwsignal: bound indices from firmware signals Maoyi Xie
2026-11-15 18:58 ` [PATCH wireless 1/2] wifi: brcmfmac: fwsignal: bound the AMPDU reorder indices Maoyi Xie
@ 2026-11-15 18:58 ` Maoyi Xie
1 sibling, 0 replies; 3+ messages in thread
From: Maoyi Xie @ 2026-11-15 18:58 UTC (permalink / raw)
To: Arend van Spriel
Cc: linux-wireless, brcm80211, brcm80211-dev-list.pdl, linux-kernel,
stable
brcmf_fws_macdesc_indicate() stores the interface index of a
MACDESC_ADD signal without a check. brcmf_fws_macdesc_closed() later
uses it as an index.
if_entry = &fws->desc.iface[entry->interface_id];
A device that reports an index of BRCMF_MAX_IFS or more makes this read
past iface[]. On USB this parser runs with the default module
parameters. Reject such a signal as
brcmf_fws_interface_state_indicate() does, and count it as a failed
update.
This was found with a static analysis tool that we develop. A userspace
harness that compiles the fwsignal.c functions unchanged shows the
out-of-bounds index under UBSAN before this patch, but not after it. No
hardware was available for testing.
Fixes: b75c1a301b7d ("brcmfmac: destination mac closed when interface is closed")
Cc: stable@vger.kernel.org
Signed-off-by: Maoyi Xie <maoyixie.tju@gmail.com>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
index e81105154d6543..56eb2c2943dbe8 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
@@ -1010,7 +1010,7 @@ int brcmf_fws_macdesc_indicate(struct brcmf_fws_info *fws, u8 type, u8 *data)
existing = brcmf_fws_macdesc_lookup(fws, addr);
if (IS_ERR(existing)) {
- if (!entry->occupied) {
+ if (!entry->occupied && ifidx < BRCMF_MAX_IFS) {
brcmf_fws_lock(fws);
entry->mac_handle = mac_handle;
brcmf_fws_macdesc_init(entry, addr, ifidx);
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread