| 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
| ||
|
Message-ID: <fe24e109e6524e57814a1d2060a45905@SC-EXCH02.marvell.com>
Date: Wed, 17 May 2017 04:02:46 +0000
From: Xinming Hu <huxm@...vell.com>
To: Brian Norris <briannorris@...omium.org>,
Ganapathi Bhat <gbhat@...vell.com>,
Nishant Sarmukadam <nishants@...vell.com>,
Cathy Luo <cluo@...vell.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Dmitry Torokhov" <dmitry.torokhov@...il.com>,
Amitkumar Karwar <amitkarwar@...il.com>,
Kalle Valo <kvalo@...eaurora.org>,
"linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
"Doug Anderson" <dianders@...omium.org>
Subject: RE: [PATCH 01/11] mwifiex: fixup error cases in
mwifiex_add_virtual_intf()
Hi,
This patch serials looks fine, thanks.
> -----Original Message-----
> From: linux-wireless-owner@...r.kernel.org
> [mailto:linux-wireless-owner@...r.kernel.org] On Behalf Of Brian Norris
> Sent: 2017年5月13日 0:42
> To: Ganapathi Bhat; Nishant Sarmukadam
> Cc: linux-kernel@...r.kernel.org; Dmitry Torokhov; Amitkumar Karwar; Kalle
> Valo; linux-wireless@...r.kernel.org; Doug Anderson; Brian Norris
> Subject: [PATCH 01/11] mwifiex: fixup error cases in mwifiex_add_virtual_intf()
>
> 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").
>
> Cc: <stable@...r.kernel.org>
> Signed-off-by: Brian Norris <briannorris@...omium.org>
> ---
> drivers/net/wireless/marvell/mwifiex/cfg80211.c | 71
> ++++++++++++-------------
> 1 file changed, 35 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> index 7ec06bf13413..025bc06a19d6 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> @@ -2964,10 +2964,8 @@ struct wireless_dev
> *mwifiex_add_virtual_intf(struct wiphy *wiphy,
> if (!dev) {
> mwifiex_dbg(adapter, ERROR,
> "no memory available for netdevice\n");
> - memset(&priv->wdev, 0, sizeof(priv->wdev));
> - priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED;
> - priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED;
> - return ERR_PTR(-ENOMEM);
> + ret = -ENOMEM;
> + goto err_alloc_netdev;
> }
>
> mwifiex_init_priv_params(priv, dev);
> @@ -2976,11 +2974,11 @@ struct wireless_dev
> *mwifiex_add_virtual_intf(struct wiphy *wiphy,
> ret = mwifiex_send_cmd(priv, HostCmd_CMD_SET_BSS_MODE,
> HostCmd_ACT_GEN_SET, 0, NULL, true);
> if (ret)
> - return ERR_PTR(ret);
> + goto err_set_bss_mode;
>
> ret = mwifiex_sta_init_cmd(priv, false, false);
> if (ret)
> - return ERR_PTR(ret);
> + goto err_sta_init;
>
> mwifiex_setup_ht_caps(&wiphy->bands[NL80211_BAND_2GHZ]->ht_cap,
> priv);
> if (adapter->is_hw_11ac_capable)
> @@ -3011,31 +3009,14 @@ struct wireless_dev
> *mwifiex_add_virtual_intf(struct wiphy *wiphy,
>
> SET_NETDEV_DEV(dev, adapter->dev);
>
> - /* Register network device */
> - if (register_netdevice(dev)) {
> - mwifiex_dbg(adapter, ERROR,
> - "cannot register virtual network device\n");
> - free_netdev(dev);
> - priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED;
> - priv->netdev = NULL;
> - memset(&priv->wdev, 0, sizeof(priv->wdev));
> - priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED;
> - return ERR_PTR(-EFAULT);
> - }
> -
> priv->dfs_cac_workqueue = alloc_workqueue("MWIFIEX_DFS_CAC%s",
> WQ_HIGHPRI |
> WQ_MEM_RECLAIM |
> WQ_UNBOUND, 1, name);
> if (!priv->dfs_cac_workqueue) {
> - mwifiex_dbg(adapter, ERROR,
> - "cannot register virtual network device\n");
> - free_netdev(dev);
> - priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED;
> - priv->netdev = NULL;
> - memset(&priv->wdev, 0, sizeof(priv->wdev));
> - priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED;
> - return ERR_PTR(-ENOMEM);
> + mwifiex_dbg(adapter, ERROR, "cannot alloc DFS CAC queue\n");
> + ret = -ENOMEM;
> + goto err_alloc_cac;
> }
>
> INIT_DELAYED_WORK(&priv->dfs_cac_work,
> mwifiex_dfs_cac_work_queue); @@ -3044,16 +3025,9 @@ struct wireless_dev
> *mwifiex_add_virtual_intf(struct wiphy *wiphy,
> WQ_HIGHPRI | WQ_UNBOUND |
> WQ_MEM_RECLAIM, 1, name);
> if (!priv->dfs_chan_sw_workqueue) {
> - mwifiex_dbg(adapter, ERROR,
> - "cannot register virtual network device\n");
> - free_netdev(dev);
> - priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED;
> - priv->netdev = NULL;
> - memset(&priv->wdev, 0, sizeof(priv->wdev));
> - priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED;
> - destroy_workqueue(priv->dfs_cac_workqueue);
> - priv->dfs_cac_workqueue = NULL;
> - return ERR_PTR(-ENOMEM);
> + mwifiex_dbg(adapter, ERROR, "cannot alloc DFS channel sw
> queue\n");
> + ret = -ENOMEM;
> + goto err_alloc_chsw;
> }
>
> INIT_DELAYED_WORK(&priv->dfs_chan_sw_work,
> @@ -3061,6 +3035,13 @@ struct wireless_dev
> *mwifiex_add_virtual_intf(struct wiphy *wiphy,
>
> sema_init(&priv->async_sem, 1);
>
> + /* Register network device */
> + if (register_netdevice(dev)) {
> + mwifiex_dbg(adapter, ERROR, "cannot register network device\n");
> + ret = -EFAULT;
> + goto err_reg_netdev;
> + }
> +
> mwifiex_dbg(adapter, INFO,
> "info: %s: Marvell 802.11 Adapter\n", dev->name);
>
> @@ -3081,11 +3062,29 @@ struct wireless_dev
> *mwifiex_add_virtual_intf(struct wiphy *wiphy,
> adapter->curr_iface_comb.p2p_intf++;
> break;
> default:
> + /* This should be dead code; checked above */
> mwifiex_dbg(adapter, ERROR, "type not supported\n");
> return ERR_PTR(-EINVAL);
> }
>
> return &priv->wdev;
> +
> +err_reg_netdev:
> + destroy_workqueue(priv->dfs_chan_sw_workqueue);
> + priv->dfs_chan_sw_workqueue = NULL;
> +err_alloc_chsw:
> + destroy_workqueue(priv->dfs_cac_workqueue);
> + priv->dfs_cac_workqueue = NULL;
> +err_alloc_cac:
> + free_netdev(dev);
> + priv->netdev = NULL;
> +err_sta_init:
> +err_set_bss_mode:
> +err_alloc_netdev:
> + memset(&priv->wdev, 0, sizeof(priv->wdev));
> + priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED;
> + priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED;
> + return ERR_PTR(ret);
> }
> EXPORT_SYMBOL_GPL(mwifiex_add_virtual_intf);
>
Regards,
Simon
> --
> 2.13.0.rc2.291.g57267f2277-goog
Powered by blists - more mailing lists