| 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
| ||
|
Date: Fri, 27 May 2016 09:57:52 +0000 (GMT)
From: Chung-Geol Kim <chunggeol.kim@...sung.com>
To: "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
Cc: "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>,
"stern@...land.harvard.edu" <stern@...land.harvard.edu>,
"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, 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@...sung.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
>
Powered by blists - more mailing lists