usb/126884: [patch] Bug in buffer handling in ugen.c

[Daan Vreeken [PA4DAN]]

>Number:         126884
>Category:       usb
>Synopsis:       [patch] Bug in buffer handling in ugen.c
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    freebsd-usb
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Wed Aug 27 11:40:02 UTC 2008
>Closed-Date:
>Last-Modified:
>Originator:     Daan Vreeken [PA4DAN]
>Release:        FreeBSD 7.0-STABLE amd64
>Organization:
VEHosting.nl
>Environment:
System: FreeBSD TVlottePert.VEHosting.nl 7.0-STABLE FreeBSD 7.0-STABLE #0: Fri Jun 13 18:02:09 CEST 2008 root at TVlottePert.VEHosting.nl:/usr/obj/usr/src/sys/GENERIC amd64


	
>Description:
	In ugen_isoc_rintr() old data is thrown away when the fifo is full.
	This code contains multiple errors :
	o Only in the case of sce->fill < sce->cur data is thrown away.
	  (No data is discarded when sce->fill is near the end of the buffer
	  and sce->cur is near the beginning of the buffer.)
	o Too much data is discarded. If 10 bytes need to be written with
	  only space left for 8 bytes, 10 bytes of input are discarded instead
	  of just 2.
	o When sce->cur reaches/passes the end of the buffer, the pointer is
	  set to sce->ibuf + (sce->limit - sce->cur) instead of to
	  sce->ibuf + (sce->cur - sce->limit). This could leave the sce->cur
	  pointer ahead of the buffer pointing to unknown memory.

>How-To-Repeat:
	Try to use the ugen interface to read data from a device with an
	isochronous endpoint that delivers isochronous packets of varying
	length and see that data gets corrupted.
	
>Fix:
	The attached patch is to -CURRENT, but the bug is found in other
	branches too.
	The patch moves the deletion of data into the while loop that copies
	data from the incoming packet. This way data is also discarded in the
	case where sce->fill >= sce->cur initially (because the while loop
	already breaks filling the end & beginning of the buffer into two
	itterations).

	

--- ugen.c.patch begins here ---
--- ugen.c-1.112	2008-08-27 11:48:37.000000000 +0200
+++ ugen.c	2008-08-27 12:08:10.000000000 +0200
@@ -1054,15 +1054,6 @@
 	DPRINTFN(5,("ugen_isoc_rintr: xfer %td, count=%d\n", req - sce->isoreqs,
 		    count));
 
-	/* throw away oldest input if the buffer is full */
-	if(sce->fill < sce->cur && sce->cur <= sce->fill + count) {
-		sce->cur += count;
-		if(sce->cur >= sce->limit)
-			sce->cur = sce->ibuf + (sce->limit - sce->cur);
-		DPRINTFN(5, ("ugen_isoc_rintr: throwing away %d bytes\n",
-			     count));
-	}
-
 	isize = UGETW(sce->edesc->wMaxPacketSize);
 	for (i = 0; i < UGEN_NISORFRMS; i++) {
 		u_int32_t actlen = req->sizes[i];
@@ -1071,6 +1062,22 @@
 		/* copy data to buffer */
 		while (actlen > 0) {
 			n = min(actlen, sce->limit - sce->fill);
+
+			/* throw away oldest input if the buffer is full */
+			if(sce->fill < sce->cur &&
+			    sce->cur <= sce->fill + n) {
+				u_int32_t drop;
+				
+				/* calculate fow many bytes we have to drop */
+				drop = scr->cur - sce->fill;
+
+				sce->cur += drop;
+				if(sce->cur >= sce->limit)
+					sce->cur = sce->ibuf;
+				DPRINTFN(5, ("ugen_isoc_rintr: throwing "
+				    "away %d bytes\n", drop));
+			}
+
 			memcpy(sce->fill, buf, n);
 
 			buf += n;
--- ugen.c.patch ends here ---


>Release-Note:
>Audit-Trail:
>Unformatted: