kern/44907: crash due to race in ehci.c

[gmcnutt%cradlepoint.com@localhost]

>Number:         44907
>Category:       kern
>Synopsis:       crash due to race in ehci.c
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Apr 26 21:20:01 +0000 2011
>Originator:     Gordon McNutt
>Release:        5.1
>Organization:
CradlePoint, Inc
>Environment:
NetBSD  5.1 NetBSD 5.1 (SR) #8: Tue Apr 26 15:01:01 MDT 2011  
gmcnutt@gmcnutt-desktop:/home/gmcnutt/svn/cpbsd/trunk/sys/arch/evbmips/compile/obj/SR
 evbmips
#
>Description:
There is a race condition in ehci.c between ehci_timeout and callers of
ehci_freex/ehci_allocx:

1. ehci_timeout() calls usb_add_task(), enqueueing xfer.abort_task
2. someone calls ehci_freex on the same xfer (its abort_task remains enqueued)
3. someone calls ehci_allocx and gets the same xfer, wiping it to zero
4. usb_task_thread subsequently runs and tries to use TAILQ_REMOVE(), which
   results in a 0 pointer deref

Yep, it really happens. The following suggested patch fixes it by always
initializing the abort_task in ehci_allocx and always removing it in
ehci_freex.
>How-To-Repeat:
I run lots of torrents concurrently over multiple usb modems. A couple of usb 
hubs are in the tree as well.
>Fix:
Index: sys/dev/usb/ehci.c
===================================================================
--- sys/dev/usb/ehci.c  (revision 4123)
+++ sys/dev/usb/ehci.c  (working copy)
@@ -1298,7 +1299,9 @@
                xfer = malloc(sizeof(struct ehci_xfer), M_USB, M_NOWAIT);
        }
        if (xfer != NULL) {
+               struct ehci_xfer *exfer = EXFER(xfer);
                memset(xfer, 0, sizeof(struct ehci_xfer));
+               usb_init_task(&exfer->abort_task, ehci_timeout_task, exfer);
 #ifdef DIAGNOSTIC
                EXFER(xfer)->isdone = 1;
                xfer->busy_free = XFER_BUSY;
@@ -1311,6 +1314,7 @@
 ehci_freex(struct usbd_bus *bus, usbd_xfer_handle xfer)
 {
        struct ehci_softc *sc = bus->hci_private;
+       struct ehci_xfer *exfer = EXFER(xfer);
 
 #ifdef DIAGNOSTIC
        if (xfer->busy_free != XFER_BUSY) {
@@ -1322,6 +1326,7 @@
                printf("ehci_freex: !isdone\n");
        }
 #endif
+       usb_rem_task(xfer->pipe->device, &exfer->abort_task);
        SIMPLEQ_INSERT_HEAD(&sc->sc_free_xfers, xfer, next);
 }
 
@@ -3115,7 +3120,6 @@
        }
 
        /* Execute the abort in a process context. */
-       usb_init_task(&exfer->abort_task, ehci_timeout_task, addr);
        usb_add_task(exfer->xfer.pipe->device, &exfer->abort_task,
            USB_TASKQ_HC);
 }