--- 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);