| Message ID | 20230809215817.3080280-1-pinkperfect2021@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v8] wifi: mwifiex: Fix OOB and integer underflow when rx packets | expand |
On Wed, 2023-08-09 at 21:58 +0000, Polaris Pi wrote: > Make sure mwifiex_process_mgmt_packet, > mwifiex_process_sta_rx_packet and mwifiex_process_uap_rx_packet, > mwifiex_uap_queue_bridged_pkt and mwifiex_process_rx_packet > not out-of-bounds access the skb->data buffer. > > Fixes: 2dbaf751b1de ("mwifiex: report received management frames to cfg80211") > Signed-off-by: Polaris Pi <pinkperfect2021@gmail.com> > --- > V5: Follow chromeos comments: preserve the original flow of mwifiex_process_uap_rx_packet > V6: Simplify check in mwifiex_process_uap_rx_packet > V7: Fix drop packets issue when auotest V6, now pass manual and auto tests > V8: Fix missing return after free skb > Arguably, as Brian also said, that missing return is completely unrelated and should perhaps be a separate commit? johannes
On Thu, Aug 10, 2023 at 09:32:57AM +0200, Johannes Berg wrote: > On Wed, 2023-08-09 at 21:58 +0000, Polaris Pi wrote: > > Make sure mwifiex_process_mgmt_packet, > > mwifiex_process_sta_rx_packet and mwifiex_process_uap_rx_packet, > > mwifiex_uap_queue_bridged_pkt and mwifiex_process_rx_packet > > not out-of-bounds access the skb->data buffer. > > > > Fixes: 2dbaf751b1de ("mwifiex: report received management frames to cfg80211") > > Signed-off-by: Polaris Pi <pinkperfect2021@gmail.com> > > --- > > V5: Follow chromeos comments: preserve the original flow of mwifiex_process_uap_rx_packet > > V6: Simplify check in mwifiex_process_uap_rx_packet > > V7: Fix drop packets issue when auotest V6, now pass manual and auto tests > > V8: Fix missing return after free skb > > > > Arguably, as Brian also said, that missing return is completely > unrelated and should perhaps be a separate commit? I think the key here is that Polaris's buggy patch has already been applied, so this v8 doesn't really work. He has since submitted a proper fixup patch that applies appropriately, and I've Ack'd that. Brian
On Thu, 2023-08-10 at 16:54 -0700, Brian Norris wrote: > On Thu, Aug 10, 2023 at 09:32:57AM +0200, Johannes Berg wrote: > > On Wed, 2023-08-09 at 21:58 +0000, Polaris Pi wrote: > > > Make sure mwifiex_process_mgmt_packet, > > > mwifiex_process_sta_rx_packet and mwifiex_process_uap_rx_packet, > > > mwifiex_uap_queue_bridged_pkt and mwifiex_process_rx_packet > > > not out-of-bounds access the skb->data buffer. > > > > > > Fixes: 2dbaf751b1de ("mwifiex: report received management frames to cfg80211") > > > Signed-off-by: Polaris Pi <pinkperfect2021@gmail.com> > > > --- > > > V5: Follow chromeos comments: preserve the original flow of mwifiex_process_uap_rx_packet > > > V6: Simplify check in mwifiex_process_uap_rx_packet > > > V7: Fix drop packets issue when auotest V6, now pass manual and auto tests > > > V8: Fix missing return after free skb > > > > > > > Arguably, as Brian also said, that missing return is completely > > unrelated and should perhaps be a separate commit? > > I think the key here is that Polaris's buggy patch has already been > applied, so this v8 doesn't really work. He has since submitted a proper > fixup patch that applies appropriately, and I've Ack'd that. > Right, sorry, I missed that, was looking only at wireless, not wireless- next. Thanks all, I'll leave the -next patches for Kalle though :) johannes
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_rx.c b/drivers/net/wireless/marvell/mwifiex/sta_rx.c index 13659b02ba88..65420ad67416 100644 --- a/drivers/net/wireless/marvell/mwifiex/sta_rx.c +++ b/drivers/net/wireless/marvell/mwifiex/sta_rx.c @@ -86,6 +86,15 @@ int mwifiex_process_rx_packet(struct mwifiex_private *priv, rx_pkt_len = le16_to_cpu(local_rx_pd->rx_pkt_length); rx_pkt_hdr = (void *)local_rx_pd + rx_pkt_off; + if (sizeof(*rx_pkt_hdr) + rx_pkt_off > skb->len) { + mwifiex_dbg(priv->adapter, ERROR, + "wrong rx packet offset: len=%d, rx_pkt_off=%d\n", + skb->len, rx_pkt_off); + priv->stats.rx_dropped++; + dev_kfree_skb_any(skb); + return -1; + } + if ((!memcmp(&rx_pkt_hdr->rfc1042_hdr, bridge_tunnel_header, sizeof(bridge_tunnel_header))) || (!memcmp(&rx_pkt_hdr->rfc1042_hdr, rfc1042_header, @@ -194,7 +203,8 @@ int mwifiex_process_sta_rx_packet(struct mwifiex_private *priv, rx_pkt_hdr = (void *)local_rx_pd + rx_pkt_offset; - if ((rx_pkt_offset + rx_pkt_length) > (u16) skb->len) { + if ((rx_pkt_offset + rx_pkt_length) > skb->len || + sizeof(rx_pkt_hdr->eth803_hdr) + rx_pkt_offset > skb->len) { mwifiex_dbg(adapter, ERROR, "wrong rx packet: len=%d, rx_pkt_offset=%d, rx_pkt_length=%d\n", skb->len, rx_pkt_offset, rx_pkt_length); diff --git a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c index e495f7eaea03..c1b8d41dd753 100644 --- a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c +++ b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c @@ -103,6 +103,16 @@ static void mwifiex_uap_queue_bridged_pkt(struct mwifiex_private *priv, return; } + if (sizeof(*rx_pkt_hdr) + + le16_to_cpu(uap_rx_pd->rx_pkt_offset) > skb->len) { + mwifiex_dbg(adapter, ERROR, + "wrong rx packet offset: len=%d,rx_pkt_offset=%d\n", + skb->len, le16_to_cpu(uap_rx_pd->rx_pkt_offset)); + priv->stats.rx_dropped++; + dev_kfree_skb_any(skb); + return; + } + if ((!memcmp(&rx_pkt_hdr->rfc1042_hdr, bridge_tunnel_header, sizeof(bridge_tunnel_header))) || (!memcmp(&rx_pkt_hdr->rfc1042_hdr, rfc1042_header, @@ -367,6 +377,16 @@ int mwifiex_process_uap_rx_packet(struct mwifiex_private *priv, rx_pkt_type = le16_to_cpu(uap_rx_pd->rx_pkt_type); rx_pkt_hdr = (void *)uap_rx_pd + le16_to_cpu(uap_rx_pd->rx_pkt_offset); + if (le16_to_cpu(uap_rx_pd->rx_pkt_offset) + + sizeof(rx_pkt_hdr->eth803_hdr) > skb->len) { + mwifiex_dbg(adapter, ERROR, + "wrong rx packet for struct ethhdr: len=%d, offset=%d\n", + skb->len, le16_to_cpu(uap_rx_pd->rx_pkt_offset)); + priv->stats.rx_dropped++; + dev_kfree_skb_any(skb); + return 0; + } + ether_addr_copy(ta, rx_pkt_hdr->eth803_hdr.h_source); if ((le16_to_cpu(uap_rx_pd->rx_pkt_offset) + diff --git a/drivers/net/wireless/marvell/mwifiex/util.c b/drivers/net/wireless/marvell/mwifiex/util.c index 94c2d219835d..745b1d925b21 100644 --- a/drivers/net/wireless/marvell/mwifiex/util.c +++ b/drivers/net/wireless/marvell/mwifiex/util.c @@ -393,11 +393,15 @@ mwifiex_process_mgmt_packet(struct mwifiex_private *priv, } rx_pd = (struct rxpd *)skb->data; + pkt_len = le16_to_cpu(rx_pd->rx_pkt_length); + if (pkt_len < sizeof(struct ieee80211_hdr) + sizeof(pkt_len)) { + mwifiex_dbg(priv->adapter, ERROR, "invalid rx_pkt_length"); + return -1; + } skb_pull(skb, le16_to_cpu(rx_pd->rx_pkt_offset)); skb_pull(skb, sizeof(pkt_len)); - - pkt_len = le16_to_cpu(rx_pd->rx_pkt_length); + pkt_len -= sizeof(pkt_len); ieee_hdr = (void *)skb->data; if (ieee80211_is_mgmt(ieee_hdr->frame_control)) { @@ -410,7 +414,7 @@ mwifiex_process_mgmt_packet(struct mwifiex_private *priv, skb->data + sizeof(struct ieee80211_hdr), pkt_len - sizeof(struct ieee80211_hdr)); - pkt_len -= ETH_ALEN + sizeof(pkt_len); + pkt_len -= ETH_ALEN; rx_pd->rx_pkt_length = cpu_to_le16(pkt_len); cfg80211_rx_mgmt(&priv->wdev, priv->roc_cfg.chan.center_freq,
Make sure mwifiex_process_mgmt_packet, mwifiex_process_sta_rx_packet and mwifiex_process_uap_rx_packet, mwifiex_uap_queue_bridged_pkt and mwifiex_process_rx_packet not out-of-bounds access the skb->data buffer. Fixes: 2dbaf751b1de ("mwifiex: report received management frames to cfg80211") Signed-off-by: Polaris Pi <pinkperfect2021@gmail.com> --- V5: Follow chromeos comments: preserve the original flow of mwifiex_process_uap_rx_packet V6: Simplify check in mwifiex_process_uap_rx_packet V7: Fix drop packets issue when auotest V6, now pass manual and auto tests V8: Fix missing return after free skb --- drivers/net/wireless/marvell/mwifiex/sta_rx.c | 12 ++++++++++- .../net/wireless/marvell/mwifiex/uap_txrx.c | 20 +++++++++++++++++++ drivers/net/wireless/marvell/mwifiex/util.c | 10 +++++++--- 3 files changed, 38 insertions(+), 4 deletions(-)