NetBSD Problem Report #24636

Received: (qmail 6893 invoked by uid 605); 2 Mar 2004 02:55:37 -0000
Message-Id: <Pine.BSF.4.51.0403012134050.23469@vegeta.city-net.com>
Date: Mon, 1 Mar 2004 21:48:32 -0500 (EST)
From: Matthew Orgass <darkstar@city-net.com>
Sender: gnats-bugs-owner@NetBSD.org
To: gnats-bugs@netbsd.org
Subject: potential invalid memory access in usbd_transfer (not triggered in existing code)

>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:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Mar 02 02:56:00 +0000 2004
>Closed-Date:    Sun Mar 08 09:15:54 +0000 2015
>Last-Modified:  Sun Mar 08 09:15:54 +0000 2015
>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:

From: Matthew Orgass <darkstar@city-net.com>
To: gnats-bugs@netbsd.org
Cc:  
Subject: Re: kern/24636: potential invalid memory access in usbd_transfer
 (not triggered in existing code)
Date: Tue, 2 Mar 2004 16:09:29 -0500 (EST)

   It seems usb_transfer_complete calls the pipe done function after the
 callback and otherwise uses the xfer and pipe structures after the
 callback.  I don't need to do anything in the done method for slhci, so as
 long as pipe remains valid the following patch should avoid trouble for
 now.  I don't know if a pipe is ever closed in a callback.

 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 21:09:15 -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. */
 @@ -759,7 +763,8 @@
  	usbd_pipe_handle pipe = xfer->pipe;
  	usb_dma_t *dmap = &xfer->dmabuf;
  	int repeat = pipe->repeat;
 -	int polling;
 +	int polling, sync;
 +	usbd_status status;

  	SPLUSBCHECK;

 @@ -830,6 +835,9 @@
  		xfer->status = USBD_SHORT_XFER;
  	}

 +	status = xfer->status;
 +	sync = xfer->flags & USBD_SYNCHRONOUS ? 1 : 0;
 +
  	if (xfer->callback)
  		xfer->callback(xfer, xfer->priv, xfer->status);

 @@ -842,13 +850,12 @@
  	pipe->methods->done(xfer);
  #endif

 -	if ((xfer->flags & USBD_SYNCHRONOUS) && !polling)
 +	if (sync && !polling)
  		wakeup(xfer);

  	if (!repeat) {
  		/* XXX should we stop the queue on all errors? */
 -		if ((xfer->status == USBD_CANCELLED ||
 -		     xfer->status == USBD_TIMEOUT) &&
 +		if ((status == USBD_CANCELLED || status == USBD_TIMEOUT) &&
  		    pipe->iface != NULL)		/* not control pipe */
  			pipe->running = 0;
  		else
From: Matthew Orgass <darkstar@city-net.com>
To: Iain Hibbert <plunky@rya-online.net>
Cc: tech-kern@NetBSD.org, gnats-bugs@netbsd.org
Subject: kern/24636 (Re: kern/25041: USB isochronous transfers)
Date: Thu, 4 May 2006 09:52:10 -0400 (EDT)

 On 2006-05-03 plunky@rya-online.net wrote:

 >    4. http://www.netbsd.org/cgi-bin/query-pr-single.pl?number=24636
 >       "potential invalid memory access"
 >
 > 	Another one that could probably be closed? the changes mentioned
 > here have already been actioned and he mentions the issue of the done
 > method touching the xfer though not quite in this context.

   The first patch in this PR has not been applied yet (usbd_transfer uses
 xfer after the transfer even when not synchronous).

 Matthew Orgass
 darkstar@city-net.com

From: Iain Hibbert <plunky@rya-online.net>
To: Matthew Orgass <darkstar@city-net.com>
Cc: tech-kern@NetBSD.org, gnats-bugs@netbsd.org
Subject: Re: kern/24636 (Re: kern/25041: USB isochronous transfers)
Date: Thu, 4 May 2006 17:26:34 +0100 (BST)

 On Thu, 4 May 2006, Matthew Orgass wrote:

 > On 2006-05-03 plunky@rya-online.net wrote:
 >
 > >    4. http://www.netbsd.org/cgi-bin/query-pr-single.pl?number=24636
 > >       "potential invalid memory access"
 > >
 > > 	Another one that could probably be closed? the changes mentioned
 > > here have already been actioned and he mentions the issue of the done
 > > method touching the xfer though not quite in this context.
 >
 >   The first patch in this PR has not been applied yet (usbd_transfer uses
 > xfer after the transfer even when not synchronous).

 Oops yes, my mistake sorry .. I think that really, if somebody were to go
 at this then it would be beneficial to rearrange the methodology such that
 the callback was the last thing to be done and that it could not be called
 before usbd_transfer returned.

 I found it a bit strange to see that usbd_transfer can return
 NORMAL_COMPLETION for asynchronous requests (for isoc anyway, I've not
 seen it on a bulk pipe)

 iain

From: Matthew Orgass <darkstar@city-net.com>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: kern/24636 potential invalid memory access in usbd_transfer
Date: Tue, 21 Nov 2006 12:07:08 -0500 (EST)

  Improved patch:

 Index: sys/dev/usb/usbdi.c
 ===================================================================
 RCS file: /cvsroot/src/sys/dev/usb/usbdi.c,v
 retrieving revision 1.113
 diff -u -p -r1.113 usbdi.c
 --- sys/dev/usb/usbdi.c	12 Oct 2006 01:32:00 -0000	1.113
 +++ sys/dev/usb/usbdi.c	21 Nov 2006 17:43:39 -0000
 @@ -284,10 +284,10 @@ usbd_transfer(usbd_xfer_handle xfer)
  	usbd_pipe_handle pipe = xfer->pipe;
  	usb_dma_t *dmap = &xfer->dmabuf;
  	usbd_status err;
 -	u_int size;
 +	unsigned int size, flags;
  	int s;

 -	DPRINTFN(5,("usbd_transfer: xfer=%p, flags=%d, pipe=%p, running=%d\n",
 +	DPRINTFN(5,("usbd_transfer: xfer=%p, flags=%#x, pipe=%p, running=%d\n",
  		    xfer, xfer->flags, pipe, pipe->running));
  #ifdef USB_DEBUG
  	if (usbdebug > 5)
 @@ -313,11 +313,13 @@ usbd_transfer(usbd_xfer_handle xfer)
  		xfer->rqflags |= URQ_AUTO_DMABUF;
  	}

 +	flags = xfer->flags;
 +
  	/* Copy data if going out. */
 -	if (!(xfer->flags & USBD_NO_COPY) && size != 0 &&
 -	    !usbd_xfer_isread(xfer))
 +	if (!(flags & USBD_NO_COPY) && size != 0 && !usbd_xfer_isread(xfer))
  		memcpy(KERNADDR(dmap, 0), xfer->buffer, size);

 +	/* xfer is not valid after the transfer method unless synchronous */
  	err = pipe->methods->transfer(xfer);

  	if (err != USBD_IN_PROGRESS && err) {
 @@ -330,7 +332,7 @@ usbd_transfer(usbd_xfer_handle xfer)
  		}
  	}

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

  	/* Sync transfer, wait for completion. */

State-Changed-From-To: open->closed
State-Changed-By: skrll@NetBSD.org
State-Changed-When: Sun, 08 Mar 2015 09:15:54 +0000
State-Changed-Why:
Fixed with
Module Name:	src
Committed By:	kiyohara
Date:		Wed Aug 15 04:00:35 UTC 2007

Modified Files:
	src/sys/dev/usb: ehci.c ohci.c uhci.c usb.c usbdi.c usbdi.h

Log Message:
* splsoftusb, IPL_SOFTUSB, and IPL_HARDUSB defines in usbdi.h
-> the current names are confusing (didn't change other drivers)
* fix invalid memory access in usbd_transfer (kern/24636)
-> needed for this driver
* fix USB HC detach race condition (kern/32011)
-> main patch needed for this driver, sc_dying changes in other drivers
not necessary but seem right to me

Patch from Matthew Orgass.
  http://mail-index.netbsd.org/tech-kern/2007/06/26/0001.html


To generate a diff of this commit:
cvs rdiff -r1.123 -r1.124 src/sys/dev/usb/ehci.c
cvs rdiff -r1.182 -r1.183 src/sys/dev/usb/ohci.c
cvs rdiff -r1.209 -r1.210 src/sys/dev/usb/uhci.c
cvs rdiff -r1.98 -r1.99 src/sys/dev/usb/usb.c
cvs rdiff -r1.119 -r1.120 src/sys/dev/usb/usbdi.c
cvs rdiff -r1.74 -r1.75 src/sys/dev/usb/usbdi.h


>Unformatted:

Home	PR Database Search