[PATCH wireless 0/2] wifi: brcmfmac: fwsignal: bound indices from firmware signals

[PATCH wireless 0/2] wifi: brcmfmac: fwsignal: bound indices from firmware signals

[Maoyi Xie]

These patches add bounds checks for indices that the device sends in the
BCDC firmware signals. Patch 1 covers the AMPDU reorder indices. Patch 2
covers the interface index of MACDESC_ADD. On USB, brcmfmac parses these
signals with the default module parameters.

Both bugs were found with a static analysis tool that we develop. A
userspace harness that compiles the fwsignal.c functions unchanged
reproduces them under sanitizers. No hardware was available for testing.

Maoyi Xie (2):
  wifi: brcmfmac: fwsignal: bound the AMPDU reorder indices
  wifi: brcmfmac: fwsignal: bound the MACDESC_ADD interface index

 .../broadcom/brcm80211/brcmfmac/core.h        |  2 ++
 .../broadcom/brcm80211/brcmfmac/fwsignal.c    | 26 ++++++++++++++++---
 2 files changed, 24 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH wireless 1/2] wifi: brcmfmac: fwsignal: bound the AMPDU reorder indices

[Maoyi Xie]

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

[PATCH wireless 2/2] wifi: brcmfmac: fwsignal: bound the MACDESC_ADD interface index

[Maoyi Xie]

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