From: Chung-Geol Kim <chunggeol.kim@samsung.com>
To: "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
Cc: "mathias.nyman@linux.intel.com" <mathias.nyman@linux.intel.com>,
"stefan.koch10@gmail.com" <stefan.koch10@gmail.com>,
"hkallweit1@gmail.com" <hkallweit1@gmail.com>,
"sergei.shtylyov@cogentembedded.com"
<sergei.shtylyov@cogentembedded.com>,
"dan.j.williams@intel.com" <dan.j.williams@intel.com>,
"sarah.a.sharp@linux.intel.com" <sarah.a.sharp@linux.intel.com>,
"stern@rowland.harvard.edu" <stern@rowland.harvard.edu>,
"chris.bainbridge@gmail.com" <chris.bainbridge@gmail.com>,
"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: Re: [PATCH] usb: core: fix a double free in the usb driver
Date: Fri, 27 May 2016 09:57:52 +0000 (GMT) [thread overview]
Message-ID: <1893879755.217681464343067404.JavaMail.weblogic@epmlwas08d> (raw)
>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
>
next reply other threads:[~2016-05-27 9:57 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-27 9:57 Chung-Geol Kim [this message]
2016-05-27 19:14 ` Alan Stern
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1893879755.217681464343067404.JavaMail.weblogic@epmlwas08d \
--to=chunggeol.kim@samsung.com \
--cc=chris.bainbridge@gmail.com \
--cc=dan.j.williams@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=hkallweit1@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=mathias.nyman@linux.intel.com \
--cc=sarah.a.sharp@linux.intel.com \
--cc=sergei.shtylyov@cogentembedded.com \
--cc=stefan.koch10@gmail.com \
--cc=stern@rowland.harvard.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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®