Bug 9569

Summary: NULL pointer dereference on drivers/usb/host/ehci-q.c
Product: Drivers Reporter: Marcio Buss (marciobuss)
Component: USBAssignee: Greg Kroah-Hartman (greg)
Status: CLOSED CODE_FIX    
Severity: normal    
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 2.6.23 Subsystem:
Regression: --- Bisected commit-id:
Bug Depends on:    
Bug Blocks: 5089    
Attachments: proposed patch (against 2.6.24-rc5)

Description Marcio Buss 2007-12-14 19:24:38 UTC
There is a NULL pointer dereference at drivers/usb/host/ehci-q.c, line 742.
The pointer that is null at that line is urb->dev->tt. The reason for the
error is as follows:

(1) Assume the if-condition at line 663 is true
(2) assume the if-condition at line 668 is false
(3) assume the ?-condition at line 699 is false. Then it must be the case
    that "tt" is NULL. But this implies that urb->dev->tt is also NULL at
    line 683, which reads  "struct usb_tt *tt = urb->dev->tt;"

(4) the first disjunct at the "if" statement on line 731 evaluates to true
(5) the statement at line 742 executes with urb->dev->tt  NULL.
Comment 1 Anonymous Emailer 2007-12-14 21:47:05 UTC
Reply-To: akpm@linux-foundation.org

On Fri, 14 Dec 2007 19:24:38 -0800 (PST) bugme-daemon@bugzilla.kernel.org wrote:

> http://bugzilla.kernel.org/show_bug.cgi?id=9569
> 
>            Summary: NULL pointer dereference on drivers/usb/host/ehci-q.c
>            Product: Drivers
>            Version: 2.5
>      KernelVersion: 2.6.23
>           Platform: All
>         OS/Version: Linux
>               Tree: Mainline
>             Status: NEW
>           Severity: normal
>           Priority: P1
>          Component: USB
>         AssignedTo: greg@kroah.com
>         ReportedBy: marciobuss@gmail.com
> 
> 
> There is a NULL pointer dereference at drivers/usb/host/ehci-q.c, line 742.
> The pointer that is null at that line is urb->dev->tt. The reason for the
> error is as follows:
> 
> (1) Assume the if-condition at line 663 is true
> (2) assume the if-condition at line 668 is false
> (3) assume the ?-condition at line 699 is false. Then it must be the case
>     that "tt" is NULL. But this implies that urb->dev->tt is also NULL at
>     line 683, which reads  "struct usb_tt *tt = urb->dev->tt;"
> 
> (4) the first disjunct at the "if" statement on line 731 evaluates to true
> (5) the statement at line 742 executes with urb->dev->tt  NULL.
> 
Comment 2 Alan Stern 2007-12-15 08:26:53 UTC
On Fri, 14 Dec 2007, Andrew Morton wrote:

> On Fri, 14 Dec 2007 19:24:38 -0800 (PST) bugme-daemon@bugzilla.kernel.org
> wrote:
> 
> > http://bugzilla.kernel.org/show_bug.cgi?id=9569
> > 
> >            Summary: NULL pointer dereference on drivers/usb/host/ehci-q.c

> > There is a NULL pointer dereference at drivers/usb/host/ehci-q.c, line 742.
> > The pointer that is null at that line is urb->dev->tt. The reason for the
> > error is as follows:

Does changing the "||" in line 740 to "&&" fix the problem?  It 
certainly seems that the test should use an "and" rather than an "or".

It also seems that the first part of the test should be urb->dev->tt !=
NULL rather than !ehci_is_TDI(ehci).  After all, in the future there
might be sorts of controllers other than TDI's with built-in root-hub
TTs.

Alan Stern
Comment 3 David Brownell 2007-12-15 11:32:03 UTC
Created attachment 14054 [details]
proposed patch (against 2.6.24-rc5)

Looks to me like this is a longstanding bug in the root hub TT support.  See if this patch makes that work better.
Comment 4 Anonymous Emailer 2007-12-15 11:37:13 UTC
Reply-To: david-b@pacbell.net

> Does changing the "||" in line 740 to "&&" fix the problem?  It 
> certainly seems that the test should use an "and" rather than an "or".

No; when there's an external root hub, that field needs to be set.


> It also seems that the first part of the test should be urb->dev->tt !=
> NULL rather than !ehci_is_TDI(ehci).  After all, in the future there
> might be sorts of controllers other than TDI's with built-in root-hub
> TTs.

Unless they patented that obvious feature. :)

The quick'n'dirty patch is however "if (urb->dev->tt && ...) ... "
as you imply.  

- Dave