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 --- .../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