[PATCH 0/2] musb fixes for v4.17-rc4

[PATCH 0/2] musb fixes for v4.17-rc4

[Bin Liu]

Hi Greg,

Here are musb fixes for v4.17-rc4 to fix two NULL pointer dereference cases.
Please let me know if any change is needed.

Regards,
-Bin.
----

Bin Liu (2):
  usb: musb: host: fix potential NULL pointer dereference
  usb: musb: trace: fix NULL pointer dereference in musb_g_tx()

 drivers/usb/musb/musb_gadget.c | 3 ++-
 drivers/usb/musb/musb_host.c   | 4 +++-
 2 files changed, 5 insertions(+), 2 deletions(-)

-- 
1.9.1

[PATCH 1/2] usb: musb: host: fix potential NULL pointer dereference

[Bin Liu]

musb_start_urb() doesn't check the pass-in parameter if it is NULL.  But
in musb_bulk_nak_timeout() the parameter passed to musb_start_urb() is
returned from first_qh(), which could be NULL.

So wrap the musb_start_urb() call here with a if condition check to
avoid the potential NULL pointer dereference.

Fixes: f283862f3b5cb("usb: musb: NAK timeout scheme on bulk TX endpoint")

Cc: stable@vger.kernel.org # v3.7+
Signed-off-by: Bin Liu <b-liu@ti.com>
---
 drivers/usb/musb/musb_host.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 4fa372c845e1..e7f99d55922a 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -990,7 +990,9 @@ static void musb_bulk_nak_timeout(struct musb *musb, struct musb_hw_ep *ep,
 			/* set tx_reinit and schedule the next qh */
 			ep->tx_reinit = 1;
 		}
-		musb_start_urb(musb, is_in, next_qh);
+
+		if (next_qh)
+			musb_start_urb(musb, is_in, next_qh);
 	}
 }
 
-- 
1.9.1

[PATCH 2/2] usb: musb: trace: fix NULL pointer dereference in musb_g_tx()

[Bin Liu]

The usb_request pointer could be NULL in musb_g_tx(), where the
tracepoint call would trigger the NULL pointer dereference failure when
parsing the members of the usb_request pointer.

Move the tracepoint call to where the usb_request pointer is already
checked to solve the issue.

Fixes: fc78003e5345a("usb: musb: gadget: add usb-request tracepoints")

Cc: stable@vger.kernel.org # v4.8+
Signed-off-by: Bin Liu <b-liu@ti.com>
---
 drivers/usb/musb/musb_gadget.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index e564695c6c8d..71c5835ea9cd 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -417,7 +417,6 @@ void musb_g_tx(struct musb *musb, u8 epnum)
 	req = next_request(musb_ep);
 	request = &req->request;
 
-	trace_musb_req_tx(req);
 	csr = musb_readw(epio, MUSB_TXCSR);
 	musb_dbg(musb, "<== %s, txcsr %04x", musb_ep->end_point.name, csr);
 
@@ -456,6 +455,8 @@ void musb_g_tx(struct musb *musb, u8 epnum)
 		u8	is_dma = 0;
 		bool	short_packet = false;
 
+		trace_musb_req_tx(req);
+
 		if (dma && (csr & MUSB_TXCSR_DMAENAB)) {
 			is_dma = 1;
 			csr |= MUSB_TXCSR_P_WZC_BITS;
-- 
1.9.1

Re: [PATCH 1/2] usb: musb: host: fix potential NULL pointer dereference

[Greg Kroah-Hartman]

On Mon, Apr 30, 2018 at 11:20:53AM -0500, Bin Liu wrote:
> musb_start_urb() doesn't check the pass-in parameter if it is NULL.  But
> in musb_bulk_nak_timeout() the parameter passed to musb_start_urb() is
> returned from first_qh(), which could be NULL.
> 
> So wrap the musb_start_urb() call here with a if condition check to
> avoid the potential NULL pointer dereference.
> 
> Fixes: f283862f3b5cb("usb: musb: NAK timeout scheme on bulk TX endpoint")

Nit, you forgot a ' ', this should be:
	f283862f3b5c ("usb: musb: NAK timeout scheme on bulk TX endpoint")

You also had one extra id value in there, odd.  I'll edit this by
hand...

greg k-h

Re: [PATCH 1/2] usb: musb: host: fix potential NULL pointer dereference

[Bin Liu]

On Mon, Apr 30, 2018 at 09:42:15AM -0700, Greg Kroah-Hartman wrote:
> On Mon, Apr 30, 2018 at 11:20:53AM -0500, Bin Liu wrote:
> > musb_start_urb() doesn't check the pass-in parameter if it is NULL.  But
> > in musb_bulk_nak_timeout() the parameter passed to musb_start_urb() is
> > returned from first_qh(), which could be NULL.
> > 
> > So wrap the musb_start_urb() call here with a if condition check to
> > avoid the potential NULL pointer dereference.
> > 
> > Fixes: f283862f3b5cb("usb: musb: NAK timeout scheme on bulk TX endpoint")
> 
> Nit, you forgot a ' ', this should be:
> 	f283862f3b5c ("usb: musb: NAK timeout scheme on bulk TX endpoint")

Sorry, thanks.

> You also had one extra id value in there, odd.  I'll edit this by

Not sure why 'git blame' gives that one extra on my computer. I will see
if I will figure it out...

> hand...

Thanks,
-Bin.

Re: [PATCH 1/2] usb: musb: host: fix potential NULL pointer dereference

[Greg Kroah-Hartman]

On Mon, Apr 30, 2018 at 01:11:34PM -0500, Bin Liu wrote:
> On Mon, Apr 30, 2018 at 09:42:15AM -0700, Greg Kroah-Hartman wrote:
> > On Mon, Apr 30, 2018 at 11:20:53AM -0500, Bin Liu wrote:
> > > musb_start_urb() doesn't check the pass-in parameter if it is NULL.  But
> > > in musb_bulk_nak_timeout() the parameter passed to musb_start_urb() is
> > > returned from first_qh(), which could be NULL.
> > > 
> > > So wrap the musb_start_urb() call here with a if condition check to
> > > avoid the potential NULL pointer dereference.
> > > 
> > > Fixes: f283862f3b5cb("usb: musb: NAK timeout scheme on bulk TX endpoint")
> > 
> > Nit, you forgot a ' ', this should be:
> > 	f283862f3b5c ("usb: musb: NAK timeout scheme on bulk TX endpoint")
> 
> Sorry, thanks.
> 
> > You also had one extra id value in there, odd.  I'll edit this by
> 
> Not sure why 'git blame' gives that one extra on my computer. I will see
> if I will figure it out...

Why use 'git blame'?  I use:
	git show -s --abbrev-commit --abbrev=12 --pretty=format:"%h (\"%s\")%n"

to generate these types of lines.

thanks,

greg k-h

Re: [PATCH 1/2] usb: musb: host: fix potential NULL pointer dereference

[Bin Liu]

On Mon, Apr 30, 2018 at 11:24:48AM -0700, Greg Kroah-Hartman wrote:
> On Mon, Apr 30, 2018 at 01:11:34PM -0500, Bin Liu wrote:
> > On Mon, Apr 30, 2018 at 09:42:15AM -0700, Greg Kroah-Hartman wrote:
> > > On Mon, Apr 30, 2018 at 11:20:53AM -0500, Bin Liu wrote:
> > > > musb_start_urb() doesn't check the pass-in parameter if it is NULL.  But
> > > > in musb_bulk_nak_timeout() the parameter passed to musb_start_urb() is
> > > > returned from first_qh(), which could be NULL.
> > > > 
> > > > So wrap the musb_start_urb() call here with a if condition check to
> > > > avoid the potential NULL pointer dereference.
> > > > 
> > > > Fixes: f283862f3b5cb("usb: musb: NAK timeout scheme on bulk TX endpoint")
> > > 
> > > Nit, you forgot a ' ', this should be:
> > > 	f283862f3b5c ("usb: musb: NAK timeout scheme on bulk TX endpoint")
> > 
> > Sorry, thanks.
> > 
> > > You also had one extra id value in there, odd.  I'll edit this by
> > 
> > Not sure why 'git blame' gives that one extra on my computer. I will see
> > if I will figure it out...
> 
> Why use 'git blame'?  I use:
> 	git show -s --abbrev-commit --abbrev=12 --pretty=format:"%h (\"%s\")%n"
> 
> to generate these types of lines.

My ~/.gitconfig already has
	abbrev = 12

but I used 'git blame' to find the commit id which introduces the bug,
then just directly copied the commit id from there... only until now
know 'git blame' gives 13 chars...

I will create a shortcut for the command you gave above to avoid this 13
chars problem from now on.

Thanks,
-Bin.