From 135ff0532b1eee5a6edac4198dc40034c21fcf81 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 3 Sep 2021 15:05:00 +0000 Subject: [PATCH 1/9] ugen(4): Sprinkle KERNEL_LOCKED_P assertions around sc_is_open. --- sys/dev/usb/ugen.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/sys/dev/usb/ugen.c b/sys/dev/usb/ugen.c index bb84b23cc7d1..a0699768b8c4 100644 --- a/sys/dev/usb/ugen.c +++ b/sys/dev/usb/ugen.c @@ -441,6 +441,8 @@ ugen_set_config(struct ugen_softc *sc, int configno, int chkopen) DPRINTFN(1,("ugen_set_config: %s to configno %d, sc=%p\n", device_xname(sc->sc_dev), configno, sc)); + KASSERT(KERNEL_LOCKED_P()); /* sc_is_open */ + if (chkopen) { /* * We start at 1, not 0, because we don't care whether the @@ -509,6 +511,8 @@ ugenopen(dev_t dev, int flag, int mode, struct lwp *l) int i, j; int error; + KASSERT(KERNEL_LOCKED_P()); /* sc_is_open */ + if ((sc = ugenif_acquire(unit)) == NULL) return ENXIO; @@ -675,6 +679,8 @@ ugenclose(dev_t dev, int flag, int mode, struct lwp *l) int i; int error; + KASSERT(KERNEL_LOCKED_P()); /* sc_is_open */ + if ((sc = ugenif_acquire(UGENUNIT(dev))) == NULL) return ENXIO; @@ -1530,6 +1536,8 @@ ugen_do_ioctl(struct ugen_softc *sc, int endpt, u_long cmd, int error; int dir; + KASSERT(KERNEL_LOCKED_P()); /* ugen_set_config */ + DPRINTFN(5, ("ugenioctl: cmd=%08lx\n", cmd)); switch (cmd) { -- 2.28.0 From 10ea7c6638ac5a1bc4b42a486a7d2592a1731f18 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 3 Sep 2021 15:06:27 +0000 Subject: [PATCH 2/9] ugen(4): Fix race of ugenopen against itself. Even though we have the kernel lock held, a sleep during kmem_alloc or usbd_open_pipe could allow another ugenopen to run concurrently before we have marked the endpoint opened. To avoid this, mark the endpoint open immediately (while we still have the kernel lock held and before any sleeps, so there is no TOCTOU error here), and then revert on unwind in the event of failure. --- sys/dev/usb/ugen.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/sys/dev/usb/ugen.c b/sys/dev/usb/ugen.c index a0699768b8c4..24a2b3ac532a 100644 --- a/sys/dev/usb/ugen.c +++ b/sys/dev/usb/ugen.c @@ -510,6 +510,7 @@ ugenopen(dev_t dev, int flag, int mode, struct lwp *l) struct usbd_xfer *xfer; int i, j; int error; + int opened; KASSERT(KERNEL_LOCKED_P()); /* sc_is_open */ @@ -521,7 +522,7 @@ ugenopen(dev_t dev, int flag, int mode, struct lwp *l) /* The control endpoint allows multiple opens. */ if (endpt == USB_CONTROL_ENDPOINT) { - sc->sc_is_open[USB_CONTROL_ENDPOINT] = 1; + opened = sc->sc_is_open[USB_CONTROL_ENDPOINT] = 1; error = 0; goto out; } @@ -530,6 +531,7 @@ ugenopen(dev_t dev, int flag, int mode, struct lwp *l) error = EBUSY; goto out; } + opened = sc->sc_is_open[endpt] = 1; /* Make sure there are pipes for all directions. */ for (dir = OUT; dir <= IN; dir++) { @@ -663,9 +665,10 @@ ugenopen(dev_t dev, int flag, int mode, struct lwp *l) goto out; } } - sc->sc_is_open[endpt] = 1; error = 0; -out: ugenif_release(sc); +out: if (error && opened) + sc->sc_is_open[endpt] = 0; + ugenif_release(sc); return error; } -- 2.28.0 From 34417c7961869b9532cd655a9d7be8cf33a0c7e4 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 7 Sep 2021 10:20:12 +0000 Subject: [PATCH 3/9] ugen(4): Prevent ugenopen while ugen_set_config is in progress. (except on the control endpoint) Although we hold the kernel lock (which we should eventually change), we may sleep in usbd_set_config_no at which point ugenopen might happen and start making use of endpoint state which we'll stomp all over once usbd_set_config_no returns. Setting sc_is_open[endpt] while we wait prevents this. --- sys/dev/usb/ugen.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/sys/dev/usb/ugen.c b/sys/dev/usb/ugen.c index 24a2b3ac532a..f24c21f18da9 100644 --- a/sys/dev/usb/ugen.c +++ b/sys/dev/usb/ugen.c @@ -455,6 +455,12 @@ ugen_set_config(struct ugen_softc *sc, int configno, int chkopen) device_xname(sc->sc_dev), endptno)); return USBD_IN_USE; } + + /* Prevent opening while we're setting the config. */ + for (endptno = 1; endptno < USB_MAX_ENDPOINTS; endptno++) { + KASSERT(!sc->sc_is_open[endptno]); + sc->sc_is_open[endptno] = 1; + } } /* Avoid setting the current value. */ @@ -462,23 +468,23 @@ ugen_set_config(struct ugen_softc *sc, int configno, int chkopen) if (!cdesc || cdesc->bConfigurationValue != configno) { err = usbd_set_config_no(dev, configno, 1); if (err) - return err; + goto out; } ugen_clear_endpoints(sc); err = usbd_interface_count(dev, &niface); if (err) - return err; + goto out; for (ifaceno = 0; ifaceno < niface; ifaceno++) { DPRINTFN(1,("ugen_set_config: ifaceno %d\n", ifaceno)); err = usbd_device2interface_handle(dev, ifaceno, &iface); if (err) - return err; + goto out; err = usbd_endpoint_count(iface, &nendpt); if (err) - return err; + goto out; for (endptno = 0; endptno < nendpt; endptno++) { ed = usbd_interface2endpoint_descriptor(iface,endptno); KASSERT(ed != NULL); @@ -494,7 +500,19 @@ ugen_set_config(struct ugen_softc *sc, int configno, int chkopen) sce->iface = iface; } } - return USBD_NORMAL_COMPLETION; + err = USBD_NORMAL_COMPLETION; + +out: if (chkopen) { + /* + * Allow open again now that we're done trying to set + * the config. + */ + for (endptno = 1; endptno < USB_MAX_ENDPOINTS; endptno++) { + KASSERT(sc->sc_is_open[endptno]); + sc->sc_is_open[endptno] = 0; + } + } + return err; } static int -- 2.28.0 From aaa778b56d8af9a75cd139efc8e93fa402f5429a Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 3 Sep 2021 15:08:25 +0000 Subject: [PATCH 4/9] ugen(4): Refuse non-forced detach with EBUSY if endpoints are open. Sprinkle some comments. --- sys/dev/usb/ugen.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/sys/dev/usb/ugen.c b/sys/dev/usb/ugen.c index f24c21f18da9..66fb9fe3d04f 100644 --- a/sys/dev/usb/ugen.c +++ b/sys/dev/usb/ugen.c @@ -1167,9 +1167,27 @@ ugen_detach(device_t self, int flags) DPRINTF(("ugen_detach: sc=%p flags=%d\n", sc, flags)); + KASSERT(KERNEL_LOCKED_P()); /* sc_is_open */ + + /* + * Fail if we're not forced to detach and userland has any + * endpoints open. + */ + if ((flags & DETACH_FORCE) == 0) { + for (i = 0; i < USB_MAX_ENDPOINTS; i++) { + if (sc->sc_is_open[i]) + return EBUSY; + } + } + + /* Prevent new users. Prevent suspend/resume. */ sc->sc_dying = 1; pmf_device_deregister(self); + /* + * If we never finished attaching, skip nixing endpoints and + * users because there aren't any. + */ if (!sc->sc_attached) goto out; -- 2.28.0 From a2e694b611bb18c315360a755507531beba13b9d Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sat, 4 Sep 2021 09:57:49 +0000 Subject: [PATCH 5/9] ugen(4): Ensure we close pipes on detach. Calling vdevgone has the effect of VOP_REVOKE -> spec_node_revoke -> VOP_CLOSE -> spec_close -> cdev_close -> ugenclose, but: (a) the flags passed to VOP_CLOSE don't have FREAD or FWRITE, and (b) ugenclose has no effect when sc_dying is set anyway. So create another path into the close logic, ugen_do_close. We have to do this in detach _after_ the references have drained because we may free buffers that other users are still using while the reference count is nonzero. --- sys/dev/usb/ugen.c | 60 +++++++++++++++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 19 deletions(-) diff --git a/sys/dev/usb/ugen.c b/sys/dev/usb/ugen.c index 66fb9fe3d04f..77c82a8a12cd 100644 --- a/sys/dev/usb/ugen.c +++ b/sys/dev/usb/ugen.c @@ -690,30 +690,25 @@ out: if (error && opened) return error; } -static int -ugenclose(dev_t dev, int flag, int mode, struct lwp *l) +static void +ugen_do_close(struct ugen_softc *sc, int flag, int endpt) { - int endpt = UGENENDPOINT(dev); - struct ugen_softc *sc; struct ugen_endpoint *sce; int dir; int i; - int error; KASSERT(KERNEL_LOCKED_P()); /* sc_is_open */ - if ((sc = ugenif_acquire(UGENUNIT(dev))) == NULL) - return ENXIO; - - DPRINTFN(5, ("ugenclose: flag=%d, mode=%d, unit=%d, endpt=%d\n", - flag, mode, UGENUNIT(dev), endpt)); - - KASSERT(sc->sc_is_open[endpt]); + if (!sc->sc_is_open[endpt]) { + KASSERT(sc->sc_endpoints[endpt][IN].pipeh == NULL); + KASSERT(sc->sc_endpoints[endpt][OUT].pipeh == NULL); + KASSERT(sc->sc_endpoints[endpt][IN].ibuf == NULL); + KASSERT(sc->sc_endpoints[endpt][OUT].ibuf == NULL); + return; + } if (endpt == USB_CONTROL_ENDPOINT) { DPRINTFN(5, ("ugenclose: close control\n")); - sc->sc_is_open[endpt] = 0; - error = 0; goto out; } @@ -758,11 +753,31 @@ ugenclose(dev_t dev, int flag, int mode, struct lwp *l) sce->ibuf = NULL; } } - sc->sc_is_open[endpt] = 0; - error = 0; -out: ugenif_release(sc); - return error; +out: sc->sc_is_open[endpt] = 0; +} + +static int +ugenclose(dev_t dev, int flag, int mode, struct lwp *l) +{ + int endpt = UGENENDPOINT(dev); + struct ugen_softc *sc; + + DPRINTFN(5, ("ugenclose: flag=%d, mode=%d, unit=%d, endpt=%d\n", + flag, mode, UGENUNIT(dev), endpt)); + + KASSERT(KERNEL_LOCKED_P()); /* ugen_do_close */ + + if ((sc = ugenif_acquire(UGENUNIT(dev))) == NULL) + return ENXIO; + + KASSERT(sc->sc_is_open[endpt]); + ugen_do_close(sc, flag, endpt); + KASSERT(!sc->sc_is_open[endpt]); + + ugenif_release(sc); + + return 0; } Static int @@ -1214,10 +1229,17 @@ ugen_detach(device_t self, int flags) /* locate the major number */ maj = cdevsw_lookup_major(&ugen_cdevsw); - /* Nuke the vnodes for any open instances (calls close). */ + /* + * Nuke the vnodes for any open instances (calls ugenclose, but + * with no effect because we already set sc_dying). + */ mn = sc->sc_unit * USB_MAX_ENDPOINTS; vdevgone(maj, mn, mn + USB_MAX_ENDPOINTS - 1, VCHR); + /* Actually close any lingering pipes. */ + for (i = 0; i < USB_MAX_ENDPOINTS; i++) + ugen_do_close(sc, FREAD|FWRITE, i); + usbd_add_drv_event(USB_EVENT_DRIVER_DETACH, sc->sc_udev, sc->sc_dev); ugenif_put_unit(sc); -- 2.28.0 From 3fa2cd77ae85cde733cc5011d3e1665d2cbc3905 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sat, 4 Sep 2021 10:08:16 +0000 Subject: [PATCH 6/9] ugen(4): Issue explicit wakeup on detach for OUT endpoints too. Writers can be blocked in cv_timedwait_sig too. While here, fix comment: aborting the pipes does not cause all waiters to wake, because the xfer completion callbacks sometimes skip the notification. We should maybe change that, but this is a simpler fix to ensure everyone waiting on I/O is woken to notice sc_dying. --- sys/dev/usb/ugen.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/sys/dev/usb/ugen.c b/sys/dev/usb/ugen.c index 77c82a8a12cd..00c39dc8f486 100644 --- a/sys/dev/usb/ugen.c +++ b/sys/dev/usb/ugen.c @@ -1206,7 +1206,7 @@ ugen_detach(device_t self, int flags) if (!sc->sc_attached) goto out; - /* Abort all pipes. Causes processes waiting for transfer to wake. */ + /* Abort all pipes. */ for (i = 0; i < USB_MAX_ENDPOINTS; i++) { for (dir = OUT; dir <= IN; dir++) { sce = &sc->sc_endpoints[i][dir]; @@ -1218,8 +1218,10 @@ ugen_detach(device_t self, int flags) mutex_enter(&sc->sc_lock); if (--sc->sc_refcnt >= 0) { /* Wake everyone */ - for (i = 0; i < USB_MAX_ENDPOINTS; i++) - cv_signal(&sc->sc_endpoints[i][IN].cv); + for (i = 0; i < USB_MAX_ENDPOINTS; i++) { + for (dir = OUT; dir <= IN; dir++) + cv_signal(&sc->sc_endpoints[i][dir].cv); + } /* Wait for processes to go away. */ if (cv_timedwait(&sc->sc_detach_cv, &sc->sc_lock, hz * 60)) aprint_error_dev(self, ": didn't detach\n"); -- 2.28.0 From 2780abf938b78e5a07f67d244eeb27c273133bc8 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Mon, 6 Sep 2021 23:37:56 +0000 Subject: [PATCH 7/9] ugen(4): Use cv_broadcast to wake all I/O operations on detach. Nothing prevents two concurrent reads or two concurrent writes on any particular ugen endpoint, as far as I can tell, and we need to wake all of them, so use cv_broadcast rather than cv_signal on detach. XXX It's not clear to me that cv_signal in the xfer completion callbacks is correct either: any one consumer might use less than the full buffer. So I think either we should use cv_broadcast, or consumers that don't use the whole buffer need to issue cv_signal too to wake up another consumer even if we want to avoid a thundering herd. --- sys/dev/usb/ugen.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sys/dev/usb/ugen.c b/sys/dev/usb/ugen.c index 00c39dc8f486..6c7882c9c03d 100644 --- a/sys/dev/usb/ugen.c +++ b/sys/dev/usb/ugen.c @@ -1220,7 +1220,7 @@ ugen_detach(device_t self, int flags) /* Wake everyone */ for (i = 0; i < USB_MAX_ENDPOINTS; i++) { for (dir = OUT; dir <= IN; dir++) - cv_signal(&sc->sc_endpoints[i][dir].cv); + cv_broadcast(&sc->sc_endpoints[i][dir].cv); } /* Wait for processes to go away. */ if (cv_timedwait(&sc->sc_detach_cv, &sc->sc_lock, hz * 60)) -- 2.28.0 From 28c11c5ba2d1eee224a812eceb6768a23460553b Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sat, 4 Sep 2021 10:13:00 +0000 Subject: [PATCH 8/9] ugen(4): Use cv_wait loop for draining reference count on detach. - Should be no need to use cv_timedwait because all users have now been given a wakeup (previously writers were not, so we relied on the timeouts to work out). - Need to run this in a loop or else a spurious wakeup could cause us to free data structures before the users have actually drained. --- sys/dev/usb/ugen.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/sys/dev/usb/ugen.c b/sys/dev/usb/ugen.c index 6c7882c9c03d..38a0d858c6bc 100644 --- a/sys/dev/usb/ugen.c +++ b/sys/dev/usb/ugen.c @@ -1215,6 +1215,12 @@ ugen_detach(device_t self, int flags) } } + /* + * Wait for users to drain. Before this point there can be no + * more I/O operations started because we set sc_dying; after + * this, there can be no more I/O operations in progress, so it + * will be safe to free things. + */ mutex_enter(&sc->sc_lock); if (--sc->sc_refcnt >= 0) { /* Wake everyone */ @@ -1223,8 +1229,9 @@ ugen_detach(device_t self, int flags) cv_broadcast(&sc->sc_endpoints[i][dir].cv); } /* Wait for processes to go away. */ - if (cv_timedwait(&sc->sc_detach_cv, &sc->sc_lock, hz * 60)) - aprint_error_dev(self, ": didn't detach\n"); + do { + cv_wait(&sc->sc_detach_cv, &sc->sc_lock); + } while (sc->sc_refcnt >= 0); } mutex_exit(&sc->sc_lock); -- 2.28.0 From 243c23f092939c1f343e54be0b86d4afb7d4805d Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 5 Sep 2021 13:48:35 +0000 Subject: [PATCH 9/9] ugen(4): Keep fields null when not allocated; kassert on close. This avoids silent leaks in DIAGNOSTIC kernels. --- sys/dev/usb/ugen.c | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/sys/dev/usb/ugen.c b/sys/dev/usb/ugen.c index 38a0d858c6bc..83f4fdf7d759 100644 --- a/sys/dev/usb/ugen.c +++ b/sys/dev/usb/ugen.c @@ -669,8 +669,10 @@ ugenopen(dev_t dev, int flag, int mode, struct lwp *l) DPRINTFN(5, ("ugenopen: isoc open done\n")); break; bad: - while (--i >= 0) /* implicit buffer free */ + while (--i >= 0) { /* implicit buffer free */ usbd_destroy_xfer(sce->isoreqs[i].xfer); + sce->isoreqs[i].xfer = NULL; + } usbd_close_pipe(sce->pipeh); sce->pipeh = NULL; kmem_free(sce->ibuf, isize * UGEN_NISOFRAMES); @@ -699,13 +701,8 @@ ugen_do_close(struct ugen_softc *sc, int flag, int endpt) KASSERT(KERNEL_LOCKED_P()); /* sc_is_open */ - if (!sc->sc_is_open[endpt]) { - KASSERT(sc->sc_endpoints[endpt][IN].pipeh == NULL); - KASSERT(sc->sc_endpoints[endpt][OUT].pipeh == NULL); - KASSERT(sc->sc_endpoints[endpt][IN].ibuf == NULL); - KASSERT(sc->sc_endpoints[endpt][OUT].ibuf == NULL); - return; - } + if (!sc->sc_is_open[endpt]) + goto out; if (endpt == USB_CONTROL_ENDPOINT) { DPRINTFN(5, ("ugenclose: close control\n")); @@ -733,13 +730,16 @@ ugen_do_close(struct ugen_softc *sc, int flag, int endpt) msize = isize; break; case UE_ISOCHRONOUS: - for (i = 0; i < UGEN_NISOREQS; ++i) + for (i = 0; i < UGEN_NISOREQS; ++i) { usbd_destroy_xfer(sce->isoreqs[i].xfer); + sce->isoreqs[i].xfer = NULL; + } msize = isize * UGEN_NISOFRAMES; break; case UE_BULK: if (sce->state & (UGEN_BULK_RA | UGEN_BULK_WB)) { usbd_destroy_xfer(sce->ra_wb_xfer); + sce->ra_wb_xfer = NULL; msize = sce->ra_wb_bufsize; } break; @@ -755,6 +755,14 @@ ugen_do_close(struct ugen_softc *sc, int flag, int endpt) } out: sc->sc_is_open[endpt] = 0; + for (dir = OUT; dir <= IN; dir++) { + sce = &sc->sc_endpoints[endpt][dir]; + KASSERT(sce->pipeh == NULL); + KASSERT(sce->ibuf == NULL); + KASSERT(sce->ra_wb_xfer == NULL); + for (i = 0; i < UGEN_NISOREQS; i++) + KASSERT(sce->isoreqs[i].xfer == NULL); + } } static int @@ -1649,6 +1657,8 @@ ugen_do_ioctl(struct ugen_softc *sc, int endpt, u_long cmd, /* Only turn RA on if it's currently off. */ if (sce->state & UGEN_BULK_RA) return 0; + KASSERT(sce->ra_wb_xfer == NULL); + KASSERT(sce->ibuf == NULL); if (sce->ra_wb_bufsize == 0 || sce->ra_wb_reqsize == 0) /* shouldn't happen */ @@ -1674,6 +1684,7 @@ ugen_do_ioctl(struct ugen_softc *sc, int endpt, u_long cmd, kmem_free(sce->ibuf, sce->ra_wb_bufsize); sce->ibuf = NULL; usbd_destroy_xfer(sce->ra_wb_xfer); + sce->ra_wb_xfer = NULL; return EIO; } } else { @@ -1684,6 +1695,7 @@ ugen_do_ioctl(struct ugen_softc *sc, int endpt, u_long cmd, sce->state &= ~UGEN_BULK_RA; usbd_abort_pipe(sce->pipeh); usbd_destroy_xfer(sce->ra_wb_xfer); + sce->ra_wb_xfer = NULL; /* * XXX Discard whatever's in the buffer, but we * should keep it around and drain the buffer @@ -1707,12 +1719,15 @@ ugen_do_ioctl(struct ugen_softc *sc, int endpt, u_long cmd, /* Only turn WB on if it's currently off. */ if (sce->state & UGEN_BULK_WB) return 0; + KASSERT(sce->ra_wb_xfer == NULL); + KASSERT(sce->ibuf == NULL); if (sce->ra_wb_bufsize == 0 || sce->ra_wb_reqsize == 0) /* shouldn't happen */ return EINVAL; error = usbd_create_xfer(sce->pipeh, sce->ra_wb_reqsize, 0, 0, &sce->ra_wb_xfer); + /* XXX check error??? */ sce->ra_wb_xferlen = sce->ra_wb_reqsize; sce->ibuf = kmem_alloc(sce->ra_wb_bufsize, KM_SLEEP); sce->fill = sce->cur = sce->ibuf; @@ -1732,6 +1747,7 @@ ugen_do_ioctl(struct ugen_softc *sc, int endpt, u_long cmd, */ usbd_abort_pipe(sce->pipeh); usbd_destroy_xfer(sce->ra_wb_xfer); + sce->ra_wb_xfer = NULL; kmem_free(sce->ibuf, sce->ra_wb_bufsize); sce->ibuf = NULL; } -- 2.28.0