linux-kernel - RE: Re: Re: Re: [PATCH] usb: core: fix a double free in the usb driver

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>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.44L0.1606071044140.1604-100000@iolanthe.rowland.org>
Date:	Tue, 7 Jun 2016 10:54:09 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Chung-Geol Kim <chunggeol.kim@...sung.com>
cc:	gregkh@...uxfoundation.org 
	<gregkh@...uxfoundation.org>,
	mathias.nyman@...ux.intel.com 
	<mathias.nyman@...ux.intel.com>,
	stefan.koch10@...il.com 
	<stefan.koch10@...il.com>,
	hkallweit1@...il.com <hkallweit1@...il.com>,
	sergei.shtylyov@...entembedded.com 
	<sergei.shtylyov@...entembedded.com>,
	dan.j.williams@...el.com 
	<dan.j.williams@...el.com>,
	chris.bainbridge@...il.com 
	<chris.bainbridge@...il.com>,
	linux-usb@...r.kernel.org 
	<linux-usb@...r.kernel.org>,
	linux-kernel@...r.kernel.org 
	<linux-kernel@...r.kernel.org>
Subject: RE: Re: Re: Re: [PATCH] usb: core: fix a double free in the usb driver

On Tue, 7 Jun 2016, Chung-Geol Kim wrote:

> =================================================
>      At *remove USB(3.0) Storage 
>      sequence <1> --> <5> ((Problem Case))
> =================================================
>                                   VOLD
> ------------------------------------|------------
>                                  (uevent)
>                             ________|_________
>                            |<1>               |
>                            |dwc3_otg_sm_work  |
>                            |usb_put_hcd       |
>                            |shared_hcd(kref=2)|
>                            |__________________|
>                             ________|_________ 
>                            |<2>               |
>                            |New USB BUS #2    |
>                            |                  |
>                            |shared_hcd(kref=1)|
>                            |                  |
>                          --(Link)-bandXX_mutex|
>                          | |__________________|
>                          | 
>     ___________________  |
>    |<3>                | |
>    |dwc3_otg_sm_work   | |
>    |usb_put_hcd        | |
>    |primary_hcd(kref=1)| |
>    |___________________| |
>     _________|_________  |
>    |<4>                | |
>    |New USB BUS #1     | |
>    |hcd_release        | |
>    |primary_hcd(kref=0)| |
>    |                   | |
>    |bandXX_mutex(free) |<-
>    |___________________| 
>                                (( VOLD ))
>                             ______|___________
>                            |<5>               |
>                            |      SCSI        |
>                            |usb_put_hcd       |
>                            |shared_hcd(kref=0)|
>                            |*hcd_release      |
>                            |bandXX_mutex(free*)|<- double free
>                            |__________________|
> 
> =================================================

Okay, now I understand the problem you want to solve.  What we need to
do is make sure the mutex is deallocated when the _last_ hcd is
released, which is not necessarily the same as when the _primary_ hcd
is released.

Can you please test the patch below?

By the way, a good change (if you want to do it) would be to rename the
"shared_hcd" field to "other_hcd" or "peer_hcd".  This is because it
always points to the other hcd in the peer set: In the primary
structure it points to the secondary, and in the secondary structure it
points to the primary.

Alan Stern



Index: usb-4.x/drivers/usb/core/hcd.c
===================================================================
--- usb-4.x.orig/drivers/usb/core/hcd.c
+++ usb-4.x/drivers/usb/core/hcd.c
@@ -2588,24 +2588,22 @@ EXPORT_SYMBOL_GPL(usb_create_hcd);
  * Don't deallocate the bandwidth_mutex until the last shared usb_hcd is
  * deallocated.
  *
- * Make sure to only deallocate the bandwidth_mutex when the primary HCD is
- * freed.  When hcd_release() is called for either hcd in a peer set
- * invalidate the peer's ->shared_hcd and ->primary_hcd pointers to
- * block new peering attempts
+ * Make sure to deallocate the bandwidth_mutex only when the last HCD is
+ * freed.  When hcd_release() is called for either hcd in a peer set,
+ * invalidate the peer's ->shared_hcd and ->primary_hcd pointers.
  */
 static void hcd_release(struct kref *kref)
 {
 	struct usb_hcd *hcd = container_of (kref, struct usb_hcd, kref);
 
 	mutex_lock(&usb_port_peer_mutex);
-	if (usb_hcd_is_primary_hcd(hcd))
-		kfree(hcd->bandwidth_mutex);
 	if (hcd->shared_hcd) {
 		struct usb_hcd *peer = hcd->shared_hcd;
 
 		peer->shared_hcd = NULL;
-		if (peer->primary_hcd == hcd)
-			peer->primary_hcd = NULL;
+		peer->primary_hcd = NULL;
+	} else {
+		kfree(hcd->bandwidth_mutex);
 	}
 	mutex_unlock(&usb_port_peer_mutex);
 	kfree(hcd);