linux-kernel - Re: [01/11] 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] [day] [month] [year] [list]

Message-Id: <20170519060250.3457960585@smtp.codeaurora.org>
Date:   Fri, 19 May 2017 06:02:50 +0000 (UTC)
From:   Kalle Valo <kvalo@...eaurora.org>
To:     Brian Norris <briannorris@...omium.org>
Cc:     Ganapathi Bhat <gbhat@...vell.com>,
        Nishant Sarmukadam <nishants@...vell.com>,
        <linux-kernel@...r.kernel.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Amitkumar Karwar <amitkarwar@...il.com>,
        linux-wireless@...r.kernel.org,
        Doug Anderson <dianders@...omium.org>,
        Brian Norris <briannorris@...omium.org>
Subject: Re: [01/11] mwifiex: fixup error cases in mwifiex_add_virtual_intf()

Brian Norris <briannorris@...omium.org> wrote:
> 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>

11 patches applied to wireless-drivers-next.git, thanks.

8535107aa4ef mwifiex: fixup error cases in mwifiex_add_virtual_intf()
e0b636e5ee15 mwifiex: Don't release tx_ba_stream_tbl_lock while iterating
90ad0be83676 mwifiex: Don't release cmd_pending_q_lock while iterating
09bdb6500551 mwifiex: Add locking to mwifiex_11n_delba
0f13acf0c612 mwifiex: don't drop lock between list-retrieval / list-deletion
6eb2d002d4ea mwifiex: don't leak stashed beacon buffer on reset
bc69ca391eff mwifiex: remove useless 'mwifiex_lock'
7170862738dc mwifiex: remove redundant 'adapter' check in mwifiex_adapter_cleanup
7ade530e7384 mwifiex: 11h: drop unnecessary check for '!priv'
fa4651e12ae8 mwifiex: pcie: remove useless pdev check
68efd0386988 mwifiex: pcie: stop setting/clearing 'surprise_removed'

-- 
https://patchwork.kernel.org/patch/9724599/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches