mwifiex: fix memory corruption when unsetting multicast list

PATCH In review Daniel Drake 2 participants 2 messages Jun 14, 2013 – Jun 15, 2013 Networking drivers (wireless)

Share & tools

Daniel Drake

author

2013-06-14 19:29 UTC #0 lore raw

PATCH 1/1 mwifiex: fix memory corruption when unsetting multicast list

When trying to unset a previously-set multicast list (i.e. the new list has 0 entries), mwifiex_set_multicast_list() was calling down to mwifiex_request_set_multicast_list() while leaving mcast_list.num_multicast_addr as an uninitialized value.

We were arriving at mwifiex_cmd_mac_multicast_adr() which would then proceed to do an often huge memcpy of mcast_list.num_multicast_addr*ETH_ALEN bytes, causing memory corruption and hard to debug crashes.

Fix this by setting mcast_list.num_multicast_addr to 0 when no multicast list is provided. Similarly, fix up the logic in mwifiex_request_set_multicast_list() to unset the multicast list that was previously sent to the hardware in such cases.

Signed-off-by Daniel Drake <[email protected]>

---
 drivers/net/wireless/mwifiex/main.c      |  5 ++---
 drivers/net/wireless/mwifiex/sta_ioctl.c | 18 ++++++++----------
 2 files changed, 10 insertions(+), 13 deletions(-)

drivers/net/wireless/mwifiex/main.c +2 -3

index 2eb88ea..4f5c37a 100644
@@ -573,9 +573,8 @@ static void mwifiex_set_multicast_list(struct net_device *dev)
 		mcast_list.mode = MWIFIEX_ALL_MULTI_MODE;
 	} else {
 		mcast_list.mode = MWIFIEX_MULTICAST_MODE;
-		if (netdev_mc_count(dev))
-			mcast_list.num_multicast_addr =
-				mwifiex_copy_mcast_addr(&mcast_list, dev);
+		mcast_list.num_multicast_addr =
+			mwifiex_copy_mcast_addr(&mcast_list, dev);
 	}
 	mwifiex_request_set_multicast_list(priv, &mcast_list);
 }

drivers/net/wireless/mwifiex/sta_ioctl.c +8 ~~-10~~

index 1a8a19d..23aa910 100644
@@ -104,16 +104,14 @@ int mwifiex_request_set_multicast_list(struct mwifiex_private *priv,
 		} else {
 			priv->curr_pkt_filter &=
 				~HostCmd_ACT_MAC_ALL_MULTICAST_ENABLE;
-			if (mcast_list->num_multicast_addr) {
-				dev_dbg(priv->adapter->dev,
-					"info: Set multicast list=%d\n",
-				       mcast_list->num_multicast_addr);
-				/* Send multicast addresses to firmware */
-				ret = mwifiex_send_cmd_async(priv,
-					HostCmd_CMD_MAC_MULTICAST_ADR,
-					HostCmd_ACT_GEN_SET, 0,
-					mcast_list);
-			}
+			dev_dbg(priv->adapter->dev,
+				"info: Set multicast list=%d\n",
+				mcast_list->num_multicast_addr);
+			/* Send multicast addresses to firmware */
+			ret = mwifiex_send_cmd_async(priv,
+				HostCmd_CMD_MAC_MULTICAST_ADR,
+				HostCmd_ACT_GEN_SET, 0,
+				mcast_list);
 		}
 	}
 	dev_dbg(priv->adapter->dev,

Signature · 1 line

1.8.1.4

Bing Zhao

Acked-by on v1 1/1

2013-06-15 04:07 UTC #1 lore raw

Hi Daniel,

Thanks for the patch.

Quoted 10 lines

When trying to unset a previously-set multicast list (i.e. the new list has 0 entries), mwifiex_set_multicast_list() was calling down to mwifiex_request_set_multicast_list() while leaving mcast_list.num_multicast_addr as an uninitialized value.
We were arriving at mwifiex_cmd_mac_multicast_adr() which would then proceed to do an often huge memcpy of mcast_list.num_multicast_addr*ETH_ALEN bytes, causing memory corruption and hard to debug crashes.

Quoted 6 lines

Fix this by setting mcast_list.num_multicast_addr to 0 when no multicast list is provided. Similarly, fix up the logic in mwifiex_request_set_multicast_list() to unset the multicast list that was previously sent to the hardware in such cases.
Signed-off-by Daniel Drake <[email protected]>

Acked-by Bing Zhao <[email protected]>

Thanks,
Bing

Quoted 4 lines

---
 drivers/net/wireless/mwifiex/main.c      |  5 ++---
 drivers/net/wireless/mwifiex/sta_ioctl.c | 18 ++++++++----------
 2 files changed, 10 insertions(+), 13 deletions(-)