linux-kernel - [PATCH V2] usb: hub: Fix unbalanced reference count/memory leak/deadlocks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:	Thu,  4 Aug 2016 13:32:22 -0700
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:	linaro-kernel@...ts.linaro.org, alexandru.cornea@...el.com,
	cristina.moraru@...el.com, Alex Elder <elder@...aro.org>,
	Johan Hovold <johan@...oldconsulting.com>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	"#4 . 4+" <stable@...r.kernel.org>,
	Manu Gautam <mgautam@...eaurora.org>,
	Alan Stern <stern@...land.harvard.edu>,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: [PATCH V2] usb: hub: Fix unbalanced reference count/memory leak/deadlocks

Memory leak and unbalanced reference count:

If the hub gets disconnected while the core is still activating it, this
can result in leaking memory of few USB structures.

This will happen if we have done a kref_get() from hub_activate() and
scheduled a delayed work item for HUB_INIT2/3. Now if hub_disconnect()
gets called before the delayed work expires, then we will cancel the
work from hub_quiesce(), but wouldn't do a kref_put(). And so the
unbalance.

kmemleak reports this as (with the commit e50293ef9775 backported to
3.10 kernel with other changes, though the same is true for mainline as
well):

unreferenced object 0xffffffc08af5b800 (size 1024):
  comm "khubd", pid 73, jiffies 4295051211 (age 6482.350s)
  hex dump (first 32 bytes):
    30 68 f3 8c c0 ff ff ff 00 a0 b2 2e c0 ff ff ff  0h..............
    01 00 00 00 00 00 00 00 00 94 7d 40 c0 ff ff ff  ..........}@....
  backtrace:
    [<ffffffc0003079ec>] create_object+0x148/0x2a0
    [<ffffffc000cc150c>] kmemleak_alloc+0x80/0xbc
    [<ffffffc000303a7c>] kmem_cache_alloc_trace+0x120/0x1ac
    [<ffffffc0006fa610>] hub_probe+0x120/0xb84
    [<ffffffc000702b20>] usb_probe_interface+0x1ec/0x298
    [<ffffffc0005d50cc>] driver_probe_device+0x160/0x374
    [<ffffffc0005d5308>] __device_attach+0x28/0x4c
    [<ffffffc0005d3164>] bus_for_each_drv+0x78/0xac
    [<ffffffc0005d4ee0>] device_attach+0x6c/0x9c
    [<ffffffc0005d42b8>] bus_probe_device+0x28/0xa0
    [<ffffffc0005d23a4>] device_add+0x324/0x604
    [<ffffffc000700fcc>] usb_set_configuration+0x660/0x6cc
    [<ffffffc00070a350>] generic_probe+0x44/0x84
    [<ffffffc000702914>] usb_probe_device+0x54/0x74
    [<ffffffc0005d50cc>] driver_probe_device+0x160/0x374
    [<ffffffc0005d5308>] __device_attach+0x28/0x4c

Deadlocks:

If the hub gets disconnected early enough (i.e. before INIT2/INIT3 are
finished and the init_work is still queued), the core may call
hub_quiesce() after acquiring interface device locks and it will wait
for the work to be cancelled synchronously. But if the work handler is
already running in parallel, it may try to acquire the same interface
device lock and this may result in deadlock.

Fix both the issues by removing the call to cancel_delayed_work_sync().

CC: <stable@...r.kernel.org> #4.4+
Fixes: e50293ef9775 ("USB: fix invalid memory access in hub_activate()")
Reported-by: Manu Gautam <mgautam@...eaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
Acked-by: Alan Stern <stern@...land.harvard.edu>
---
V1->V2:
- Solve the deadlock problem as well
- Added Ack from Alan as he was happy with this solution in advance :)
- Dropped cancel-work and kref-put from V1

 drivers/usb/core/hub.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index bee13517676f..3ccffac0f647 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1315,8 +1315,6 @@ static void hub_quiesce(struct usb_hub *hub, enum hub_quiescing_type type)
 	struct usb_device *hdev = hub->hdev;
 	int i;
 
-	cancel_delayed_work_sync(&hub->init_work);
-
 	/* hub_wq and related activity won't re-trigger */
 	hub->quiescing = 1;
 
-- 
2.7.4