Hi, On Thu, Nov 10, 2016 at 07:57:04PM +0800, Xinming Hu wrote: > From: Brian Norris <briannorris@xxxxxxxxxxxx> > > 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 <briannorris@xxxxxxxxxxxx> > --- > 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; > } > ...