If a zero-length bulk or interrupt transfer is requested then assume

USBD_FORCE_SHORT_XFER to ensure that we actually build and execute
a transfer. This means that the various alloc_sqtd_chain functions
will always construct a transfer, so it is safe to modify the
allocated descriptors on return. Previously there were cases where
a zero length transfer would cause a NULL dereference.

Reported by:	bp

3 files changed, 17 insertions, 18 deletions

diff --git a/sys/dev/usb/ehci.c b/sys/dev/usb/ehci.c
index addf576..71d4061 100644
--- a/sys/dev/usb/ehci.c
+++ b/sys/dev/usb/ehci.c
@@ -2354,7 +2354,8 @@ ehci_alloc_sqtd_chain(struct ehci_pipe *epipe, ehci_softc_t *sc,
 	    /* BYTES set below */
 	    ;
 	mps = UGETW(epipe->pipe.endpoint->edesc->wMaxPacketSize);
-	forceshort = (xfer->flags & USBD_FORCE_SHORT_XFER) && len % mps == 0;
+	forceshort = ((xfer->flags & USBD_FORCE_SHORT_XFER) || len == 0) &&
+	    len % mps == 0;
 	/*
 	 * The control transfer data stage always starts with a toggle of 1.
 	 * For other transfers we let the hardware track the toggle state.
diff --git a/sys/dev/usb/ohci.c b/sys/dev/usb/ohci.c
index c8e6b17..7156f7e 100644
--- a/sys/dev/usb/ohci.c
+++ b/sys/dev/usb/ohci.c
@@ -511,7 +511,7 @@ ohci_alloc_std_chain(struct ohci_pipe *opipe, ohci_softc_t *sc,
 		     int alen, int rd, usbd_xfer_handle xfer,
 		     ohci_soft_td_t *sp, ohci_soft_td_t **ep)
 {
-	ohci_soft_td_t *next, *cur;
+	ohci_soft_td_t *next, *cur, *end;
 	ohci_physaddr_t dataphys, physend;
 	u_int32_t tdflags;
 	int offset = 0;
@@ -523,6 +523,7 @@ ohci_alloc_std_chain(struct ohci_pipe *opipe, ohci_softc_t *sc,
 
 	len = alen;
 	cur = sp;
+	end = NULL;
 
 	maxp = UGETW(opipe->pipe.endpoint->edesc->wMaxPacketSize);
 	tdflags = htole32(
@@ -532,7 +533,7 @@ ohci_alloc_std_chain(struct ohci_pipe *opipe, ohci_softc_t *sc,
 
 	seg = 0;
 	segoff = 0;
-	for (;;) {
+	while (len > 0) {
 		next = ohci_alloc_std(sc);
 		if (next == NULL)
 			goto nomem;
@@ -624,21 +625,17 @@ ohci_alloc_std_chain(struct ohci_pipe *opipe, ohci_softc_t *sc,
 		cur->xfer = xfer;
 		DPRINTFN(10,("ohci_alloc_std_chain: cbp=0x%08x be=0x%08x\n",
 			    dataphys, dataphys + curlen - 1));
-		if (len == 0)
-			break;
 		if (len < 0)
 			panic("Length went negative: %d curlen %d dma %p offset %08x", len, curlen, dma, (int)0);
 
 		DPRINTFN(10,("ohci_alloc_std_chain: extend chain\n"));
 		offset += curlen;
+		end = cur;
 		cur = next;
 	}
-	if ((flags & USBD_FORCE_SHORT_XFER) &&
+	if (((flags & USBD_FORCE_SHORT_XFER) || alen == 0) &&
 	    alen % UGETW(opipe->pipe.endpoint->edesc->wMaxPacketSize) == 0) {
 		/* Force a 0 length transfer at the end. */
-
-		cur = next;
-
 		next = ohci_alloc_std(sc);
 		if (next == NULL)
 			goto nomem;
@@ -652,8 +649,9 @@ ohci_alloc_std_chain(struct ohci_pipe *opipe, ohci_softc_t *sc,
 		cur->flags = 0;
 		cur->xfer = xfer;
 		DPRINTFN(2,("ohci_alloc_std_chain: add 0 xfer\n"));
+		end = cur;
 	}
-	*ep = cur;
+	*ep = end;
 
 	return (USBD_NORMAL_COMPLETION);
 
diff --git a/sys/dev/usb/uhci.c b/sys/dev/usb/uhci.c
index aee88b9..d7cbdb7 100644
--- a/sys/dev/usb/uhci.c
+++ b/sys/dev/usb/uhci.c
@@ -1766,14 +1766,12 @@ uhci_alloc_std_chain(struct uhci_pipe *upipe, uhci_softc_t *sc, int len,
 		return (USBD_INVAL);
 	}
 	ntd = (len + maxp - 1) / maxp;
+	if (len == 0)
+		flags |= USBD_FORCE_SHORT_XFER;
 	if ((flags & USBD_FORCE_SHORT_XFER) && len % maxp == 0)
 		ntd++;
 	DPRINTFN(10, ("uhci_alloc_std_chain: maxp=%d ntd=%d\n", maxp, ntd));
-	if (ntd == 0) {
-		*sp = *ep = 0;
-		DPRINTFN(-1,("uhci_alloc_std_chain: ntd=0\n"));
-		return (USBD_NORMAL_COMPLETION);
-	}
+	KASSERT(ntd > 0, ("uhci_alloc_std_chain: ntd=0"));
 	tog = upipe->nexttoggle;
 	prevp = NULL;
 	startp = NULL;
@@ -1811,9 +1809,11 @@ uhci_alloc_std_chain(struct uhci_pipe *upipe, uhci_softc_t *sc, int len,
 		    htole32(rd ? UHCI_TD_IN (l, endpt, addr, tog) :
 				 UHCI_TD_OUT(l, endpt, addr, tog));
 
-		KASSERT(seg < dma->nsegs,
+		KASSERT(seg < dma->nsegs || l == 0,
 		    ("uhci_alloc_std_chain: too few segments"));
-		if (l > dma->segs[seg].ds_len - segoff) {
+		if (l == 0) {
+			p->td.td_buffer = 0;
+		} else if (l > dma->segs[seg].ds_len - segoff) {
 			/* UHCI can't handle non-contiguous data. */
 			err = uhci_aux_dma_alloc(sc, p, (char *)xfer->buffer +
 			    i * maxp, l);
@@ -1832,7 +1832,7 @@ uhci_alloc_std_chain(struct uhci_pipe *upipe, uhci_softc_t *sc, int len,
 			    segoff);
 		}
 		segoff += l;
-		if (segoff >= dma->segs[seg].ds_len) {
+		if (l > 0 && segoff >= dma->segs[seg].ds_len) {
 			KASSERT(segoff == dma->segs[seg].ds_len,
 			    ("uhci_alloc_std_chain: overlap"));
 			if (i * maxp + l != len) {