linux-kernel - 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.1605271505321.1807-100000@iolanthe.rowland.org>
Date:	Fri, 27 May 2016 15:14:05 -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>,
	"sarah.a.sharp@...ux.intel.com" <sarah.a.sharp@...ux.intel.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: [PATCH] usb: core: fix a double free in the usb driver

On Fri, 27 May 2016, Chung-Geol Kim wrote:

> >On Fri, May 27, 2016 at 01:38:17AM +0000, Chung-Geol Kim wrote:
> >> There is a double free problem in the usb driver.
> >
> >Which driver?
> When I using the USB OTG Storage, this issue happened.
> When remove the OTG Storage, it reproduced sometimes.

>      cpu 0                                                   cpu 1
>      ----------------------------------------------------------------------------------------
>      (*Insert USB Storage)
>      usb_create_shared_hcd()
>      kmalloc(primary_hcd)
>      kmalloc(primary_hcd->bandwidth_mutex)
>       ->(primary_hcd->kref==1)
>      usb_get_hcd()
>       ->(primary_hcd->kref==2)
>                                                      usb_create_shared_hcd()
>                                                      kmalloc(hcd->shared_hcd)
>                                                       ->hcd->shared_hcd->bandwidth_mutex=primary->bandwidth_mutex
>                                                       ->primary_hcd->primary_hcd = primary_hcd
>                                                       ->hcd->shared_hcd->primary_hcd = primary_hcd
>                                                      	->(hcd->shared_hcd->kref==1)
>                                                      usb_get_hcd()
>                                                      	->(hcd->shared_hcd->kref==2)
> 
>                                                      usb_get_hcd()
>                                                       ->(hcd->shared_hcd->kref==3)

I don't understand.  Why do these actions take place on two different
CPUs?  Aren't the primary_hcd and the shared_hcd structures allocated
by the same thread, on the same CPU?

>     ---------------------------------------------------------------------------------------
>      (*remove USB Storage)	
>                                                      usb_release_dev()
>                                                       ->(hcd->shared_hcd-kref==2)
>                                                      usb_release_dev()
>                                                       ->(hcd->shared_hcd-kref==1)
>      usb_release_dev()
>       -> (primary_hcd-kref==1)
>      usb_release_dev()
>       -> (primary_hcd-kref==0)
>      hcd_release()
>       -> kfree(primary_hcd->bandwidth_mutex)
>       -> hcd->shared_hcd->primary_hcd = NULL
>       -> kfree(primary_hcd)
>                                                      usb_release_dev()
>                                                       -> (hcd->shared_hcd-kref==0)
>                                                      hcd_release()
>                                                       -> usb_hcd_is_primary_hcd(hcd->shared_hcd)
>                                                           -> hcd->shared_hcd->primary_hcd already NULL, return 1
>                                                       -> try to double kfree(primary_hcd->bandwidth_mutex)

The same question applies here.  Aren't the shared_hcd and primary_hcd 
structures released by the same thread, on the same CPU?

The real bug here is that the shared_hcd is released after the 
primary_hcd.  That's what you need to fix.

> Since hcd->shared_hcd->priary_hcd was Null it didn't reach (hcd == hcd->primary_hcd) in usb_hcd_is_primary_hcd().
> It returned 1 at since condition !hcd->primary_hcd is met.

> >> --- a/drivers/usb/core/hcd.c
> >> +++ b/drivers/usb/core/hcd.c
> >> @@ -2608,7 +2608,7 @@ 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)) {
> >> + if (hcd == hcd->primary_hcd) {
> >
> >That doesn't make sense, usb_hcd_is_primary_hcd() is the same as this
> >check, what are you changing here?
> 
> Since hcd->priary_hcd was Null it didn't reach (hcd == hcd->primary_hcd).
> It returned 1 at since condition !hcd->primary_hcd is met.
> 
> int usb_hcd_is_primary_hcd(struct usb_hcd *hcd)
> {
> 	if (!hcd->primary_hcd)
> 		return 1;
> 	return hcd == hcd->primary_hcd;
> }

That's just a symptom, not the real cause of the bug.  You need to fix 
the real cause: the shared_hcd has to be released _before_ the 
primary_hcd.

The right way to do this is to make the shared_hcd take a reference to
the primary_hcd.  This reference should be dropped when hcd_release()
is called for the shared_hcd.

Alan Stern