* Re: Re: Re: [PATCH] usb: core: fix a double free in the usb driver
@ 2016-06-03 11:51 Chung-Geol Kim
2016-06-03 14:28 ` Alan Stern
0 siblings, 1 reply; 2+ messages in thread
From: Chung-Geol Kim @ 2016-06-03 11:51 UTC (permalink / raw)
To: Alan Stern
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?
Yes, you are right, The presentational errors in order to obtain an understanding of the process.
Therefore, I will be happy to explain again the diagrammatic representation as shown below.
If using usb 3.0 storage(OTG), you can see as below.
==============================================
At *Insert USB(3.0) Storage
sequence <1> --> <5>
==============================================
VOLD
=================================|============
(uevent)
______|___________
|<5> |
| SCSI |
|usb_get_hcd |
|shared_hcd(kref=3)|
|__________________|
___________________ ________|_________
|<2> | |<4> |
|dwc3_otg_sm_work | |dwc3_otg_sm_work |
|usb_get_hcd | |usb_get_hcd |
|primary_hcd(kref=2)| |shared_hcd(kref=2)|
|___________________| |__________________|
_________|_________ ________|_________
|<1> | |<3> |
|New USB BUS #1 | |New USB BUS #2 |
|usb_create_hcd | |usb_create_hcd |
|primary_hcd(kref=1)| |shared_hcd(kref=1)|
| | | |
|bandXX_mutex(alloc)|<-(Link)-bandXX_mutex |
|___________________| |__________________|
==============================================
At *remove USB(3.0) Storage
sequence <1> --> <5> ((Normal Case))
==============================================
VOLD
=================================|============
(uevent)
______|___________
|<1> |
| SCSI |
|usb_put_hcd |
|shared_hcd(kref=2)|
|__________________|
___________________ ________|_________
|<4> | |<2> |
|dwc3_otg_sm_work | |dwc3_otg_sm_work |
|usb_put_hcd | |usb_put_hcd |
|primary_hcd(kref=1)| |shared_hcd(kref=1)|
|___________________| |__________________|
_________|_________ ________|_________
|<5> | |<3> |
|New USB BUS #1 | |New USB BUS #2 |
|hcd_release | |hcd_release |
|primary_hcd(kref=0)| |shared_hcd(kref=0)|
| | | |
|bandXX_mutex(free) | -X-cut off)-bandXX_mutex|
|___________________| |__________________|
----------------------------------------------
>
>> ---------------------------------------------------------------------------------------
>> (*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.
NO, It 's only depend on vold(scsi) release time.
If the vold later released and is being released first hcd,
Double free happened at <5> as below.
==============================================
At *remove USB(3.0) Storage
sequence <1> --> <5> ((Problem Case))
==============================================
VOLD
=================================|============
(uevent)
______|___________
|<5> |
| SCSI |
|usb_put_hcd |
|shared_hcd(kref=0)|
|*hcd_release |
|bandXX_mutex(free*)|<- double free
|__________________|
___________________ ________|_________
|<3> | |<1> |
|dwc3_otg_sm_work | |dwc3_otg_sm_work |
|usb_put_hcd | |usb_put_hcd |
|primary_hcd(kref=1)| |shared_hcd(kref=2)|
|___________________| |__________________|
_________|_________ ________|_________
|<4> | |<2> |
|New USB BUS #1 | |New USB BUS #2 |
|hcd_release | | |
|primary_hcd(kref=0)| |shared_hcd(kref=1)|
| | | |
|bandXX_mutex(free) |<-(Link)-bandXX_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.
>
>> >> --- 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
* Re: Re: Re: [PATCH] usb: core: fix a double free in the usb driver
2016-06-03 11:51 Re: Re: [PATCH] usb: core: fix a double free in the usb driver Chung-Geol Kim
@ 2016-06-03 14:28 ` Alan Stern
0 siblings, 0 replies; 2+ messages in thread
From: Alan Stern @ 2016-06-03 14:28 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, 3 Jun 2016, Chung-Geol Kim wrote:
> Yes, you are right, The presentational errors in order to obtain an understanding of the process.
> Therefore, I will be happy to explain again the diagrammatic representation as shown below.
> If using usb 3.0 storage(OTG), you can see as below.
>
> ==============================================
> At *Insert USB(3.0) Storage
> sequence <1> --> <5>
> ==============================================
> VOLD
> =================================|============
> (uevent)
> ______|___________
> |<5> |
> | SCSI |
> |usb_get_hcd |
> |shared_hcd(kref=3)|
> |__________________|
> ___________________ ________|_________
> |<2> | |<4> |
> |dwc3_otg_sm_work | |dwc3_otg_sm_work |
> |usb_get_hcd | |usb_get_hcd |
> |primary_hcd(kref=2)| |shared_hcd(kref=2)|
> |___________________| |__________________|
> _________|_________ ________|_________
> |<1> | |<3> |
> |New USB BUS #1 | |New USB BUS #2 |
> |usb_create_hcd | |usb_create_hcd |
> |primary_hcd(kref=1)| |shared_hcd(kref=1)|
> | | | |
> |bandXX_mutex(alloc)|<-(Link)-bandXX_mutex |
> |___________________| |__________________|
>
When people present diagrams like this, the universal convention is to
show earlier times at the top and later times at the bottom. That way
the order in which you read the diagram is the same as the order in
which the events are supposed to occur.
Also, the convention is to put events next to each other if they happen
at the same time. In your diagram, <1> and <3> are next to each other
but they don't happen at the same time.
> ==============================================
> At *remove USB(3.0) Storage
> sequence <1> --> <5> ((Normal Case))
> ==============================================
> VOLD
> =================================|============
> (uevent)
> ______|___________
> |<1> |
> | SCSI |
> |usb_put_hcd |
> |shared_hcd(kref=2)|
> |__________________|
> ___________________ ________|_________
> |<4> | |<2> |
> |dwc3_otg_sm_work | |dwc3_otg_sm_work |
> |usb_put_hcd | |usb_put_hcd |
> |primary_hcd(kref=1)| |shared_hcd(kref=1)|
> |___________________| |__________________|
> _________|_________ ________|_________
> |<5> | |<3> |
> |New USB BUS #1 | |New USB BUS #2 |
> |hcd_release | |hcd_release |
> |primary_hcd(kref=0)| |shared_hcd(kref=0)|
> | | | |
> |bandXX_mutex(free) | -X-cut off)-bandXX_mutex|
> |___________________| |__________________|
>
> ----------------------------------------------
> >The real bug here is that the shared_hcd is released after the
> >primary_hcd. That's what you need to fix.
>
> NO, It 's only depend on vold(scsi) release time.
> If the vold later released and is being released first hcd,
> Double free happened at <5> as below.
>
> ==============================================
> At *remove USB(3.0) Storage
> sequence <1> --> <5> ((Problem Case))
> ==============================================
> VOLD
> =================================|============
> (uevent)
> ______|___________
> |<5> |
> | SCSI |
> |usb_put_hcd |
> |shared_hcd(kref=0)|
> |*hcd_release |
> |bandXX_mutex(free*)|<- double free
> |__________________|
> ___________________ ________|_________
> |<3> | |<1> |
> |dwc3_otg_sm_work | |dwc3_otg_sm_work |
> |usb_put_hcd | |usb_put_hcd |
> |primary_hcd(kref=1)| |shared_hcd(kref=2)|
> |___________________| |__________________|
> _________|_________ ________|_________
> |<4> | |<2> |
> |New USB BUS #1 | |New USB BUS #2 |
> |hcd_release | | |
> |primary_hcd(kref=0)| |shared_hcd(kref=1)|
> | | | |
> |bandXX_mutex(free) |<-(Link)-bandXX_mutex |
> |___________________| |__________________|
>
> ----------------------------------------------
That's the same as what I said. In your diagram, vold releases the
shared_hcd (in <5>) after dwc3_otg_sm_work releases the primary_hcd
(in <4>).
If you change the code so that the shared_hcd takes a reference to the
primary_hcd and drops that reference when the shared_hcd is released,
this will never occur.
Alan Stern
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-06-03 14:28 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-03 11:51 Re: Re: [PATCH] usb: core: fix a double free in the usb driver Chung-Geol Kim
2016-06-03 14:28 ` Alan Stern
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).