Hi,
On Thu, Nov 10, 2016 at 07:57:04PM +0800, Xinming Hu wrote:
> From: Brian Norris <[email protected]>
>
> It's possible for the FW init sequence to fail, which will trigger a
> device cleanup sequence in mwifiex_fw_dpc(). This sequence can race with
> device suspend() or remove() (e.g., reboot or unbind), and can trigger
> use-after-free issues. Currently, this driver attempts (poorly) to
> synchronize remove() using a semaphore, but it doesn't protect some of
> the critical sections properly. Particularly, we grab a pointer to the
> adapter struct (card->adapter) without checking if it's being freed or
> not. We later do a NULL check on the adapter, but that doesn't work if
> the adapter was freed.
>
> Also note that the PCIe interface driver doesn't ever set card->adapter
> to NULL, so even if we get the synchronization right, we still might try
> to redo the cleanup in ->remove(), even if the FW init failure sequence
> already did it.
>
> This patch replaces the static semaphore with a per-device completion
> struct, and uses that completion to synchronize the remove() thread with
> the mwifiex_fw_dpc(). A future patch will utilize this completion to
> synchronize the suspend() thread as well.
>
> Signed-off-by: Brian Norris <[email protected]>
> ---
> v2: Same as v1
> ---
> drivers/net/wireless/marvell/mwifiex/main.c | 46
> ++++++++++-------------------
> drivers/net/wireless/marvell/mwifiex/main.h | 13 +++++---
> drivers/net/wireless/marvell/mwifiex/pcie.c | 18 +++++------
> drivers/net/wireless/marvell/mwifiex/pcie.h | 2 ++
> drivers/net/wireless/marvell/mwifiex/sdio.c | 18 +++++------
> drivers/net/wireless/marvell/mwifiex/sdio.h | 2 ++
> drivers/net/wireless/marvell/mwifiex/usb.c | 23 +++++++--------
> drivers/net/wireless/marvell/mwifiex/usb.h | 2 ++
> 8 files changed, 55 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c
> b/drivers/net/wireless/marvell/mwifiex/main.c
> index c710d5e..09d46d6 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -521,7 +521,6 @@ static void mwifiex_fw_dpc(const struct firmware
> *firmware, void *context)
> struct mwifiex_private *priv;
> struct mwifiex_adapter *adapter = context;
> struct mwifiex_fw_image fw;
> - struct semaphore *sem = adapter->card_sem;
> bool init_failed = false;
> struct wireless_dev *wdev;
>
> @@ -670,7 +669,8 @@ static void mwifiex_fw_dpc(const struct firmware
> *firmware, void *context)
> }
> if (init_failed)
> mwifiex_free_adapter(adapter);
> - up(sem);
> + /* Tell all current and future waiters we're finished */
> + complete_all(adapter->fw_done);
This part introduces a new use-after-free. We need to dereference
adapter->fw_done *before* we free the adapter in mwifiex_free_adapter().
So you need a diff that looks something like this:
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -514,6 +514,7 @@ static void mwifiex_fw_dpc(const struct firmware *firmware,
void *context)
struct mwifiex_fw_image fw;
bool init_failed = false;
struct wireless_dev *wdev;
+ struct completion *fw_done = adapter->fw_done;
if (!firmware) {
mwifiex_dbg(adapter, ERROR,
@@ -654,7 +655,7 @@ done:
if (init_failed)
mwifiex_free_adapter(adapter);
/* Tell all current and future waiters we're finished */
- complete_all(adapter->fw_done);
+ complete_all(fw_done);
return;
}
Brian
> return;
> }
>
...
