* [PATCH] usb: musb: Fix potential NULL dereference
@ 2019-01-23 17:51 Matwey V. Kornilov
2019-01-24 18:47 ` Matwey V. Kornilov
2019-01-25 16:37 ` [PATCH] usb: musb: Fix potential NULL dereference Bin Liu
0 siblings, 2 replies; 7+ messages in thread
From: Matwey V. Kornilov @ 2019-01-23 17:51 UTC (permalink / raw)
To: Bin Liu, Greg Kroah-Hartman
Cc: matwey.kornilov, Matwey V. Kornilov,
open list:MUSB MULTIPOINT HIGH SPEED DUAL-ROLE CONTROLLER,
open list
We assign "urb->hcpriv = qh;" a few lines down. The valid qh for the urb is
hep->hcpriv in this code path.
Fixes: 714bc5ef3eda ("musb: potential use after free")
Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
---
drivers/usb/musb/musb_host.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index c6118a962d37..6f267716768e 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -2336,7 +2336,7 @@ static int musb_urb_enqueue(
* odd, rare, error prone, but legal.
*/
kfree(qh);
- qh = NULL;
+ qh = hep->hcpriv;
ret = 0;
} else
ret = musb_schedule(musb, qh,
--
2.16.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: musb: Fix potential NULL dereference
2019-01-23 17:51 [PATCH] usb: musb: Fix potential NULL dereference Matwey V. Kornilov
@ 2019-01-24 18:47 ` Matwey V. Kornilov
2019-01-25 16:44 ` Bin Liu
2019-01-25 16:37 ` [PATCH] usb: musb: Fix potential NULL dereference Bin Liu
1 sibling, 1 reply; 7+ messages in thread
From: Matwey V. Kornilov @ 2019-01-24 18:47 UTC (permalink / raw)
To: Bin Liu, Greg Kroah-Hartman
Cc: open list:MUSB MULTIPOINT HIGH SPEED DUAL-ROLE CONTROLLER, open list
By the way, why do we need to store the qh in urb->hcpriv?
qh can always be accessible through urb->ep->hcpriv
Wouldn't it be better to drop entire urb->hcpriv usage?
ср, 23 янв. 2019 г. в 20:52, Matwey V. Kornilov <matwey@sai.msu.ru>:
>
> We assign "urb->hcpriv = qh;" a few lines down. The valid qh for the urb is
> hep->hcpriv in this code path.
>
> Fixes: 714bc5ef3eda ("musb: potential use after free")
> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
> ---
> drivers/usb/musb/musb_host.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> index c6118a962d37..6f267716768e 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -2336,7 +2336,7 @@ static int musb_urb_enqueue(
> * odd, rare, error prone, but legal.
> */
> kfree(qh);
> - qh = NULL;
> + qh = hep->hcpriv;
> ret = 0;
> } else
> ret = musb_schedule(musb, qh,
> --
> 2.16.4
>
--
With best regards,
Matwey V. Kornilov
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: musb: Fix potential NULL dereference
2019-01-23 17:51 [PATCH] usb: musb: Fix potential NULL dereference Matwey V. Kornilov
2019-01-24 18:47 ` Matwey V. Kornilov
@ 2019-01-25 16:37 ` Bin Liu
1 sibling, 0 replies; 7+ messages in thread
From: Bin Liu @ 2019-01-25 16:37 UTC (permalink / raw)
To: Matwey V. Kornilov
Cc: Greg Kroah-Hartman, matwey.kornilov,
open list:MUSB MULTIPOINT HIGH SPEED DUAL-ROLE CONTROLLER,
open list
On Wed, Jan 23, 2019 at 08:51:42PM +0300, Matwey V. Kornilov wrote:
> We assign "urb->hcpriv = qh;" a few lines down. The valid qh for the urb is
> hep->hcpriv in this code path.
>
> Fixes: 714bc5ef3eda ("musb: potential use after free")
> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
> ---
> drivers/usb/musb/musb_host.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> index c6118a962d37..6f267716768e 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -2336,7 +2336,7 @@ static int musb_urb_enqueue(
> * odd, rare, error prone, but legal.
> */
> kfree(qh);
> - qh = NULL;
> + qh = hep->hcpriv;
> ret = 0;
> } else
> ret = musb_schedule(musb, qh,
Did you run into any issue? The original code seems to be correct to me.
Regards,
-Bin.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: musb: Fix potential NULL dereference
2019-01-24 18:47 ` Matwey V. Kornilov
@ 2019-01-25 16:44 ` Bin Liu
2019-01-25 21:37 ` Alan Stern
0 siblings, 1 reply; 7+ messages in thread
From: Bin Liu @ 2019-01-25 16:44 UTC (permalink / raw)
To: Matwey V. Kornilov
Cc: Greg Kroah-Hartman,
open list:MUSB MULTIPOINT HIGH SPEED DUAL-ROLE CONTROLLER,
open list
On Thu, Jan 24, 2019 at 09:47:02PM +0300, Matwey V. Kornilov wrote:
> By the way, why do we need to store the qh in urb->hcpriv?
> qh can always be accessible through urb->ep->hcpriv
> Wouldn't it be better to drop entire urb->hcpriv usage?
I am not sure why. The code is there since the first commit in a decade
ago. But I tend to agree with you.
In a quick search for urb->hcpriv and urb->ep->hcpriv, based on the
usage in core/hcd.c, it seems to me that urb->hcpriv should not be
changed in each controller driver, but I see both have been used in most
controller drivers. I will leave this to others to educate me.
Regards,
-Bin.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: musb: Fix potential NULL dereference
2019-01-25 16:44 ` Bin Liu
@ 2019-01-25 21:37 ` Alan Stern
2019-01-26 9:45 ` Matwey V. Kornilov
0 siblings, 1 reply; 7+ messages in thread
From: Alan Stern @ 2019-01-25 21:37 UTC (permalink / raw)
To: Bin Liu
Cc: Matwey V. Kornilov, Greg Kroah-Hartman,
open list:MUSB MULTIPOINT HIGH SPEED DUAL-ROLE CONTROLLER,
open list
On Fri, 25 Jan 2019, Bin Liu wrote:
> On Thu, Jan 24, 2019 at 09:47:02PM +0300, Matwey V. Kornilov wrote:
> > By the way, why do we need to store the qh in urb->hcpriv?
> > qh can always be accessible through urb->ep->hcpriv
> > Wouldn't it be better to drop entire urb->hcpriv usage?
>
> I am not sure why. The code is there since the first commit in a decade
> ago. But I tend to agree with you.
>
> In a quick search for urb->hcpriv and urb->ep->hcpriv, based on the
> usage in core/hcd.c, it seems to me that urb->hcpriv should not be
> changed in each controller driver, but I see both have been used in most
> controller drivers. I will leave this to others to educate me.
In some of the older HCDs, urb->hcpriv != NULL is used to indicate that
urb is on an endpoint queue. Perhaps that usage was copied.
Alan Stern
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: musb: Fix potential NULL dereference
2019-01-25 21:37 ` Alan Stern
@ 2019-01-26 9:45 ` Matwey V. Kornilov
2019-02-18 16:44 ` [PATCH v2] usb: musb: Fix urb->hcpriv value Matwey V. Kornilov
0 siblings, 1 reply; 7+ messages in thread
From: Matwey V. Kornilov @ 2019-01-26 9:45 UTC (permalink / raw)
To: Alan Stern
Cc: Bin Liu, Greg Kroah-Hartman,
open list:MUSB MULTIPOINT HIGH SPEED DUAL-ROLE CONTROLLER,
open list
сб, 26 янв. 2019 г. в 00:37, Alan Stern <stern@rowland.harvard.edu>:
>
> On Fri, 25 Jan 2019, Bin Liu wrote:
>
> > On Thu, Jan 24, 2019 at 09:47:02PM +0300, Matwey V. Kornilov wrote:
> > > By the way, why do we need to store the qh in urb->hcpriv?
> > > qh can always be accessible through urb->ep->hcpriv
> > > Wouldn't it be better to drop entire urb->hcpriv usage?
> >
> > I am not sure why. The code is there since the first commit in a decade
> > ago. But I tend to agree with you.
> >
> > In a quick search for urb->hcpriv and urb->ep->hcpriv, based on the
> > usage in core/hcd.c, it seems to me that urb->hcpriv should not be
> > changed in each controller driver, but I see both have been used in most
> > controller drivers. I will leave this to others to educate me.
>
> In some of the older HCDs, urb->hcpriv != NULL is used to indicate that
> urb is on an endpoint queue. Perhaps that usage was copied.
>
> Alan Stern
>
Hi,
Thank you for the explanation. I think that It would be great to
document it somewhere. Such a purpose for variable named `hcpriv' is
not entirely obvious.
Now it is clear to me, why __usb_hcd_giveback_urb() sets urb->hcpriv to NULL.
Returning to my initial patch. I think that it is still valid, though
I admit that the commit message must be rewritten.
In this code path we successfully queued the new urb, so urb->hcpriv
should be NOT NULL indicating that the urb is queued according to Alan
explanation.
musb_urb_enqueue(), musb_host.c line 2345:
if (ret == 0) {
urb->hcpriv = qh;
/* FIXME set urb->start_frame for iso/intr, it's tested in
* musb_start_urb(), but otherwise only konicawc cares ...
*/
}
--
With best regards,
Matwey V. Kornilov
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] usb: musb: Fix urb->hcpriv value
2019-01-26 9:45 ` Matwey V. Kornilov
@ 2019-02-18 16:44 ` Matwey V. Kornilov
0 siblings, 0 replies; 7+ messages in thread
From: Matwey V. Kornilov @ 2019-02-18 16:44 UTC (permalink / raw)
To: b-liu, gregkh
Cc: matwey.kornilov, Matwey V. Kornilov, stern, linux-usb, linux-kernel
urb->hcpriv != NULL is used to indicate that the URB is queued [1].
Also see __usb_hcd_giveback_urb() and usb_hcd_submit_urb() for
the reference.
In this code path, the URB is actually queued and valid qh is hep->hcpriv.
[1] https://lkml.org/lkml/2019/1/25/750
Fixes: 714bc5ef3eda ("musb: potential use after free")
Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
---
drivers/usb/musb/musb_host.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index b59ce9ad14ce..a60d52c7e112 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -2302,7 +2302,7 @@ static int musb_urb_enqueue(
* odd, rare, error prone, but legal.
*/
kfree(qh);
- qh = NULL;
+ qh = hep->hcpriv;
ret = 0;
} else
ret = musb_schedule(musb, qh,
--
2.16.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-02-18 16:54 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-23 17:51 [PATCH] usb: musb: Fix potential NULL dereference Matwey V. Kornilov
2019-01-24 18:47 ` Matwey V. Kornilov
2019-01-25 16:44 ` Bin Liu
2019-01-25 21:37 ` Alan Stern
2019-01-26 9:45 ` Matwey V. Kornilov
2019-02-18 16:44 ` [PATCH v2] usb: musb: Fix urb->hcpriv value Matwey V. Kornilov
2019-01-25 16:37 ` [PATCH] usb: musb: Fix potential NULL dereference Bin Liu
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).