| Message ID | 20230714224809.3929539-1-pinkperfect2021@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v6] wifi: mwifiex: Fix OOB and integer underflow when rx packets | expand |
Reviewed-by: Matthew Wang <matthewmwang@chromium.org> On Sat, Jul 15, 2023 at 12:48 AM Polaris Pi <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: 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 > --- > 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, 17 insertions(+), 1 deletion(-) > > 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..f0711b73ba3e 100644 > --- a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c > +++ b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c > @@ -367,6 +367,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) > skb->len) { > + mwifiex_dbg(adapter, ERROR, > + "wrong rx packet offset: 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..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, > -- > 2.25.1 >
Hi Jakub, This patch has been reviewed by chromeos wifi team, and review tag has been added by their tech leader. Could you please review it again? Thanks!
On Mon, 17 Jul 2023 14:02:32 +0000 Polaris Pi wrote: > Hi Jakub, > > This patch has been reviewed by chromeos wifi team, and review tag > has been added by their tech leader. > Could you please review it again? Looks fine, I'll leave it to the wireless maintainer to process once they are back from their vacation. It doesn't look very urgent to me.
> 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) { I've tested this patch a bit on a ChromeOS device and I've noticed empirically that skb->len is often (always?) two less than pkt_len, implying that pkt_len actually includes the rx_pkt_length field as well (note that pkt_len gets adjusted by ETH_ALEN + sizeof(pkt_len) below), so we end up hitting this condition reliably in certain situations. This probably means the memmove below is not entirely correct, but either way I don't think this patch is correct on its own. Consider my Reviewed-by tag removed until this gets resolved. > + 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, > --
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..f0711b73ba3e 100644 --- a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c +++ b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c @@ -367,6 +367,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) > skb->len) { + mwifiex_dbg(adapter, ERROR, + "wrong rx packet offset: 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..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: 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 --- 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, 17 insertions(+), 1 deletion(-)