>Number:         24636
>Category:       kern
>Synopsis:       potential invalid memory access in usbd_transfer (not triggered in existing code)
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Mar 02 02:56:00 UTC 2004
>Closed-Date:
>Last-Modified:
>Originator:     darkstar@city-net.com
>Release:        NetBSD 1.6ZK
>Organization:
>Environment:
System: NetBSD jove.city-net.com 1.6ZK NetBSD 1.6ZK (JOVE) #2: Tue Feb 24 09:57:53 UTC 2004 root@jove.city-net.com:/usr/iobj/sys/arch/i386/compile/JOVE i386
Architecture: i386
Machine: i386
>Description:

  usbdi_transfer accesses xfer after calling the host controller to
determine if the transfer is synchronous.  However, if it is not
synchronous and the host controller does the transfer immediately anyway
and the callback frees the transfer structure, then this memory is no
longer valid.  I don't think this can happen in existing code, although it
might with slhci (if current slhci actually works with any revision of the
chip, which I doubt.  Even if it did, I don't think the invalid memory
could be overwritten by then and is not freed, so it would not cause a
problem).

>How-To-Repeat:

  I ran into this working on slhci, in particular adding a pcmcia
attachment for the Ratoc CF USB controller.  Currently slhci steals the
dma of the host bus to use the standard usb_mem functions even though it
does not itself use dma, however this cannot be done for pcmcia (and
should not be done for other buses).  I changed the alloc/free functions
to use malloc and free and fake the part of the dma info that is used.
Running DEBUG, this causes freed memory to be overwritten immedately.
When a usb keyboard is attached, the recently fixed ukbd_set_leds causes
the above described sequence to take place, causing usbd_transfer to
return 0xdeadbeef as error (because 0xdeadbeef matches USBD_SYNCHRONOUS),
which then causes usbd_do_request_async to try to free the memory again,
causing a fault in usbd_free_buffer (because 0xdeadeef matches
URQ_DEV_DMABUF).


>Fix:

  This fix is not necessary until the updated slhci driver is finished
(hopefully soon), but could be applied now anyway for correctness.  A
better fix would be to remove usbd/usbdi entirely and replace with a sane
HC-driver interface.

Index: sys/dev/usb/usbdi.c
===================================================================
RCS file: /cvsroot/src/sys/dev/usb/usbdi.c,v
retrieving revision 1.103
diff -u -r1.103 usbdi.c
--- sys/dev/usb/usbdi.c	27 Sep 2002 15:37:38 -0000	1.103
+++ sys/dev/usb/usbdi.c	2 Mar 2004 01:39:42 -0000
@@ -284,7 +284,7 @@
 	usb_dma_t *dmap = &xfer->dmabuf;
 	usbd_status err;
 	u_int size;
-	int s;
+	int s, sync;

 	DPRINTFN(5,("usbd_transfer: xfer=%p, flags=%d, pipe=%p, running=%d\n",
 		    xfer, xfer->flags, pipe, pipe->running));
@@ -317,6 +317,10 @@
 	    !usbd_xfer_isread(xfer))
 		memcpy(KERNADDR(dmap, 0), xfer->buffer, size);

+	/* If the transfer is not synchronous, xfer may have been freed in a
+	 * callback before transfer returns, so check for synchronous now. */
+	sync = xfer->flags & USBD_SYNCHRONOUS ? 1 : 0;
+
 	err = pipe->methods->transfer(xfer);

 	if (err != USBD_IN_PROGRESS && err) {
@@ -329,7 +333,7 @@
 		}
 	}

-	if (!(xfer->flags & USBD_SYNCHRONOUS))
+	if (!sync)
 		return (err);

 	/* Sync transfer, wait for completion. */
>Release-Note:
>Audit-Trail:
>Unformatted: