[PATCH 1/4] mwifiex: pcie: fix cmd_buf use-after-free in remove/reset

[PATCH 1/4] mwifiex: pcie: fix cmd_buf use-after-free in remove/reset

[Brian Norris]

Command buffers (skb's) are allocated by the main driver, and freed upon
the last use. That last use is often in mwifiex_free_cmd_buffer(). In
the meantime, if the command buffer gets used by the PCI driver, we map
it as DMA-able, and store the mapping information in the 'cb' memory.

However, if a command was in-flight when resetting the device (and
therefore was still mapped), we don't get a chance to unmap this memory
until after the core has cleaned up its command handling.

Let's keep a refcount within the PCI driver, so we ensure the memory
only gets freed after we've finished unmapping it.

Noticed by KASAN when forcing a reset via:

  echo 1 > /sys/bus/pci/.../reset

The same code path can presumably be exercised in remove() and
shutdown().

[  205.390377] mwifiex_pcie 0000:01:00.0: info: shutdown mwifiex...
[  205.400393] ==================================================================
[  205.407719] BUG: KASAN: use-after-free in mwifiex_unmap_pci_memory.isra.14+0x4c/0x100 [mwifiex_pcie] at addr ffffffc0ad471b28
[  205.419040] Read of size 16 by task bash/1913
[  205.423421] =============================================================================
[  205.431625] BUG skbuff_head_cache (Tainted: G    B          ): kasan: bad access detected
[  205.439815] -----------------------------------------------------------------------------
[  205.439815]
[  205.449534] INFO: Allocated in __build_skb+0x48/0x114 age=1311 cpu=4 pid=1913
[  205.456709] 	alloc_debug_processing+0x124/0x178
[  205.461282] 	___slab_alloc.constprop.58+0x528/0x608
[  205.466196] 	__slab_alloc.isra.54.constprop.57+0x44/0x54
[  205.471542] 	kmem_cache_alloc+0xcc/0x278
[  205.475497] 	__build_skb+0x48/0x114
[  205.479019] 	__netdev_alloc_skb+0xe0/0x170
[  205.483244] 	mwifiex_alloc_cmd_buffer+0x68/0xdc [mwifiex]
[  205.488759] 	mwifiex_init_fw+0x40/0x6cc [mwifiex]
[  205.493584] 	_mwifiex_fw_dpc+0x158/0x520 [mwifiex]
[  205.498491] 	mwifiex_reinit_sw+0x2c4/0x398 [mwifiex]
[  205.503510] 	mwifiex_pcie_reset_notify+0x114/0x15c [mwifiex_pcie]
[  205.509643] 	pci_reset_notify+0x5c/0x6c
[  205.513519] 	pci_reset_function+0x6c/0x7c
[  205.517567] 	reset_store+0x68/0x98
[  205.521003] 	dev_attr_store+0x54/0x60
[  205.524705] 	sysfs_kf_write+0x9c/0xb0
[  205.528413] INFO: Freed in __kfree_skb+0xb0/0xbc age=131 cpu=4 pid=1913
[  205.535064] 	free_debug_processing+0x264/0x370
[  205.539550] 	__slab_free+0x84/0x40c
[  205.543075] 	kmem_cache_free+0x1c8/0x2a0
[  205.547030] 	__kfree_skb+0xb0/0xbc
[  205.550465] 	consume_skb+0x164/0x178
[  205.554079] 	__dev_kfree_skb_any+0x58/0x64
[  205.558304] 	mwifiex_free_cmd_buffer+0xa0/0x158 [mwifiex]
[  205.563817] 	mwifiex_shutdown_drv+0x578/0x5c4 [mwifiex]
[  205.569164] 	mwifiex_shutdown_sw+0x178/0x310 [mwifiex]
[  205.574353] 	mwifiex_pcie_reset_notify+0xd4/0x15c [mwifiex_pcie]
[  205.580398] 	pci_reset_notify+0x5c/0x6c
[  205.584274] 	pci_dev_save_and_disable+0x24/0x6c
[  205.588837] 	pci_reset_function+0x30/0x7c
[  205.592885] 	reset_store+0x68/0x98
[  205.596324] 	dev_attr_store+0x54/0x60
[  205.600017] 	sysfs_kf_write+0x9c/0xb0
...
[  205.800488] Call trace:
[  205.802980] [<ffffffc00020a69c>] dump_backtrace+0x0/0x190
[  205.808415] [<ffffffc00020a96c>] show_stack+0x20/0x28
[  205.813506] [<ffffffc0005d020c>] dump_stack+0xa4/0xcc
[  205.818598] [<ffffffc0003be44c>] print_trailer+0x158/0x168
[  205.824120] [<ffffffc0003be5f0>] object_err+0x4c/0x5c
[  205.829210] [<ffffffc0003c45bc>] kasan_report+0x334/0x500
[  205.834641] [<ffffffc0003c3994>] check_memory_region+0x20/0x14c
[  205.840593] [<ffffffc0003c3b14>] __asan_loadN+0x14/0x1c
[  205.845879] [<ffffffbffc46171c>] mwifiex_unmap_pci_memory.isra.14+0x4c/0x100 [mwifiex_pcie]
[  205.854282] [<ffffffbffc461864>] mwifiex_pcie_delete_cmdrsp_buf+0x94/0xa8 [mwifiex_pcie]
[  205.862421] [<ffffffbffc462028>] mwifiex_pcie_free_buffers+0x11c/0x158 [mwifiex_pcie]
[  205.870302] [<ffffffbffc4620d4>] mwifiex_pcie_down_dev+0x70/0x80 [mwifiex_pcie]
[  205.877736] [<ffffffbffc1397a8>] mwifiex_shutdown_sw+0x190/0x310 [mwifiex]
[  205.884658] [<ffffffbffc4606b4>] mwifiex_pcie_reset_notify+0xd4/0x15c [mwifiex_pcie]
[  205.892446] [<ffffffc000635f54>] pci_reset_notify+0x5c/0x6c
[  205.898048] [<ffffffc00063a044>] pci_dev_save_and_disable+0x24/0x6c
[  205.904350] [<ffffffc00063cf0c>] pci_reset_function+0x30/0x7c
[  205.910134] [<ffffffc000641118>] reset_store+0x68/0x98
[  205.915312] [<ffffffc000771588>] dev_attr_store+0x54/0x60
[  205.920750] [<ffffffc00046f53c>] sysfs_kf_write+0x9c/0xb0
[  205.926182] [<ffffffc00046dfb0>] kernfs_fop_write+0x184/0x1f8
[  205.931963] [<ffffffc0003d64f4>] __vfs_write+0x6c/0x17c
[  205.937221] [<ffffffc0003d7164>] vfs_write+0xf0/0x1c4
[  205.942310] [<ffffffc0003d7da0>] SyS_write+0x78/0xd8
[  205.947312] [<ffffffc000204634>] el0_svc_naked+0x24/0x28
...
[  205.998268] ==================================================================

This bug has been around in different forms for a while. It was sort of
noticed in commit 955ab095c51a ("mwifiex: Do not kfree cmd buf while
unregistering PCIe"), but it just fixed the double-free, without
acknowledging the potential for use-after-free.

Fixes: fc3314609047 ("mwifiex: use pci_alloc/free_consistent APIs for PCIe")
Cc: <stable@vger.kernel.org>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 92ffb1ba17e6..99e8a5cfda1b 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -1035,6 +1035,7 @@ static int mwifiex_pcie_delete_cmdrsp_buf(struct mwifiex_adapter *adapter)
 	if (card && card->cmd_buf) {
 		mwifiex_unmap_pci_memory(adapter, card->cmd_buf,
 					 PCI_DMA_TODEVICE);
+		dev_kfree_skb_any(card->cmd_buf);
 	}
 	return 0;
 }
@@ -1601,6 +1602,11 @@ mwifiex_pcie_send_cmd(struct mwifiex_adapter *adapter, struct sk_buff *skb)
 		return -1;
 
 	card->cmd_buf = skb;
+	/*
+	 * Need to keep a reference, since core driver might free up this
+	 * buffer before we've unmapped it.
+	 */
+	skb_get(skb);
 
 	/* To send a command, the driver will:
 		1. Write the 64bit physical address of the data buffer to
@@ -1703,6 +1709,7 @@ static int mwifiex_pcie_process_cmd_complete(struct mwifiex_adapter *adapter)
 	if (card->cmd_buf) {
 		mwifiex_unmap_pci_memory(adapter, card->cmd_buf,
 					 PCI_DMA_TODEVICE);
+		dev_kfree_skb_any(card->cmd_buf);
 		card->cmd_buf = NULL;
 	}
 
-- 
2.12.2.762.g0e3151a226-goog

[PATCH 2/4] mwifiex: reset timeout flag when resetting device

[Brian Norris]

If we reset because of a command timeout, we should reset this flag.
Otherwise, we might erroneously think the next command after reset is
timing out, and trigger another reset.

The above behavior effectively neuters the automatic card_reset()
behavior, as it means we will never recover from a command timeout
properly (and in fact, we might enter an infinite loop:

  timeout -> reset -> (fake) timeout -> reset -> ...

This fixes a bug introduced with introduction of PCIe function level
reset support, but it was carried into the SDIO driver when it was
converted to use the same codepaths. And this is currently mostly a
problem only in the SDIO driver, because it's the only one with
automatic card_reset() support (e.g., on command timeout). But it will
be a problem for PCIe too, as I'm working on supporting automatic
card_reset() for PCIe.

Fixes: c742e623e941 ("mwifiex: sdio card reset enhancement")
Fixes: 4c5dae59d2e9 ("mwifiex: add PCIe function level reset support")
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/net/wireless/marvell/mwifiex/main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 912b687f4671..89eccd7adbe5 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1441,6 +1441,7 @@ mwifiex_reinit_sw(struct mwifiex_adapter *adapter)
 	init_waitqueue_head(&adapter->init_wait_q);
 	adapter->is_suspended = false;
 	adapter->hs_activated = false;
+	adapter->is_cmd_timedout = 0;
 	init_waitqueue_head(&adapter->hs_activate_wait_q);
 	init_waitqueue_head(&adapter->cmd_wait_q.wait);
 	adapter->cmd_wait_q.status = 0;
-- 
2.12.2.762.g0e3151a226-goog

[PATCH 3/4] mwifiex: pcie: clear outstanding work when resetting

[Brian Norris]

When we shut down the device (i.e., during 'reset'), we cancel any
outstanding work, but we don't clear any work-related flags. This can
cause problems if, e.g., we begin to queue a new firmware dump or card
reset while the other one is in progress. That might leave work_flags
with a stale value, and we might begin one of these *after* we've
completely reset the device. That doesn't make sense, because all
firmware context will have been lost by then.

This fixes some forms of cascading failures, where I:

(a) force a firmware dump (cat /sys/kernel/debug/mwifiex/mlan0/device_dump)
(b) run a Wifi scan in parallel (iw mlan0 scan)
(c) the scan times out due to (a) hogging the interface
(d) the command timeout triggers another firmware dump and a reset [*]
(e) the 2nd firmware dump flag persists across the reset
(f) as soon as the interface comes back up, we trigger the pending
    firmware dump
(g) subsequent commands time out again, while we are processing the
    firmware dump; return to (d)

[*] Note that automatic card_reset() support is not yet implemented for
the mwifiex PCIe driver, so we won't hit *exactly* this behavior yet.
But we can see similarly-confusing behaviors today.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
This might qualify as 4.11 material (bugfix), though it's probably not too
widely triggered yet.

 drivers/net/wireless/marvell/mwifiex/pcie.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 99e8a5cfda1b..bacac2949f10 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -371,6 +371,8 @@ static void mwifiex_pcie_reset_notify(struct pci_dev *pdev, bool prepare)
 		 */
 		mwifiex_shutdown_sw(adapter);
 		adapter->surprise_removed = true;
+		clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags);
+		clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &card->work_flags);
 	} else {
 		/* Kernel stores and restores PCIe function context before and
 		 * after performing FLR respectively. Reconfigure the software
-- 
2.12.2.762.g0e3151a226-goog

[PATCH 4/4] mwifiex: don't leak 'chan_stats' on reset

[Brian Norris]

'chan_stats' is (re)allocated in _mwifiex_fw_dpc() ->
mwifiex_init_channel_scan_gap(), which is called whenever the device is
initialized -- at probe or at reset.

But we only free it in we completely unregister the adapter, meaning we
leak a copy of it during every reset.

Let's free it in the shutdown / removal paths instead (and in the
error-handling path), to avoid the leak.

Ideally, we can eventually unify much of mwifiex_shutdown_sw() and
mwifiex_remove_card() (way too much copy-and-paste) to reduce the burden
on bugfixes like this. But that's work for tomorrow.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
Technically a bugfix, but I don't know where to "blame" it.

 drivers/net/wireless/marvell/mwifiex/main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 89eccd7adbe5..31fa62c4c148 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -149,7 +149,6 @@ static int mwifiex_unregister(struct mwifiex_adapter *adapter)
 
 	kfree(adapter->regd);
 
-	vfree(adapter->chan_stats);
 	kfree(adapter);
 	return 0;
 }
@@ -633,6 +632,7 @@ static int _mwifiex_fw_dpc(const struct firmware *firmware, void *context)
 	goto done;
 
 err_add_intf:
+	vfree(adapter->chan_stats);
 	wiphy_unregister(adapter->wiphy);
 	wiphy_free(adapter->wiphy);
 err_init_fw:
@@ -1417,6 +1417,7 @@ mwifiex_shutdown_sw(struct mwifiex_adapter *adapter)
 			mwifiex_del_virtual_intf(adapter->wiphy, &priv->wdev);
 		rtnl_unlock();
 	}
+	vfree(adapter->chan_stats);
 
 	mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);
 exit_return:
@@ -1728,6 +1729,7 @@ int mwifiex_remove_card(struct mwifiex_adapter *adapter)
 			mwifiex_del_virtual_intf(adapter->wiphy, &priv->wdev);
 		rtnl_unlock();
 	}
+	vfree(adapter->chan_stats);
 
 	wiphy_unregister(adapter->wiphy);
 	wiphy_free(adapter->wiphy);
-- 
2.12.2.762.g0e3151a226-goog

Re: [1/4] mwifiex: pcie: fix cmd_buf use-after-free in remove/reset

[Kalle Valo]

Brian Norris <briannorris@chromium.org> wrote:
> Command buffers (skb's) are allocated by the main driver, and freed upon
> the last use. That last use is often in mwifiex_free_cmd_buffer(). In
> the meantime, if the command buffer gets used by the PCI driver, we map
> it as DMA-able, and store the mapping information in the 'cb' memory.
> 
> However, if a command was in-flight when resetting the device (and
> therefore was still mapped), we don't get a chance to unmap this memory
> until after the core has cleaned up its command handling.
> 
> Let's keep a refcount within the PCI driver, so we ensure the memory
> only gets freed after we've finished unmapping it.
> 
> Noticed by KASAN when forcing a reset via:
> 
>   echo 1 > /sys/bus/pci/.../reset
> 
> The same code path can presumably be exercised in remove() and
> shutdown().
> 
> [  205.390377] mwifiex_pcie 0000:01:00.0: info: shutdown mwifiex...
> [  205.400393] ==================================================================
> [  205.407719] BUG: KASAN: use-after-free in mwifiex_unmap_pci_memory.isra.14+0x4c/0x100 [mwifiex_pcie] at addr ffffffc0ad471b28
> [  205.419040] Read of size 16 by task bash/1913
> [  205.423421] =============================================================================
> [  205.431625] BUG skbuff_head_cache (Tainted: G    B          ): kasan: bad access detected
> [  205.439815] -----------------------------------------------------------------------------
> [  205.439815]
> [  205.449534] INFO: Allocated in __build_skb+0x48/0x114 age=1311 cpu=4 pid=1913
> [  205.456709] 	alloc_debug_processing+0x124/0x178
> [  205.461282] 	___slab_alloc.constprop.58+0x528/0x608
> [  205.466196] 	__slab_alloc.isra.54.constprop.57+0x44/0x54
> [  205.471542] 	kmem_cache_alloc+0xcc/0x278
> [  205.475497] 	__build_skb+0x48/0x114
> [  205.479019] 	__netdev_alloc_skb+0xe0/0x170
> [  205.483244] 	mwifiex_alloc_cmd_buffer+0x68/0xdc [mwifiex]
> [  205.488759] 	mwifiex_init_fw+0x40/0x6cc [mwifiex]
> [  205.493584] 	_mwifiex_fw_dpc+0x158/0x520 [mwifiex]
> [  205.498491] 	mwifiex_reinit_sw+0x2c4/0x398 [mwifiex]
> [  205.503510] 	mwifiex_pcie_reset_notify+0x114/0x15c [mwifiex_pcie]
> [  205.509643] 	pci_reset_notify+0x5c/0x6c
> [  205.513519] 	pci_reset_function+0x6c/0x7c
> [  205.517567] 	reset_store+0x68/0x98
> [  205.521003] 	dev_attr_store+0x54/0x60
> [  205.524705] 	sysfs_kf_write+0x9c/0xb0
> [  205.528413] INFO: Freed in __kfree_skb+0xb0/0xbc age=131 cpu=4 pid=1913
> [  205.535064] 	free_debug_processing+0x264/0x370
> [  205.539550] 	__slab_free+0x84/0x40c
> [  205.543075] 	kmem_cache_free+0x1c8/0x2a0
> [  205.547030] 	__kfree_skb+0xb0/0xbc
> [  205.550465] 	consume_skb+0x164/0x178
> [  205.554079] 	__dev_kfree_skb_any+0x58/0x64
> [  205.558304] 	mwifiex_free_cmd_buffer+0xa0/0x158 [mwifiex]
> [  205.563817] 	mwifiex_shutdown_drv+0x578/0x5c4 [mwifiex]
> [  205.569164] 	mwifiex_shutdown_sw+0x178/0x310 [mwifiex]
> [  205.574353] 	mwifiex_pcie_reset_notify+0xd4/0x15c [mwifiex_pcie]
> [  205.580398] 	pci_reset_notify+0x5c/0x6c
> [  205.584274] 	pci_dev_save_and_disable+0x24/0x6c
> [  205.588837] 	pci_reset_function+0x30/0x7c
> [  205.592885] 	reset_store+0x68/0x98
> [  205.596324] 	dev_attr_store+0x54/0x60
> [  205.600017] 	sysfs_kf_write+0x9c/0xb0
> ...
> [  205.800488] Call trace:
> [  205.802980] [<ffffffc00020a69c>] dump_backtrace+0x0/0x190
> [  205.808415] [<ffffffc00020a96c>] show_stack+0x20/0x28
> [  205.813506] [<ffffffc0005d020c>] dump_stack+0xa4/0xcc
> [  205.818598] [<ffffffc0003be44c>] print_trailer+0x158/0x168
> [  205.824120] [<ffffffc0003be5f0>] object_err+0x4c/0x5c
> [  205.829210] [<ffffffc0003c45bc>] kasan_report+0x334/0x500
> [  205.834641] [<ffffffc0003c3994>] check_memory_region+0x20/0x14c
> [  205.840593] [<ffffffc0003c3b14>] __asan_loadN+0x14/0x1c
> [  205.845879] [<ffffffbffc46171c>] mwifiex_unmap_pci_memory.isra.14+0x4c/0x100 [mwifiex_pcie]
> [  205.854282] [<ffffffbffc461864>] mwifiex_pcie_delete_cmdrsp_buf+0x94/0xa8 [mwifiex_pcie]
> [  205.862421] [<ffffffbffc462028>] mwifiex_pcie_free_buffers+0x11c/0x158 [mwifiex_pcie]
> [  205.870302] [<ffffffbffc4620d4>] mwifiex_pcie_down_dev+0x70/0x80 [mwifiex_pcie]
> [  205.877736] [<ffffffbffc1397a8>] mwifiex_shutdown_sw+0x190/0x310 [mwifiex]
> [  205.884658] [<ffffffbffc4606b4>] mwifiex_pcie_reset_notify+0xd4/0x15c [mwifiex_pcie]
> [  205.892446] [<ffffffc000635f54>] pci_reset_notify+0x5c/0x6c
> [  205.898048] [<ffffffc00063a044>] pci_dev_save_and_disable+0x24/0x6c
> [  205.904350] [<ffffffc00063cf0c>] pci_reset_function+0x30/0x7c
> [  205.910134] [<ffffffc000641118>] reset_store+0x68/0x98
> [  205.915312] [<ffffffc000771588>] dev_attr_store+0x54/0x60
> [  205.920750] [<ffffffc00046f53c>] sysfs_kf_write+0x9c/0xb0
> [  205.926182] [<ffffffc00046dfb0>] kernfs_fop_write+0x184/0x1f8
> [  205.931963] [<ffffffc0003d64f4>] __vfs_write+0x6c/0x17c
> [  205.937221] [<ffffffc0003d7164>] vfs_write+0xf0/0x1c4
> [  205.942310] [<ffffffc0003d7da0>] SyS_write+0x78/0xd8
> [  205.947312] [<ffffffc000204634>] el0_svc_naked+0x24/0x28
> ...
> [  205.998268] ==================================================================
> 
> This bug has been around in different forms for a while. It was sort of
> noticed in commit 955ab095c51a ("mwifiex: Do not kfree cmd buf while
> unregistering PCIe"), but it just fixed the double-free, without
> acknowledging the potential for use-after-free.
> 
> Fixes: fc3314609047 ("mwifiex: use pci_alloc/free_consistent APIs for PCIe")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Brian Norris <briannorris@chromium.org>

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

3c8cb9ad032d mwifiex: pcie: fix cmd_buf use-after-free in remove/reset
9ae3fbd109d9 mwifiex: reset timeout flag when resetting device
35e67d3d58b9 mwifiex: pcie: clear outstanding work when resetting
fb9e67bee3ab mwifiex: don't leak 'chan_stats' on reset

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

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