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

[Chung-Geol Kim]

>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.

>
>> This is caused by delayed deregister for scsi device.
>> <*> at Insert USB Storage
>>     -  USB bus #1 register
>>      usb_create_hcd (primary-kref==1)
>>      * primary-bandwidth_mutex(alloc))
>> usb_get_hcd    (primary-kref==2)
>>     -  USB bus #2 register
>> usb_create_hcd (second-kref==1)
>>      * second-bandwidth_mutex==primary-bandwidth_mutex
>>      usb_get_hcd    (second-kref==2)
>>     -  scsi_device_get
>> usb_get_hcd    (second-kref==3)
>> 
>> <*> at remove USB Storage (Normal)
>>     -  scsi_device_put
>> usb_put_hcd    (second-kref==2)
>>     -  USB bus #2 deregister
>>      usb_release_dev(second-kref==1)
>> usb_release_dev(second-kref==0)  -> hcd_release()
>>     -  USB bus #1 deregister
>>      usb_release_dev(primary-kref==1)
>>      usb_release_dev(primary-kref==0) -> hcd_release()
>> *(primary-bandwidth_mutex free)
>> 
>> at remove USB Storage 
>>     -  USB bus #2 deregister
>>      usb_release_dev(second-kref==2)
>> usb_release_dev(second-kref==1)
>>     -  USB bus #1 deregister
>>      usb_release_dev(primary-kref==1)
>>      usb_release_dev(primary-kref==0) -> hcd_release()
>>      *(primary-bandwidth_mutex free)
>>     -  scsi_device_put
>>      usb_put_hcd    (second-kref==0)  -> hcd_release(*)
>>      * at this, second->primary==0 therefore try to
>> free the primary-bandwidth_mutex.(already freed)
>
>The formatting for this is all confused, can you fix it up?
Sorry to confuse you, Let me change as below.
     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)
    ---------------------------------------------------------------------------------------
     (*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)

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.

>
>> 
>>     To fix this problem kfree(hcd->bandwidth_mutex);
>>     should be executed at only (hcd->primary_hcd==hcd).
>> 
>> Signed-off-by: Chunggeol Kim 
>
>We need an email address at the end of this line, look at how the
>commits in the kernel git history look like for examples.

sorry, maybe deleted during transfer the patch.
Signed-off-by: Chunggeol Kim <chunggeol.kim@xxxxxxxxxxx>

>
>> ---
>> drivers/usb/core/hcd.c |    2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index 34b837a..60077f3 100644
>> --- 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;
}

>
>> kfree(hcd->address0_mutex);
>> kfree(hcd->bandwidth_mutex);
>> }
>
>Your patch itself is also corrupted, and can't be applied, can you also
>resolve this and resend?
>
>thanks,
>
>greg k-h
>