| Message ID | 20230723070741.1544662-1-pinkperfect2021@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 11958528161731c58e105b501ed60b83a91ea941 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v7] wifi: mwifiex: Fix OOB and integer underflow when rx packets | expand |
Reviewed-by: Matthew Wang <matthewmwang@chromium.org> On Sun, Jul 23, 2023 at 9:07 AM Polaris Pi <pinkperfect2021@gmail.com> 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 > --- > drivers/net/wireless/marvell/mwifiex/sta_rx.c | 11 ++++++++++- > .../net/wireless/marvell/mwifiex/uap_txrx.c | 19 +++++++++++++++++++ > drivers/net/wireless/marvell/mwifiex/util.c | 10 +++++++--- > 3 files changed, 36 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/wireless/marvell/mwifiex/sta_rx.c b/drivers/net/wireless/marvell/mwifiex/sta_rx.c > index 13659b02ba88..f2899d53a43f 100644 > --- a/drivers/net/wireless/marvell/mwifiex/sta_rx.c > +++ b/drivers/net/wireless/marvell/mwifiex/sta_rx.c > @@ -86,6 +86,14 @@ 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); > + } > + > if ((!memcmp(&rx_pkt_hdr->rfc1042_hdr, bridge_tunnel_header, > sizeof(bridge_tunnel_header))) || > (!memcmp(&rx_pkt_hdr->rfc1042_hdr, rfc1042_header, > @@ -194,7 +202,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..04ff051f5d18 100644 > --- a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c > +++ b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c > @@ -103,6 +103,15 @@ 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); > + } > + > if ((!memcmp(&rx_pkt_hdr->rfc1042_hdr, bridge_tunnel_header, > sizeof(bridge_tunnel_header))) || > (!memcmp(&rx_pkt_hdr->rfc1042_hdr, rfc1042_header, > @@ -367,6 +376,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, > -- > 2.25.1 >
On Sun, Jul 23, 2023 at 07:07:41AM +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 "auto tests" isn't clear to anyone not familiar with Chromium stuff. It'd be courteous to at least make an attempt to describe what this means (even just, "ChromeOS WiFi test suite" or something). For the record, I believe that's approximately this? https://chromium.googlesource.com/chromiumos/third_party/autotest/+/HEAD/docs/wificell.md Anyway, I think the patch contents look good: Reviewed-by: Brian Norris <briannorris@chromium.org>
Brian Norris <briannorris@chromium.org> writes: > On Sun, Jul 23, 2023 at 07:07:41AM +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 > > "auto tests" isn't clear to anyone not familiar with Chromium stuff. > It'd be courteous to at least make an attempt to describe what this > means (even just, "ChromeOS WiFi test suite" or something). For the > record, I believe that's approximately this? > > https://chromium.googlesource.com/chromiumos/third_party/autotest/+/HEAD/docs/wificell.md > > Anyway, I think the patch contents look good: > > Reviewed-by: Brian Norris <briannorris@chromium.org> I'm nitpicking but now that you (Brian) are a maintainer I would prefer that you use Acked-by instead of Reviewed-by. Patchwork shows the statistics (A/R/T in the web ui) and then it's easy for me to see that the patch is ready to be applied. This is for the future, no need to change anything here.
On Wed, Jul 26, 2023 at 11:10 PM Kalle Valo <kvalo@kernel.org> wrote: > > Brian Norris <briannorris@chromium.org> writes: > > Reviewed-by: Brian Norris <briannorris@chromium.org> > > I'm nitpicking but now that you (Brian) are a maintainer I would prefer > that you use Acked-by instead of Reviewed-by. Patchwork shows the > statistics (A/R/T in the web ui) and then it's easy for me to see that > the patch is ready to be applied. This is for the future, no need to > change anything here. Argh, I knew that's the recommendation, and I thought I did that here, but obviously not. Thanks for the reminder. I'm sure I'll fix my muscle memory eventually :) Brian
Polaris Pi <pinkperfect2021@gmail.com> 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> > Reviewed-by: Matthew Wang <matthewmwang@chromium.org> > Reviewed-by: Brian Norris <briannorris@chromium.org> Patch applied to wireless-next.git, thanks. 119585281617 wifi: mwifiex: Fix OOB and integer underflow when rx packets
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_rx.c b/drivers/net/wireless/marvell/mwifiex/sta_rx.c index 13659b02ba88..f2899d53a43f 100644 --- a/drivers/net/wireless/marvell/mwifiex/sta_rx.c +++ b/drivers/net/wireless/marvell/mwifiex/sta_rx.c @@ -86,6 +86,14 @@ 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); + } + if ((!memcmp(&rx_pkt_hdr->rfc1042_hdr, bridge_tunnel_header, sizeof(bridge_tunnel_header))) || (!memcmp(&rx_pkt_hdr->rfc1042_hdr, rfc1042_header, @@ -194,7 +202,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..04ff051f5d18 100644 --- a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c +++ b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c @@ -103,6 +103,15 @@ 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); + } + if ((!memcmp(&rx_pkt_hdr->rfc1042_hdr, bridge_tunnel_header, sizeof(bridge_tunnel_header))) || (!memcmp(&rx_pkt_hdr->rfc1042_hdr, rfc1042_header, @@ -367,6 +376,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 --- drivers/net/wireless/marvell/mwifiex/sta_rx.c | 11 ++++++++++- .../net/wireless/marvell/mwifiex/uap_txrx.c | 19 +++++++++++++++++++ drivers/net/wireless/marvell/mwifiex/util.c | 10 +++++++--- 3 files changed, 36 insertions(+), 4 deletions(-)