⬢ DragonFlyBSD Kernel Audit
DF-5288 / fix.diff
← back to finding ↓ download raw
--- a/sys/bus/u4b/usb_generic.c
+++ b/sys/bus/u4b/usb_generic.c
@@ -972,15 +972,33 @@
 int
 ugen_fs_uninit(struct usb_fifo *f)
 {
+	struct usb_xfer **fs_xfer;
+	uint8_t fs_ep_max;
+
+	/*
+	 * Detach the endpoint array under priv_lock BEFORE freeing it
+	 * (DF-5288): concurrent USB_FS_START/STOP/COMPLETE commands on
+	 * another thread sharing the same file descriptor re-read
+	 * f->fs_xfer[] under priv_lock and must never observe a
+	 * dangling array.
+	 */
+	lockmgr(f->priv_lock, LK_EXCLUSIVE);
+
 	if (f->fs_xfer == NULL) {
+		lockmgr(f->priv_lock, LK_RELEASE);
 		return (EINVAL);
 	}
-	usbd_transfer_unsetup(f->fs_xfer, f->fs_ep_max);
-	kfree(f->fs_xfer, M_USB);
+	fs_xfer = f->fs_xfer;
+	fs_ep_max = f->fs_ep_max;
 	f->fs_xfer = NULL;
 	f->fs_ep_max = 0;
 	f->fs_ep_ptr = NULL;
 	f->flag_iscomplete = 0;
+
+	lockmgr(f->priv_lock, LK_RELEASE);
+
+	usbd_transfer_unsetup(fs_xfer, fs_ep_max);
+	kfree(fs_xfer, M_USB);
 	usb_fifo_free_buffer(f);
 	return (0);
 }
@@ -1045,23 +1063,33 @@
 	int error;
 	uint8_t isread;
 
-	if (ep_index >= f->fs_ep_max) {
+	/*
+	 * DF-5288: acquire the FIFO lock, revalidate the array under it
+	 * and keep it held across the whole function (it is a sleepable
+	 * lockmgr lock), so that a concurrent USB_FS_UNINIT cannot free
+	 * "xfer" while we are installing transfer parameters into it.
+	 * Every exit path below releases the lock exactly once.
+	 */
+	lockmgr(f->priv_lock, LK_EXCLUSIVE);
+
+	if ((ep_index >= f->fs_ep_max) || (f->fs_xfer == NULL)) {
+		lockmgr(f->priv_lock, LK_RELEASE);
 		return (EINVAL);
 	}
 	xfer = f->fs_xfer[ep_index];
 	if (xfer == NULL) {
+		lockmgr(f->priv_lock, LK_RELEASE);
 		return (EINVAL);
 	}
-	lockmgr(f->priv_lock, LK_EXCLUSIVE);
 	if (usbd_transfer_pending(xfer)) {
 		lockmgr(f->priv_lock, LK_RELEASE);
 		return (EBUSY);		/* should not happen */
 	}
-	lockmgr(f->priv_lock, LK_RELEASE);
 
 	error = copyin(f->fs_ep_ptr +
 	    ep_index, &fs_ep, sizeof(fs_ep));
 	if (error) {
+		lockmgr(f->priv_lock, LK_RELEASE);
 		return (error);
 	}
 	/* security checks */
@@ -1098,6 +1126,7 @@
 		if (length != 0) {
 			error = copyin(uaddr, req, length);
 			if (error) {
+				lockmgr(f->priv_lock, LK_RELEASE);
 				return (error);
 			}
 		}
@@ -1159,7 +1188,7 @@
 		error = copyin(fs_ep.pLength + n,
 		    &length, sizeof(length));
 		if (error) {
-			break;
+			goto done_locked;
 		}
 		usbd_xfer_set_frame_len(xfer, n, length);
 
@@ -1175,7 +1204,7 @@
 			error = copyin(fs_ep.ppBuffer + n,
 			    &uaddr, sizeof(uaddr));
 			if (error) {
-				break;
+				goto done_locked;
 			}
 			if (xfer->flags_int.isochronous_xfr) {
 				/* get kernel buffer address */
@@ -1197,10 +1226,16 @@
 		}
 		offset += length;
 	}
+done_locked:
+	lockmgr(f->priv_lock, LK_RELEASE);
 	return (error);
 
 complete:
-	lockmgr(f->priv_lock, LK_EXCLUSIVE);
+	/*
+	 * DF-5288: every "goto complete" above happens while priv_lock
+	 * is held (validation sites) -- keep the completion event
+	 * generation under the lock like the original code did.
+	 */
 	ugen_fs_set_complete(f, ep_index);
 	lockmgr(f->priv_lock, LK_RELEASE);
 	return (0);
@@ -1223,23 +1258,27 @@
 	int error;
 	uint8_t isread;
 
-	if (ep_index >= f->fs_ep_max)
-		return (EINVAL);
+	/* DF-5288: see ugen_fs_copy_in() */
+	lockmgr(f->priv_lock, LK_EXCLUSIVE);
 
+	if ((ep_index >= f->fs_ep_max) || (f->fs_xfer == NULL)) {
+		lockmgr(f->priv_lock, LK_RELEASE);
+		return (EINVAL);
+	}
 	xfer = f->fs_xfer[ep_index];
-	if (xfer == NULL)
+	if (xfer == NULL) {
+		lockmgr(f->priv_lock, LK_RELEASE);
 		return (EINVAL);
-
-	lockmgr(f->priv_lock, LK_EXCLUSIVE);
+	}
 	if (usbd_transfer_pending(xfer)) {
 		lockmgr(f->priv_lock, LK_RELEASE);
 		return (EBUSY);		/* should not happen */
 	}
-	lockmgr(f->priv_lock, LK_RELEASE);
 
 	fs_ep_uptr = f->fs_ep_ptr + ep_index;
 	error = copyin(fs_ep_uptr, &fs_ep, sizeof(fs_ep));
 	if (error) {
+		lockmgr(f->priv_lock, LK_RELEASE);
 		return (error);
 	}
 	fs_ep.status = xfer->error;
@@ -1282,7 +1321,7 @@
 		error = copyin(fs_ep.pLength + n,
 		    &temp, sizeof(temp));
 		if (error) {
-			return (error);
+			goto done_locked;
 		}
 		if (temp > rem) {
 			/* the userland length has been corrupted */
@@ -1308,7 +1347,7 @@
 			error = copyin(fs_ep.ppBuffer + n,
 			    &uaddr, sizeof(uaddr));
 			if (error) {
-				return (error);
+				goto done_locked;
 			}
 			if (xfer->flags_int.isochronous_xfr) {
 				/* only one frame buffer */
@@ -1322,7 +1361,7 @@
 			/* move data */
 			error = copyout(kaddr, uaddr, length);
 			if (error) {
-				return (error);
+				goto done_locked;
 			}
 		}
 		/*
@@ -1335,11 +1374,21 @@
 		error = copyout(&length,
 		    fs_ep.pLength + n, sizeof(length));
 		if (error) {
-			return (error);
+			goto done_locked;
 		}
 	}
 
+done_locked:
+	lockmgr(f->priv_lock, LK_RELEASE);
+
 complete:
+	/*
+	 * DF-5288: paths that jump here directly from the validation
+	 * section above still hold priv_lock; release it exactly once.
+	 */
+	if (lockowned(f->priv_lock))
+		lockmgr(f->priv_lock, LK_RELEASE);
+
 	/* update "aFrames" */
 	error = copyout(&fs_ep.aFrames, &fs_ep_uptr->aFrames,
 	    sizeof(fs_ep.aFrames));
@@ -1426,6 +1475,13 @@
 		if (error)
 			break;
 		lockmgr(f->priv_lock, LK_EXCLUSIVE);
+		/* DF-5288: revalidate under the lock */
+		if ((u.pstart->ep_index >= f->fs_ep_max) ||
+		    (f->fs_xfer == NULL)) {
+			lockmgr(f->priv_lock, LK_RELEASE);
+			error = EINVAL;
+			break;
+		}
 		xfer = f->fs_xfer[u.pstart->ep_index];
 		usbd_transfer_start(xfer);
 		lockmgr(f->priv_lock, LK_RELEASE);
@@ -1437,6 +1493,13 @@
 			break;
 		}
 		lockmgr(f->priv_lock, LK_EXCLUSIVE);
+		/* DF-5288: revalidate under the lock */
+		if ((u.pstop->ep_index >= f->fs_ep_max) ||
+		    (f->fs_xfer == NULL)) {
+			lockmgr(f->priv_lock, LK_RELEASE);
+			error = EINVAL;
+			break;
+		}
 		xfer = f->fs_xfer[u.pstart->ep_index];
 		if (usbd_transfer_pending(xfer)) {
 			usbd_transfer_stop(xfer);