Return-path: Received: from mx0b-0016f401.pphosted.com ([67.231.156.173]:37887 "EHLO mx0b-0016f401.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751788AbcKROBc (ORCPT ); Fri, 18 Nov 2016 09:01:32 -0500 From: Amitkumar Karwar To: CC: Cathy Luo , Nishant Sarmukadam , , , , Brian Norris Subject: [PATCH v4 03/11] mwifiex: resolve races between async FW init (failure) and device removal Date: Fri, 18 Nov 2016 19:30:26 +0530 Message-ID: <1479477634-27841-3-git-send-email-akarwar@marvell.com> (sfid-20161118_150135_503619_DBA9FF90) In-Reply-To: <1479477634-27841-1-git-send-email-akarwar@marvell.com> References: <1479477634-27841-1-git-send-email-akarwar@marvell.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-wireless-owner@vger.kernel.org List-ID: From: Brian Norris 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 --- v2: Same as v1 v3: Included Brian's suggested change which fixes new use-after-free introduced by this patch. v4: Resolved minor conflict while rebasing v3 on top of "[v4,1/3] mwifiex: Allow mwifiex early access to device structure" --- drivers/net/wireless/marvell/mwifiex/main.c | 47 +++++++++++------------------ drivers/net/wireless/marvell/mwifiex/main.h | 11 +++++-- 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(+), 68 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c index 9e7acb4..eac44fe 100644 --- a/drivers/net/wireless/marvell/mwifiex/main.c +++ b/drivers/net/wireless/marvell/mwifiex/main.c @@ -521,9 +521,9 @@ 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; + struct completion *fw_done = adapter->fw_done; if (!firmware) { mwifiex_dbg(adapter, ERROR, @@ -670,7 +670,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(fw_done); return; } @@ -1365,7 +1366,7 @@ static void mwifiex_main_work_queue(struct work_struct *work) * code is extracted from mwifiex_remove_card() */ static int -mwifiex_shutdown_sw(struct mwifiex_adapter *adapter, struct semaphore *sem) +mwifiex_shutdown_sw(struct mwifiex_adapter *adapter) { struct mwifiex_private *priv; int i; @@ -1373,8 +1374,9 @@ static void mwifiex_main_work_queue(struct work_struct *work) if (!adapter) goto exit_return; - if (down_interruptible(sem)) - goto exit_sem_err; + wait_for_completion(adapter->fw_done); + /* Caller should ensure we aren't suspending while this happens */ + reinit_completion(adapter->fw_done); priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY); mwifiex_deauthenticate(priv, NULL); @@ -1431,8 +1433,6 @@ static void mwifiex_main_work_queue(struct work_struct *work) rtnl_unlock(); } - up(sem); -exit_sem_err: mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__); exit_return: return 0; @@ -1442,21 +1442,18 @@ static void mwifiex_main_work_queue(struct work_struct *work) * code is extracted from mwifiex_add_card() */ static int -mwifiex_reinit_sw(struct mwifiex_adapter *adapter, struct semaphore *sem, +mwifiex_reinit_sw(struct mwifiex_adapter *adapter, struct completion *fw_done, struct mwifiex_if_ops *if_ops, u8 iface_type) { char fw_name[32]; struct pcie_service_card *card = adapter->card; - if (down_interruptible(sem)) - goto exit_sem_err; - mwifiex_init_lock_list(adapter); if (adapter->if_ops.up_dev) adapter->if_ops.up_dev(adapter); adapter->iface_type = iface_type; - adapter->card_sem = sem; + adapter->fw_done = fw_done; adapter->hw_status = MWIFIEX_HW_STATUS_INITIALIZING; adapter->surprise_removed = false; @@ -1507,7 +1504,8 @@ static void mwifiex_main_work_queue(struct work_struct *work) } strcpy(adapter->fw_name, fw_name); mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__); - up(sem); + + complete_all(adapter->fw_done); return 0; err_init_fw: @@ -1527,8 +1525,7 @@ static void mwifiex_main_work_queue(struct work_struct *work) err_kmalloc: mwifiex_terminate_workqueue(adapter); adapter->surprise_removed = true; - up(sem); -exit_sem_err: + complete_all(adapter->fw_done); mwifiex_dbg(adapter, INFO, "%s, error\n", __func__); return -1; @@ -1543,12 +1540,12 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare) struct mwifiex_if_ops if_ops; if (!prepare) { - mwifiex_reinit_sw(adapter, adapter->card_sem, &if_ops, + mwifiex_reinit_sw(adapter, adapter->fw_done, &if_ops, adapter->iface_type); } else { memcpy(&if_ops, &adapter->if_ops, sizeof(struct mwifiex_if_ops)); - mwifiex_shutdown_sw(adapter, adapter->card_sem); + mwifiex_shutdown_sw(adapter); } } EXPORT_SYMBOL_GPL(mwifiex_do_flr); @@ -1618,15 +1615,12 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter) * - Add logical interfaces */ int -mwifiex_add_card(void *card, struct semaphore *sem, +mwifiex_add_card(void *card, struct completion *fw_done, struct mwifiex_if_ops *if_ops, u8 iface_type, struct device *dev) { struct mwifiex_adapter *adapter; - if (down_interruptible(sem)) - goto exit_sem_err; - if (mwifiex_register(card, if_ops, (void **)&adapter)) { pr_err("%s: software init failed\n", __func__); goto err_init_sw; @@ -1636,7 +1630,7 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter) mwifiex_probe_of(adapter); adapter->iface_type = iface_type; - adapter->card_sem = sem; + adapter->fw_done = fw_done; adapter->hw_status = MWIFIEX_HW_STATUS_INITIALIZING; adapter->surprise_removed = false; @@ -1705,9 +1699,7 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter) mwifiex_free_adapter(adapter); err_init_sw: - up(sem); -exit_sem_err: return -1; } EXPORT_SYMBOL_GPL(mwifiex_add_card); @@ -1723,14 +1715,11 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter) * - Unregister the device * - Free the adapter structure */ -int mwifiex_remove_card(struct mwifiex_adapter *adapter, struct semaphore *sem) +int mwifiex_remove_card(struct mwifiex_adapter *adapter) { struct mwifiex_private *priv = NULL; int i; - if (down_trylock(sem)) - goto exit_sem_err; - if (!adapter) goto exit_remove; @@ -1800,8 +1789,6 @@ int mwifiex_remove_card(struct mwifiex_adapter *adapter, struct semaphore *sem) mwifiex_free_adapter(adapter); exit_remove: - up(sem); -exit_sem_err: return 0; } EXPORT_SYMBOL_GPL(mwifiex_remove_card); diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h index 904a2ed..cf4c780 100644 --- a/drivers/net/wireless/marvell/mwifiex/main.h +++ b/drivers/net/wireless/marvell/mwifiex/main.h @@ -20,6 +20,7 @@ #ifndef _MWIFIEX_MAIN_H_ #define _MWIFIEX_MAIN_H_ +#include #include #include #include @@ -985,7 +986,10 @@ struct mwifiex_adapter { u32 usr_dot_11ac_mcs_support; atomic_t pending_bridged_pkts; - struct semaphore *card_sem; + + /* For synchronizing FW initialization with device lifecycle. */ + struct completion *fw_done; + bool ext_scan; u8 fw_api_ver; u8 key_api_major_ver, key_api_minor_ver; @@ -1438,10 +1442,11 @@ static inline void mwifiex_enable_wake(struct mwifiex_adapter *adapter) int mwifiex_init_shutdown_fw(struct mwifiex_private *priv, u32 func_init_shutdown); -int mwifiex_add_card(void *card, struct semaphore *sem, + +int mwifiex_add_card(void *card, struct completion *fw_done, struct mwifiex_if_ops *if_ops, u8 iface_type, struct device *dev); -int mwifiex_remove_card(struct mwifiex_adapter *, struct semaphore *); +int mwifiex_remove_card(struct mwifiex_adapter *adapter); void mwifiex_get_version(struct mwifiex_adapter *adapter, char *version, int maxlen); diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c index cfb45ef..547806f 100644 --- a/drivers/net/wireless/marvell/mwifiex/pcie.c +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c @@ -35,8 +35,6 @@ static struct mwifiex_if_ops pcie_ops; -static struct semaphore add_remove_card_sem; - static const struct of_device_id mwifiex_pcie_of_match_table[] = { { .compatible = "pci11ab,2b42" }, { .compatible = "pci1b4b,2b42" }, @@ -212,6 +210,8 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev, if (!card) return -ENOMEM; + init_completion(&card->fw_done); + card->dev = pdev; if (ent->driver_data) { @@ -232,7 +232,7 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev, return ret; } - if (mwifiex_add_card(card, &add_remove_card_sem, &pcie_ops, + if (mwifiex_add_card(card, &card->fw_done, &pcie_ops, MWIFIEX_PCIE, &pdev->dev)) { pr_err("%s failed\n", __func__); return -1; @@ -254,6 +254,8 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev) if (!card) return; + wait_for_completion(&card->fw_done); + adapter = card->adapter; if (!adapter || !adapter->priv_num) return; @@ -273,7 +275,7 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev) mwifiex_init_shutdown_fw(priv, MWIFIEX_FUNC_SHUTDOWN); } - mwifiex_remove_card(card->adapter, &add_remove_card_sem); + mwifiex_remove_card(adapter); } static void mwifiex_pcie_shutdown(struct pci_dev *pdev) @@ -3175,8 +3177,7 @@ static void mwifiex_pcie_down_dev(struct mwifiex_adapter *adapter) /* * This function initializes the PCIE driver module. * - * This initiates the semaphore and registers the device with - * PCIE bus. + * This registers the device with PCIE bus. */ static int mwifiex_pcie_init_module(void) { @@ -3184,8 +3185,6 @@ static int mwifiex_pcie_init_module(void) pr_debug("Marvell PCIe Driver\n"); - sema_init(&add_remove_card_sem, 1); - /* Clear the flag in case user removes the card. */ user_rmmod = 0; @@ -3209,9 +3208,6 @@ static int mwifiex_pcie_init_module(void) */ static void mwifiex_pcie_cleanup_module(void) { - if (!down_interruptible(&add_remove_card_sem)) - up(&add_remove_card_sem); - /* Set the flag as user is removing this module. */ user_rmmod = 1; diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.h b/drivers/net/wireless/marvell/mwifiex/pcie.h index 46f99ca..ae3365d 100644 --- a/drivers/net/wireless/marvell/mwifiex/pcie.h +++ b/drivers/net/wireless/marvell/mwifiex/pcie.h @@ -22,6 +22,7 @@ #ifndef _MWIFIEX_PCIE_H #define _MWIFIEX_PCIE_H +#include #include #include @@ -345,6 +346,7 @@ struct pcie_service_card { struct pci_dev *dev; struct mwifiex_adapter *adapter; struct mwifiex_pcie_device pcie; + struct completion fw_done; u8 txbd_flush; u32 txbd_wrptr; diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c index f410cf5..5312ffb 100644 --- a/drivers/net/wireless/marvell/mwifiex/sdio.c +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c @@ -49,8 +49,6 @@ static struct mwifiex_if_ops sdio_ops; static unsigned long iface_work_flags; -static struct semaphore add_remove_card_sem; - static struct memory_type_mapping generic_mem_type_map[] = { {"DUMP", NULL, 0, 0xDD}, }; @@ -115,6 +113,8 @@ static int mwifiex_sdio_probe_of(struct device *dev) if (!card) return -ENOMEM; + init_completion(&card->fw_done); + card->func = func; card->device_id = id; @@ -154,7 +154,7 @@ static int mwifiex_sdio_probe_of(struct device *dev) goto err_disable; } - ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops, + ret = mwifiex_add_card(card, &card->fw_done, &sdio_ops, MWIFIEX_SDIO, &func->dev); if (ret) { dev_err(&func->dev, "add card failed\n"); @@ -235,6 +235,8 @@ static int mwifiex_sdio_resume(struct device *dev) if (!card) return; + wait_for_completion(&card->fw_done); + adapter = card->adapter; if (!adapter || !adapter->priv_num) return; @@ -252,7 +254,7 @@ static int mwifiex_sdio_resume(struct device *dev) mwifiex_init_shutdown_fw(priv, MWIFIEX_FUNC_SHUTDOWN); } - mwifiex_remove_card(card->adapter, &add_remove_card_sem); + mwifiex_remove_card(adapter); } /* @@ -2714,14 +2716,11 @@ static void mwifiex_sdio_device_dump(struct mwifiex_adapter *adapter) /* * This function initializes the SDIO driver. * - * This initiates the semaphore and registers the device with - * SDIO bus. + * This registers the device with SDIO bus. */ static int mwifiex_sdio_init_module(void) { - sema_init(&add_remove_card_sem, 1); - /* Clear the flag in case user removes the card. */ user_rmmod = 0; @@ -2740,9 +2739,6 @@ static void mwifiex_sdio_device_dump(struct mwifiex_adapter *adapter) static void mwifiex_sdio_cleanup_module(void) { - if (!down_interruptible(&add_remove_card_sem)) - up(&add_remove_card_sem); - /* Set the flag as user is removing this module. */ user_rmmod = 1; cancel_work_sync(&sdio_work); diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.h b/drivers/net/wireless/marvell/mwifiex/sdio.h index b9fbc5c..cdbf3a3a 100644 --- a/drivers/net/wireless/marvell/mwifiex/sdio.h +++ b/drivers/net/wireless/marvell/mwifiex/sdio.h @@ -21,6 +21,7 @@ #define _MWIFIEX_SDIO_H +#include #include #include #include @@ -238,6 +239,7 @@ struct sdio_mmc_card { struct sdio_func *func; struct mwifiex_adapter *adapter; + struct completion fw_done; const char *firmware; const struct mwifiex_sdio_card_reg *reg; u8 max_ports; diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c index 9cc2a0c0..63a755c 100644 --- a/drivers/net/wireless/marvell/mwifiex/usb.c +++ b/drivers/net/wireless/marvell/mwifiex/usb.c @@ -24,7 +24,6 @@ static u8 user_rmmod; static struct mwifiex_if_ops usb_ops; -static struct semaphore add_remove_card_sem; static struct usb_device_id mwifiex_usb_table[] = { /* 8766 */ @@ -386,6 +385,8 @@ static int mwifiex_usb_probe(struct usb_interface *intf, if (!card) return -ENOMEM; + init_completion(&card->fw_done); + id_vendor = le16_to_cpu(udev->descriptor.idVendor); id_product = le16_to_cpu(udev->descriptor.idProduct); bcd_device = le16_to_cpu(udev->descriptor.bcdDevice); @@ -475,7 +476,7 @@ static int mwifiex_usb_probe(struct usb_interface *intf, usb_set_intfdata(intf, card); - ret = mwifiex_add_card(card, &add_remove_card_sem, &usb_ops, + ret = mwifiex_add_card(card, &card->fw_done, &usb_ops, MWIFIEX_USB, &card->udev->dev); if (ret) { pr_err("%s: mwifiex_add_card failed: %d\n", __func__, ret); @@ -601,13 +602,15 @@ static void mwifiex_usb_disconnect(struct usb_interface *intf) struct usb_card_rec *card = usb_get_intfdata(intf); struct mwifiex_adapter *adapter; - if (!card || !card->adapter) { - pr_err("%s: card or card->adapter is NULL\n", __func__); + if (!card) { + dev_err(&intf->dev, "%s: card is NULL\n", __func__); return; } + wait_for_completion(&card->fw_done); + adapter = card->adapter; - if (!adapter->priv_num) + if (!adapter || !adapter->priv_num) return; if (user_rmmod && !adapter->mfg_mode) { @@ -627,7 +630,7 @@ static void mwifiex_usb_disconnect(struct usb_interface *intf) mwifiex_dbg(adapter, FATAL, "%s: removing card\n", __func__); - mwifiex_remove_card(adapter, &add_remove_card_sem); + mwifiex_remove_card(adapter); usb_put_dev(interface_to_usbdev(intf)); } @@ -1200,8 +1203,7 @@ static void mwifiex_usb_submit_rem_rx_urbs(struct mwifiex_adapter *adapter) /* This function initializes the USB driver module. * - * This initiates the semaphore and registers the device with - * USB bus. + * This registers the device with USB bus. */ static int mwifiex_usb_init_module(void) { @@ -1209,8 +1211,6 @@ static int mwifiex_usb_init_module(void) pr_debug("Marvell USB8797 Driver\n"); - sema_init(&add_remove_card_sem, 1); - ret = usb_register(&mwifiex_usb_driver); if (ret) pr_err("Driver register failed!\n"); @@ -1230,9 +1230,6 @@ static int mwifiex_usb_init_module(void) */ static void mwifiex_usb_cleanup_module(void) { - if (!down_interruptible(&add_remove_card_sem)) - up(&add_remove_card_sem); - /* set the flag as user is removing this module */ user_rmmod = 1; diff --git a/drivers/net/wireless/marvell/mwifiex/usb.h b/drivers/net/wireless/marvell/mwifiex/usb.h index 30e8eb8..e5f204e 100644 --- a/drivers/net/wireless/marvell/mwifiex/usb.h +++ b/drivers/net/wireless/marvell/mwifiex/usb.h @@ -20,6 +20,7 @@ #ifndef _MWIFIEX_USB_H #define _MWIFIEX_USB_H +#include #include #define USB8XXX_VID 0x1286 @@ -75,6 +76,7 @@ struct usb_card_rec { struct mwifiex_adapter *adapter; struct usb_device *udev; struct usb_interface *intf; + struct completion fw_done; u8 rx_cmd_ep; struct urb_context rx_cmd; atomic_t rx_cmd_urb_pending; -- 1.9.1