Re: Re: [PATCH] usb: core: fix a double free in the usb driver

[Alan Stern]

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