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 <[email protected]>
---
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
Reviewed-by: Matthew Wang <[email protected]>
On Sun, Jul 23, 2023 at 9:07 AM Polaris Pi <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
> ---
> 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 <[email protected]>
Brian Norris <[email protected]> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
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.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Wed, Jul 26, 2023 at 11:10 PM Kalle Valo <[email protected]> wrote:
>
> Brian Norris <[email protected]> writes:
> > Reviewed-by: Brian Norris <[email protected]>
>
> 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 <[email protected]> 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 <[email protected]>
> Reviewed-by: Matthew Wang <[email protected]>
> Reviewed-by: Brian Norris <[email protected]>
Patch applied to wireless-next.git, thanks.
119585281617 wifi: mwifiex: Fix OOB and integer underflow when rx packets
--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches