Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 4a79aa17 authored by Brian Norris's avatar Brian Norris Committed by Kalle Valo
Browse files

mwifiex: resolve races between async FW init (failure) and device removal 



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: default avatarBrian Norris <briannorris@chromium.org>
Signed-off-by: default avatarKalle Valo <kvalo@codeaurora.org>
parent 67120768

drivers/net/wireless/marvell/mwifiex/main.c

+17 −30
Original line number Original line Diff line number Diff line
@@ -521,9 +521,9 @@ static void mwifiex_fw_dpc(const struct firmware *firmware, void *context) 
	struct mwifiex_private *priv;

	struct mwifiex_private *priv;

	struct mwifiex_adapter *adapter = context;

	struct mwifiex_adapter *adapter = context;

	struct mwifiex_fw_image fw;

	struct mwifiex_fw_image fw;

	struct semaphore *sem = adapter->card_sem;

	bool init_failed = false;

	bool init_failed = false;

	struct wireless_dev *wdev;

	struct wireless_dev *wdev;

	struct completion *fw_done = adapter->fw_done;





	if (!firmware) {

	if (!firmware) {

		mwifiex_dbg(adapter, ERROR,

		mwifiex_dbg(adapter, ERROR,
@@ -670,7 +670,8 @@ static void mwifiex_fw_dpc(const struct firmware *firmware, void *context) 
	}

	}

	if (init_failed)

	if (init_failed)

		mwifiex_free_adapter(adapter);

		mwifiex_free_adapter(adapter);

	up(sem);

	/* Tell all current and future waiters we're finished */

	complete_all(fw_done);

	return;

	return;

}

}
@@ -1365,7 +1366,7 @@ static void mwifiex_main_work_queue(struct work_struct *work) 
 * code is extracted from mwifiex_remove_card()

 * code is extracted from mwifiex_remove_card()

 */

 */

static int

static int

mwifiex_shutdown_sw(struct mwifiex_adapter *adapter, struct semaphore *sem)

mwifiex_shutdown_sw(struct mwifiex_adapter *adapter)

{

{

	struct mwifiex_private *priv;

	struct mwifiex_private *priv;

	int i;

	int i;
@@ -1373,8 +1374,9 @@ mwifiex_shutdown_sw(struct mwifiex_adapter *adapter, struct semaphore *sem) 
	if (!adapter)

	if (!adapter)

		goto exit_return;

		goto exit_return;





	if (down_interruptible(sem))

	wait_for_completion(adapter->fw_done);

		goto exit_sem_err;

	/* Caller should ensure we aren't suspending while this happens */

	reinit_completion(adapter->fw_done);





	priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);

	priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);

	mwifiex_deauthenticate(priv, NULL);

	mwifiex_deauthenticate(priv, NULL);
@@ -1431,8 +1433,6 @@ mwifiex_shutdown_sw(struct mwifiex_adapter *adapter, struct semaphore *sem) 
		rtnl_unlock();

		rtnl_unlock();

	}

	}





	up(sem);

exit_sem_err:

	mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);

	mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);

exit_return:

exit_return:

	return 0;

	return 0;
@@ -1442,21 +1442,18 @@ mwifiex_shutdown_sw(struct mwifiex_adapter *adapter, struct semaphore *sem) 
 * code is extracted from mwifiex_add_card()

 * code is extracted from mwifiex_add_card()

 */

 */

static int

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)

		  struct mwifiex_if_ops *if_ops, u8 iface_type)

{

{

	char fw_name[32];

	char fw_name[32];

	struct pcie_service_card *card = adapter->card;

	struct pcie_service_card *card = adapter->card;





	if (down_interruptible(sem))

		goto exit_sem_err;



	mwifiex_init_lock_list(adapter);

	mwifiex_init_lock_list(adapter);

	if (adapter->if_ops.up_dev)

	if (adapter->if_ops.up_dev)

		adapter->if_ops.up_dev(adapter);

		adapter->if_ops.up_dev(adapter);





	adapter->iface_type = iface_type;

	adapter->iface_type = iface_type;

	adapter->card_sem = sem;

	adapter->fw_done = fw_done;





	adapter->hw_status = MWIFIEX_HW_STATUS_INITIALIZING;

	adapter->hw_status = MWIFIEX_HW_STATUS_INITIALIZING;

	adapter->surprise_removed = false;

	adapter->surprise_removed = false;
@@ -1507,7 +1504,8 @@ mwifiex_reinit_sw(struct mwifiex_adapter *adapter, struct semaphore *sem, 
	}

	}

	strcpy(adapter->fw_name, fw_name);

	strcpy(adapter->fw_name, fw_name);

	mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);

	mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);

	up(sem);



	complete_all(adapter->fw_done);

	return 0;

	return 0;





err_init_fw:

err_init_fw:
@@ -1527,8 +1525,7 @@ mwifiex_reinit_sw(struct mwifiex_adapter *adapter, struct semaphore *sem, 
err_kmalloc:

err_kmalloc:

	mwifiex_terminate_workqueue(adapter);

	mwifiex_terminate_workqueue(adapter);

	adapter->surprise_removed = true;

	adapter->surprise_removed = true;

	up(sem);

	complete_all(adapter->fw_done);

exit_sem_err:

	mwifiex_dbg(adapter, INFO, "%s, error\n", __func__);

	mwifiex_dbg(adapter, INFO, "%s, error\n", __func__);





	return -1;

	return -1;
@@ -1543,12 +1540,12 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare) 
	struct mwifiex_if_ops if_ops;

	struct mwifiex_if_ops if_ops;





	if (!prepare) {

	if (!prepare) {

		mwifiex_reinit_sw(adapter, adapter->card_sem, &if_ops,

		mwifiex_reinit_sw(adapter, adapter->fw_done, &if_ops,

				  adapter->iface_type);

				  adapter->iface_type);

	} else {

	} else {

		memcpy(&if_ops, &adapter->if_ops,

		memcpy(&if_ops, &adapter->if_ops,

		       sizeof(struct mwifiex_if_ops));

		       sizeof(struct mwifiex_if_ops));

		mwifiex_shutdown_sw(adapter, adapter->card_sem);

		mwifiex_shutdown_sw(adapter);

	}

	}

}

}

EXPORT_SYMBOL_GPL(mwifiex_do_flr);

EXPORT_SYMBOL_GPL(mwifiex_do_flr);
@@ -1618,15 +1615,12 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter) 
 *      - Add logical interfaces

 *      - Add logical interfaces

 */

 */

int

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 mwifiex_if_ops *if_ops, u8 iface_type,

		 struct device *dev)

		 struct device *dev)

{

{

	struct mwifiex_adapter *adapter;

	struct mwifiex_adapter *adapter;





	if (down_interruptible(sem))

		goto exit_sem_err;



	if (mwifiex_register(card, if_ops, (void **)&adapter)) {

	if (mwifiex_register(card, if_ops, (void **)&adapter)) {

		pr_err("%s: software init failed\n", __func__);

		pr_err("%s: software init failed\n", __func__);

		goto err_init_sw;

		goto err_init_sw;
@@ -1636,7 +1630,7 @@ mwifiex_add_card(void *card, struct semaphore *sem, 
	mwifiex_probe_of(adapter);

	mwifiex_probe_of(adapter);





	adapter->iface_type = iface_type;

	adapter->iface_type = iface_type;

	adapter->card_sem = sem;

	adapter->fw_done = fw_done;





	adapter->hw_status = MWIFIEX_HW_STATUS_INITIALIZING;

	adapter->hw_status = MWIFIEX_HW_STATUS_INITIALIZING;

	adapter->surprise_removed = false;

	adapter->surprise_removed = false;
@@ -1705,9 +1699,7 @@ mwifiex_add_card(void *card, struct semaphore *sem, 
	mwifiex_free_adapter(adapter);

	mwifiex_free_adapter(adapter);





err_init_sw:

err_init_sw:

	up(sem);





exit_sem_err:

	return -1;

	return -1;

}

}

EXPORT_SYMBOL_GPL(mwifiex_add_card);

EXPORT_SYMBOL_GPL(mwifiex_add_card);
@@ -1723,14 +1715,11 @@ EXPORT_SYMBOL_GPL(mwifiex_add_card); 
 *      - Unregister the device

 *      - Unregister the device

 *      - Free the adapter structure

 *      - 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;

	struct mwifiex_private *priv = NULL;

	int i;

	int i;





	if (down_trylock(sem))

		goto exit_sem_err;



	if (!adapter)

	if (!adapter)

		goto exit_remove;

		goto exit_remove;
@@ -1800,8 +1789,6 @@ int mwifiex_remove_card(struct mwifiex_adapter *adapter, struct semaphore *sem) 
	mwifiex_free_adapter(adapter);

	mwifiex_free_adapter(adapter);





exit_remove:

exit_remove:

	up(sem);

exit_sem_err:

	return 0;

	return 0;

}

}

EXPORT_SYMBOL_GPL(mwifiex_remove_card);

EXPORT_SYMBOL_GPL(mwifiex_remove_card);

drivers/net/wireless/marvell/mwifiex/main.h

+8 −3
Original line number Original line Diff line number Diff line
@@ -20,6 +20,7 @@ 
#ifndef _MWIFIEX_MAIN_H_

#ifndef _MWIFIEX_MAIN_H_

#define _MWIFIEX_MAIN_H_

#define _MWIFIEX_MAIN_H_





#include <linux/completion.h>

#include <linux/kernel.h>

#include <linux/kernel.h>

#include <linux/module.h>

#include <linux/module.h>

#include <linux/sched.h>

#include <linux/sched.h>
@@ -985,7 +986,10 @@ struct mwifiex_adapter { 
	u32 usr_dot_11ac_mcs_support;

	u32 usr_dot_11ac_mcs_support;





	atomic_t pending_bridged_pkts;

	atomic_t pending_bridged_pkts;

	struct semaphore *card_sem;



	/* For synchronizing FW initialization with device lifecycle. */

	struct completion *fw_done;



	bool ext_scan;

	bool ext_scan;

	u8 fw_api_ver;

	u8 fw_api_ver;

	u8 key_api_major_ver, key_api_minor_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,

int mwifiex_init_shutdown_fw(struct mwifiex_private *priv,

			     u32 func_init_shutdown);

			     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 mwifiex_if_ops *if_ops, u8 iface_type,

		     struct device *dev);

		     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,

void mwifiex_get_version(struct mwifiex_adapter *adapter, char *version,

			 int maxlen);

			 int maxlen);

drivers/net/wireless/marvell/mwifiex/pcie.c

+7 −11
Original line number Original line Diff line number Diff line
@@ -35,8 +35,6 @@ static u8 user_rmmod; 




static struct mwifiex_if_ops pcie_ops;

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[] = {

static const struct of_device_id mwifiex_pcie_of_match_table[] = {

	{ .compatible = "pci11ab,2b42" },

	{ .compatible = "pci11ab,2b42" },

	{ .compatible = "pci1b4b,2b42" },

	{ .compatible = "pci1b4b,2b42" },
@@ -212,6 +210,8 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev, 
	if (!card)

	if (!card)

		return -ENOMEM;

		return -ENOMEM;





	init_completion(&card->fw_done);



	card->dev = pdev;

	card->dev = pdev;





	if (ent->driver_data) {

	if (ent->driver_data) {
@@ -232,7 +232,7 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev, 
			return ret;

			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)) {

			     MWIFIEX_PCIE, &pdev->dev)) {

		pr_err("%s failed\n", __func__);

		pr_err("%s failed\n", __func__);

		return -1;

		return -1;
@@ -254,6 +254,8 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev) 
	if (!card)

	if (!card)

		return;

		return;





	wait_for_completion(&card->fw_done);



	adapter = card->adapter;

	adapter = card->adapter;

	if (!adapter || !adapter->priv_num)

	if (!adapter || !adapter->priv_num)

		return;

		return;
@@ -273,7 +275,7 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev) 
		mwifiex_init_shutdown_fw(priv, MWIFIEX_FUNC_SHUTDOWN);

		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)

static void mwifiex_pcie_shutdown(struct pci_dev *pdev)
@@ -3175,8 +3177,7 @@ static struct mwifiex_if_ops pcie_ops = { 
/*

/*

 * This function initializes the PCIE driver module.

 * This function initializes the PCIE driver module.

 *

 *

 * This initiates the semaphore and registers the device with

 * This registers the device with PCIE bus.

 * PCIE bus.

 */

 */

static int mwifiex_pcie_init_module(void)

static int mwifiex_pcie_init_module(void)

{

{
@@ -3184,8 +3185,6 @@ static int mwifiex_pcie_init_module(void) 




	pr_debug("Marvell PCIe Driver\n");

	pr_debug("Marvell PCIe Driver\n");





	sema_init(&add_remove_card_sem, 1);



	/* Clear the flag in case user removes the card. */

	/* Clear the flag in case user removes the card. */

	user_rmmod = 0;

	user_rmmod = 0;
@@ -3209,9 +3208,6 @@ static int mwifiex_pcie_init_module(void) 
 */

 */

static void mwifiex_pcie_cleanup_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. */

	/* Set the flag as user is removing this module. */

	user_rmmod = 1;

	user_rmmod = 1;

drivers/net/wireless/marvell/mwifiex/pcie.h

+2 −0
Original line number Original line Diff line number Diff line
@@ -22,6 +22,7 @@ 
#ifndef	_MWIFIEX_PCIE_H

#ifndef	_MWIFIEX_PCIE_H

#define	_MWIFIEX_PCIE_H

#define	_MWIFIEX_PCIE_H





#include    <linux/completion.h>

#include    <linux/pci.h>

#include    <linux/pci.h>

#include    <linux/interrupt.h>

#include    <linux/interrupt.h>
@@ -345,6 +346,7 @@ struct pcie_service_card { 
	struct pci_dev *dev;

	struct pci_dev *dev;

	struct mwifiex_adapter *adapter;

	struct mwifiex_adapter *adapter;

	struct mwifiex_pcie_device pcie;

	struct mwifiex_pcie_device pcie;

	struct completion fw_done;





	u8 txbd_flush;

	u8 txbd_flush;

	u32 txbd_wrptr;

	u32 txbd_wrptr;

drivers/net/wireless/marvell/mwifiex/sdio.c

+7 −11
Original line number Original line Diff line number Diff line
@@ -49,8 +49,6 @@ static u8 user_rmmod; 
static struct mwifiex_if_ops sdio_ops;

static struct mwifiex_if_ops sdio_ops;

static unsigned long iface_work_flags;

static unsigned long iface_work_flags;





static struct semaphore add_remove_card_sem;



static struct memory_type_mapping generic_mem_type_map[] = {

static struct memory_type_mapping generic_mem_type_map[] = {

	{"DUMP", NULL, 0, 0xDD},

	{"DUMP", NULL, 0, 0xDD},

};

};
@@ -115,6 +113,8 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id) 
	if (!card)

	if (!card)

		return -ENOMEM;

		return -ENOMEM;





	init_completion(&card->fw_done);



	card->func = func;

	card->func = func;

	card->device_id = id;

	card->device_id = id;
@@ -154,7 +154,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id) 
			goto err_disable;

			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);

			       MWIFIEX_SDIO, &func->dev);

	if (ret) {

	if (ret) {

		dev_err(&func->dev, "add card failed\n");

		dev_err(&func->dev, "add card failed\n");
@@ -235,6 +235,8 @@ mwifiex_sdio_remove(struct sdio_func *func) 
	if (!card)

	if (!card)

		return;

		return;





	wait_for_completion(&card->fw_done);



	adapter = card->adapter;

	adapter = card->adapter;

	if (!adapter || !adapter->priv_num)

	if (!adapter || !adapter->priv_num)

		return;

		return;
@@ -252,7 +254,7 @@ mwifiex_sdio_remove(struct sdio_func *func) 
		mwifiex_init_shutdown_fw(priv, MWIFIEX_FUNC_SHUTDOWN);

		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 struct mwifiex_if_ops sdio_ops = { 
/*

/*

 * This function initializes the SDIO driver.

 * This function initializes the SDIO driver.

 *

 *

 * This initiates the semaphore and registers the device with

 * This registers the device with SDIO bus.

 * SDIO bus.

 */

 */

static int

static int

mwifiex_sdio_init_module(void)

mwifiex_sdio_init_module(void)

{

{

	sema_init(&add_remove_card_sem, 1);



	/* Clear the flag in case user removes the card. */

	/* Clear the flag in case user removes the card. */

	user_rmmod = 0;

	user_rmmod = 0;
@@ -2740,9 +2739,6 @@ mwifiex_sdio_init_module(void) 
static void

static void

mwifiex_sdio_cleanup_module(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. */

	/* Set the flag as user is removing this module. */

	user_rmmod = 1;

	user_rmmod = 1;

	cancel_work_sync(&sdio_work);

	cancel_work_sync(&sdio_work);