| Message ID | 20230714120222.3920248-1-pinkperfect2021@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v5] wifi: mwifiex: Fix OOB and integer underflow when rx packets | expand |
Left some comments around the formatting. Did you run checkpatch.pl before pushing the change , if not please run. On Fri, Jul 14, 2023 at 5:02 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 > --- > drivers/net/wireless/marvell/mwifiex/sta_rx.c | 3 ++- > drivers/net/wireless/marvell/mwifiex/uap_txrx.c | 11 +++++++++++ > drivers/net/wireless/marvell/mwifiex/util.c | 5 +++++ > 3 files changed, 18 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 || formatting issue, space after (u16) > + 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..32f4626a4d76 100644 > --- a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c > +++ b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c > @@ -367,6 +367,17 @@ 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) > skb->len || > + skb->len - le16_to_cpu(uap_rx_pd->rx_pkt_offset) < sizeof(*rx_pkt_hdr)) { > + 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; why return -1 instead of returning appropriate error code , may be EMSGSIZE Thanks Abhishek > + } > + > 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 >
On Fri, Jul 14, 2023 at 2:02 PM 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 > --- > drivers/net/wireless/marvell/mwifiex/sta_rx.c | 3 ++- > drivers/net/wireless/marvell/mwifiex/uap_txrx.c | 11 +++++++++++ > drivers/net/wireless/marvell/mwifiex/util.c | 5 +++++ > 3 files changed, 18 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..32f4626a4d76 100644 > --- a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c > +++ b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c > @@ -367,6 +367,17 @@ 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) > skb->len || > + skb->len - le16_to_cpu(uap_rx_pd->rx_pkt_offset) < sizeof(*rx_pkt_hdr)) { The second condition is strictly a subset of the first. Can we just make this: 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 >
> formatting issue, space after (u16) Delete space is required by checkpatch.pl > why return -1 instead of returning appropriate error code , may be EMSGSIZE Because of all other error paths return -1, follow original return code Thanks
From: Polaris Pi > Sent: 14 July 2023 13:02 > > 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. > ... > 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)) { What is the (u16) cast for? The domain of both tx_pkt_offset and rx_pkt_length is 16bits. Their sum is of type 'int', masking skb->len to 16 bits just discards high bits (which are probably zero). The second test should probably be: rx_pkt_offset + sizeof(*rx_pkt_hdr) > skb->len although the first test includes the 'offset > len' check. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
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..32f4626a4d76 100644 --- a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c +++ b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c @@ -367,6 +367,17 @@ 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) > skb->len || + skb->len - le16_to_cpu(uap_rx_pd->rx_pkt_offset) < sizeof(*rx_pkt_hdr)) { + 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 --- drivers/net/wireless/marvell/mwifiex/sta_rx.c | 3 ++- drivers/net/wireless/marvell/mwifiex/uap_txrx.c | 11 +++++++++++ drivers/net/wireless/marvell/mwifiex/util.c | 5 +++++ 3 files changed, 18 insertions(+), 1 deletion(-)