Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1623928 > unrolled thread
| Started by | Brian Norris <briannorris@chromium.org> |
|---|---|
| First post | 2017-04-15 00:00 +0200 |
| Last post | 2017-04-20 09:30 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH 1/4] mwifiex: pcie: fix cmd_buf use-after-free in remove/reset Brian Norris <briannorris@chromium.org> - 2017-04-15 00:00 +0200
[PATCH 2/4] mwifiex: reset timeout flag when resetting device Brian Norris <briannorris@chromium.org> - 2017-04-15 00:00 +0200
Re: [1/4] mwifiex: pcie: fix cmd_buf use-after-free in remove/reset Kalle Valo <kvalo@codeaurora.org> - 2017-04-20 09:30 +0200
| From | Brian Norris <briannorris@chromium.org> |
|---|---|
| Date | 2017-04-15 00:00 +0200 |
| Subject | [PATCH 1/4] mwifiex: pcie: fix cmd_buf use-after-free in remove/reset |
| Message-ID | <twhq1-6r7-7@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Brian Norris <briannorris@chromium.org> |
|---|---|
| Date | 2017-04-15 00:00 +0200 |
| Subject | [PATCH 2/4] mwifiex: reset timeout flag when resetting device |
| Message-ID | <twhq2-6r7-23@gated-at.bofh.it> |
| In reply to | #1623928 |
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
[toc] | [prev] | [next] | [standalone]
| From | Kalle Valo <kvalo@codeaurora.org> |
|---|---|
| Date | 2017-04-20 09:30 +0200 |
| Subject | Re: [1/4] mwifiex: pcie: fix cmd_buf use-after-free in remove/reset |
| Message-ID | <tyeHo-60B-9@gated-at.bofh.it> |
| In reply to | #1623928 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web