Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1407971 > unrolled thread
| Started by | Chung-Geol Kim <chunggeol.kim@samsung.com> |
|---|---|
| First post | 2016-05-27 12:00 +0200 |
| Last post | 2016-05-27 21:20 +0200 |
| Articles | 2 — 2 participants |
Back to article view | Back to linux.kernel
Re: Re: [PATCH] usb: core: fix a double free in the usb driver Chung-Geol Kim <chunggeol.kim@samsung.com> - 2016-05-27 12:00 +0200
Re: Re: [PATCH] usb: core: fix a double free in the usb driver Alan Stern <stern@rowland.harvard.edu> - 2016-05-27 21:20 +0200
| From | Chung-Geol Kim <chunggeol.kim@samsung.com> |
|---|---|
| Date | 2016-05-27 12:00 +0200 |
| Subject | Re: Re: [PATCH] usb: core: fix a double free in the usb driver |
| Message-ID | <rDmIK-1N3-9@gated-at.bofh.it> |
>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
>
[toc] | [next] | [standalone]
| From | Alan Stern <stern@rowland.harvard.edu> |
|---|---|
| Date | 2016-05-27 21:20 +0200 |
| Message-ID | <rDvsC-7qp-11@gated-at.bofh.it> |
| In reply to | #1407971 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web