linux-kernel - [PATCH 3.16 007/192] mwifiex: fixup error cases in mwifiex_add_virtual

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <lsq.1507553064.258443344@decadent.org.uk>
Date:   Mon, 09 Oct 2017 13:44:24 +0100
From:   Ben Hutchings <ben@...adent.org.uk>
To:     linux-kernel@...r.kernel.org, stable@...r.kernel.org
CC:     akpm@...ux-foundation.org, "Kalle Valo" <kvalo@...eaurora.org>,
        "Brian Norris" <briannorris@...omium.org>
Subject: [PATCH 3.16 007/192] mwifiex: fixup error cases in
 mwifiex_add_virtual_intf()

3.16.49-rc1 review patch.  If anyone has any objections, please let me know.

------------------

From: Brian Norris <briannorris@...omium.org>

commit 8535107aa4ef92520cbb9a4739563b389c5f8e2c upstream.

If we fail to add an interface in mwifiex_add_virtual_intf(), we might
hit a BUG_ON() in the networking code, because we didn't tear things
down properly. Among the problems:

 (a) when failing to allocate workqueues, we fail to unregister the
     netdev before calling free_netdev()
 (b) even if we do try to unregister the netdev, we're still holding the
     rtnl lock, so the device never properly unregistered; we'll be at
     state NETREG_UNREGISTERING, and then hit free_netdev()'s:
	BUG_ON(dev->reg_state != NETREG_UNREGISTERED);
 (c) we're allocating some dependent resources (e.g., DFS workqueues)
     after we've registered the interface; this may or may not cause
     problems, but it's good practice to allocate these before registering
 (d) we're not even trying to unwind anything when mwifiex_send_cmd() or
     mwifiex_sta_init_cmd() fail

To fix these issues, let's:

 * add a stacked set of error handling labels, to keep error handling
   consistent and properly ordered (resolving (a) and (d))
 * move the workqueue allocations before the registration (to resolve
   (c); also resolves (b) by avoiding error cases where we have to
   unregister)

[Incidentally, it's pretty easy to interrupt the alloc_workqueue() in,
e.g., the following:

  iw phy phy0 interface add mlan0 type station

by sending it SIGTERM.]

This bugfix covers commits like commit 7d652034d1a0 ("mwifiex: channel
switch support for mwifiex"), but parts of this bug exist all the way
back to the introduction of dynamic interface handling in commit
93a1df48d224 ("mwifiex: add cfg80211 handlers add/del_virtual_intf").

Signed-off-by: Brian Norris <briannorris@...omium.org>
Signed-off-by: Kalle Valo <kvalo@...eaurora.org>
[bwh: Backported to 3.16:
 - There is no workqueue allocation or cleanup needed here
 - Add 'ret' variable
 - Keep logging errors with wiphy_err()
 - Adjust filename]
Signed-off-by: Ben Hutchings <ben@...adent.org.uk>
---
--- a/drivers/net/wireless/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/mwifiex/cfg80211.c
@@ -2160,6 +2160,7 @@ struct wireless_dev *mwifiex_add_virtual
 	struct net_device *dev;
 	void *mdev_priv;
 	struct wireless_dev *wdev;
+	int ret;
 
 	if (!adapter)
 		return ERR_PTR(-EFAULT);
@@ -2254,8 +2255,8 @@ struct wireless_dev *mwifiex_add_virtual
 		priv->bss_num = 0;
 
 		if (mwifiex_cfg80211_init_p2p_client(priv)) {
-			wdev = ERR_PTR(-EFAULT);
-			goto done;
+			ret = -EFAULT;
+			goto err_set_bss_mode;
 		}
 
 		break;
@@ -2268,9 +2269,8 @@ struct wireless_dev *mwifiex_add_virtual
 			       ether_setup, IEEE80211_NUM_ACS, 1);
 	if (!dev) {
 		wiphy_err(wiphy, "no memory available for netdevice\n");
-		priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED;
-		wdev = ERR_PTR(-ENOMEM);
-		goto done;
+		ret = -ENOMEM;
+		goto err_alloc_netdev;
 	}
 
 	mwifiex_init_priv_params(priv, dev);
@@ -2305,31 +2305,32 @@ struct wireless_dev *mwifiex_add_virtual
 
 	SET_NETDEV_DEV(dev, adapter->dev);
 
+	sema_init(&priv->async_sem, 1);
+
 	/* Register network device */
 	if (register_netdevice(dev)) {
 		wiphy_err(wiphy, "cannot register virtual network device\n");
-		free_netdev(dev);
-		priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED;
-		priv->netdev = NULL;
-		wdev = ERR_PTR(-EFAULT);
-		goto done;
+		ret = -EFAULT;
+		goto err_reg_netdev;
 	}
 
-	sema_init(&priv->async_sem, 1);
-
 	dev_dbg(adapter->dev, "info: %s: Marvell 802.11 Adapter\n", dev->name);
 
 #ifdef CONFIG_DEBUG_FS
 	mwifiex_dev_debugfs_init(priv);
 #endif
 
-done:
-	if (IS_ERR(wdev)) {
-		kfree(priv->wdev);
-		priv->wdev = NULL;
-	}
-
 	return wdev;
+
+err_reg_netdev:
+	free_netdev(dev);
+	priv->netdev = NULL;
+err_set_bss_mode:
+err_alloc_netdev:
+	kfree(priv->wdev);
+	priv->wdev = NULL;
+	priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED;
+	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(mwifiex_add_virtual_intf);