| Message ID | 20230713023731.2518507-1-pinkperfect2021@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v3] wifi: mwifiex: Fix OOB and integer underflow when rx packets | expand |
On Thu, 13 Jul 2023 10:44:56 +0800 Pink Perfect wrote: > Sorry, forgot to change the signoff name, after you review this patch I can > submit a new one Do you mean change to "Pink Perfect", is that the name you'd sign legal documents with? > On Thu, Jul 13, 2023 at 10:37 AM pinkperfect <pinkperfect2021@gmail.com> > wrote: > > > Make sure mwifiex_process_mgmt_packet and its callers > > mwifiex_process_sta_rx_packet and mwifiex_process_uap_rx_packet > > not out-of-bounds access the skb->data buffer. > > > > Fixes: 2dbaf751b1de ("mwifiex: report received management frames to > > cfg80211") > > > > Signed-off-by: pinkperfect <pinkperfect2021@gmail.com> No empty lines between tags, please. You mentioned reporting the problem to chromeos, since mwifiex "maintainters" seem to be AWoL, would someone from their team possibly be willing to venture a review tag for the patch?
Jakub Kicinski <kuba@kernel.org> writes: > On Thu, 13 Jul 2023 10:44:56 +0800 Pink Perfect wrote: >> Sorry, forgot to change the signoff name, after you review this patch I can >> submit a new one > > Do you mean change to "Pink Perfect", is that the name you'd sign > legal documents with? > >> On Thu, Jul 13, 2023 at 10:37 AM pinkperfect <pinkperfect2021@gmail.com> >> wrote: >> >> > Make sure mwifiex_process_mgmt_packet and its callers >> > mwifiex_process_sta_rx_packet and mwifiex_process_uap_rx_packet >> > not out-of-bounds access the skb->data buffer. >> > >> > Fixes: 2dbaf751b1de ("mwifiex: report received management frames to >> > cfg80211") >> > >> > Signed-off-by: pinkperfect <pinkperfect2021@gmail.com> > > No empty lines between tags, please. > > You mentioned reporting the problem to chromeos, since mwifiex > "maintainters" seem to be AWoL, would someone from their team > possibly be willing to venture a review tag for the patch? We have four maintainers for mwifiex and total silence: MARVELL MWIFIEX WIRELESS DRIVER M: Amitkumar Karwar <amitkarwar@gmail.com> M: Ganapathi Bhat <ganapathi017@gmail.com> M: Sharvari Harisangam <sharvari.harisangam@nxp.com> M: Xinming Hu <huxinming820@gmail.com> L: linux-wireless@vger.kernel.org S: Maintained F: drivers/net/wireless/marvell/mwifiex/ I'm very close of marking this driver as orphan unless anyone steps up. This is not how to maintain a driver.
On Thu, 20 Jul 2023 09:55:38 +0300 Kalle Valo wrote: > > No empty lines between tags, please. > > > > You mentioned reporting the problem to chromeos, since mwifiex > > "maintainters" seem to be AWoL, would someone from their team > > possibly be willing to venture a review tag for the patch? > > We have four maintainers for mwifiex and total silence: > > MARVELL MWIFIEX WIRELESS DRIVER > M: Amitkumar Karwar <amitkarwar@gmail.com> > M: Ganapathi Bhat <ganapathi017@gmail.com> > M: Sharvari Harisangam <sharvari.harisangam@nxp.com> > M: Xinming Hu <huxinming820@gmail.com> > L: linux-wireless@vger.kernel.org > S: Maintained > F: drivers/net/wireless/marvell/mwifiex/ > > I'm very close of marking this driver as orphan unless anyone steps up. That seems more than justified at this point.
On Thu, Jul 20, 2023 at 09:55:38AM +0300, Kalle Valo wrote: > We have four maintainers for mwifiex and total silence: > > MARVELL MWIFIEX WIRELESS DRIVER > M: Amitkumar Karwar <amitkarwar@gmail.com> > M: Ganapathi Bhat <ganapathi017@gmail.com> > M: Sharvari Harisangam <sharvari.harisangam@nxp.com> > M: Xinming Hu <huxinming820@gmail.com> > L: linux-wireless@vger.kernel.org > S: Maintained > F: drivers/net/wireless/marvell/mwifiex/ > > I'm very close of marking this driver as orphan unless anyone steps up. > This is not how to maintain a driver. I'd be fully on board with removing these maintainers, as I don't recall hearing from any of them in years. (In fact, some of these addresses don't have a single mail logged on lore.kernel.org/all/...) I just didn't want to be the one to say it. On the other hand, I regularly look at pretty much anything for mwifiex, as long as the submitter is in relatively good faith. So I wouldn't mind being a Reviewer (or Maintainer? what's the difference, when Kalle does the committing anyway?). And that might qualify as "Odd Fixes", as I don't plan on doing much more than keeping the lights on. I'll submit the MAINTAINERS patch if you'd like. Brian
Brian Norris <briannorris@chromium.org> writes: > On Thu, Jul 20, 2023 at 09:55:38AM +0300, Kalle Valo wrote: >> We have four maintainers for mwifiex and total silence: >> >> MARVELL MWIFIEX WIRELESS DRIVER >> M: Amitkumar Karwar <amitkarwar@gmail.com> >> M: Ganapathi Bhat <ganapathi017@gmail.com> >> M: Sharvari Harisangam <sharvari.harisangam@nxp.com> >> M: Xinming Hu <huxinming820@gmail.com> >> L: linux-wireless@vger.kernel.org >> S: Maintained >> F: drivers/net/wireless/marvell/mwifiex/ >> >> I'm very close of marking this driver as orphan unless anyone steps up. >> This is not how to maintain a driver. > > I'd be fully on board with removing these maintainers, as I don't recall > hearing from any of them in years. (In fact, some of these addresses > don't have a single mail logged on lore.kernel.org/all/...) I just > didn't want to be the one to say it. > > On the other hand, I regularly look at pretty much anything for mwifiex, > as long as the submitter is in relatively good faith. So I wouldn't mind > being a Reviewer (or Maintainer? what's the difference, when Kalle > does the committing anyway?). Heh, that's a good question. I don't know what was the original intent for a reviewer role but in my view ideally a driver should 1-2 maintainers, no more, and if there are more people involved they should be reviewers. And maintainers should use Acked-by, Reviewers should use Reviewed-by. And if I see an Acked-by from a maintainer I usually don't review the patch so closely and just take it directly (of there are exceptions, as always). In this case I would prefer you being the maintainer, even if you wouldn't have much time for mwifiex. But I don't know how others see it. Jakub has been writing documentation about maintainership which is also a good read: https://lore.kernel.org/all/20230719183225.1827100-1-kuba@kernel.org/ > And that might qualify as "Odd Fixes", as > I don't plan on doing much more than keeping the lights on. > > I'll submit the MAINTAINERS patch if you'd like. Sounds very good to me, thank you! Please submit the patch if you can.
On Fri, Jul 21, 2023 at 10:15:25AM +0300, Kalle Valo wrote: > Brian Norris <briannorris@chromium.org> writes: > > I'll submit the MAINTAINERS patch if you'd like. > > Sounds very good to me, thank you! Please submit the patch if you can. Done: https://lore.kernel.org/linux-wireless/20230721160603.1.Idf0e8025f59c62d73c08960638249b58cf215acc@changeid/ Brian
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_rx.c b/drivers/net/wireless/marvell/mwifiex/sta_rx.c index 13659b02ba88..88aaec645291 100644 --- a/drivers/net/wireless/marvell/mwifiex/sta_rx.c +++ b/drivers/net/wireless/marvell/mwifiex/sta_rx.c @@ -194,7 +194,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) > (u16)skb->len || + skb->len - rx_pkt_offset < sizeof(*rx_pkt_hdr)) { 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..f84ed22518c6 100644 --- a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c +++ b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c @@ -367,20 +367,15 @@ 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); - ether_addr_copy(ta, rx_pkt_hdr->eth803_hdr.h_source); - if ((le16_to_cpu(uap_rx_pd->rx_pkt_offset) + - le16_to_cpu(uap_rx_pd->rx_pkt_length)) > (u16) skb->len) { + le16_to_cpu(uap_rx_pd->rx_pkt_length)) > (u16)skb->len || + skb->len - le16_to_cpu(uap_rx_pd->rx_pkt_offset) < sizeof(*rx_pkt_hdr)) { mwifiex_dbg(adapter, ERROR, "wrong rx packet: len=%d, offset=%d, length=%d\n", skb->len, le16_to_cpu(uap_rx_pd->rx_pkt_offset), le16_to_cpu(uap_rx_pd->rx_pkt_length)); priv->stats.rx_dropped++; - node = mwifiex_get_sta_entry(priv, ta); - if (node) - node->stats.tx_failed++; - dev_kfree_skb_any(skb); return 0; } @@ -393,6 +388,7 @@ int mwifiex_process_uap_rx_packet(struct mwifiex_private *priv, return ret; } + ether_addr_copy(ta, rx_pkt_hdr->eth803_hdr.h_source); if (rx_pkt_type != PKT_TYPE_BAR && uap_rx_pd->priority < MAX_NUM_TID) { spin_lock_bh(&priv->sta_list_spinlock); diff --git a/drivers/net/wireless/marvell/mwifiex/util.c b/drivers/net/wireless/marvell/mwifiex/util.c index 94c2d219835d..31e1a82883e4 100644 --- a/drivers/net/wireless/marvell/mwifiex/util.c +++ b/drivers/net/wireless/marvell/mwifiex/util.c @@ -399,6 +399,11 @@ mwifiex_process_mgmt_packet(struct mwifiex_private *priv, pkt_len = le16_to_cpu(rx_pd->rx_pkt_length); + if (pkt_len < sizeof(struct ieee80211_hdr) || skb->len < pkt_len) { + mwifiex_dbg(priv->adapter, ERROR, "invalid rx_pkt_length"); + return -1; + } + ieee_hdr = (void *)skb->data; if (ieee80211_is_mgmt(ieee_hdr->frame_control)) { if (mwifiex_parse_mgmt_packet(priv, (u8 *)ieee_hdr,
Make sure mwifiex_process_mgmt_packet and its callers mwifiex_process_sta_rx_packet and mwifiex_process_uap_rx_packet not out-of-bounds access the skb->data buffer. Fixes: 2dbaf751b1de ("mwifiex: report received management frames to cfg80211") Signed-off-by: pinkperfect <pinkperfect2021@gmail.com> --- drivers/net/wireless/marvell/mwifiex/sta_rx.c | 3 ++- drivers/net/wireless/marvell/mwifiex/uap_txrx.c | 10 +++------- drivers/net/wireless/marvell/mwifiex/util.c | 5 +++++ 3 files changed, 10 insertions(+), 8 deletions(-)