| Message ID | 20170414215120.145038-1-briannorris@chromium.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3c8cb9ad032d737b874e402c59eb51e3c991a144 |
| Delegated to: | Kalle Valo |
| Headers | show |
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
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; }
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(+)