* Re: Re: [PATCH] usb: core: fix a double free in the usb driver
@ 2016-05-27 9:57 Chung-Geol Kim
2016-05-27 19:14 ` Alan Stern
0 siblings, 1 reply; 2+ messages in thread
From: Chung-Geol Kim @ 2016-05-27 9:57 UTC (permalink / raw)
To: gregkh
Cc: mathias.nyman, stefan.koch10, hkallweit1, sergei.shtylyov,
dan.j.williams, sarah.a.sharp, stern, chris.bainbridge,
linux-usb, linux-kernel
>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@samsung.com>
>
>> ---
>> 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
>
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: Re: [PATCH] usb: core: fix a double free in the usb driver
2016-05-27 9:57 Re: [PATCH] usb: core: fix a double free in the usb driver Chung-Geol Kim
@ 2016-05-27 19:14 ` Alan Stern
0 siblings, 0 replies; 2+ messages in thread
From: Alan Stern @ 2016-05-27 19:14 UTC (permalink / raw)
To: Chung-Geol Kim
Cc: gregkh, mathias.nyman, stefan.koch10, hkallweit1,
sergei.shtylyov, dan.j.williams, sarah.a.sharp, chris.bainbridge,
linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-05-27 19:14 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-27 9:57 Re: [PATCH] usb: core: fix a double free in the usb driver Chung-Geol Kim
2016-05-27 19:14 ` Alan Stern
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®