From cd9a4fdd89670aeee7b1aaec9a8acdf7fe2efd37 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Sat, 12 Jun 2021 16:47:58 +0000
Subject: [PATCH 1/3] usb(4): Tighten interface locking and pipe references.
- Just use a reference count, not a list of pipes.
- Take the reference in usbd_open_pipe*, before we even look up the
endpoint by address; the endpoint is not stable until we hold the
interface and prevent usbd_set_interface.
- Make opening pipes just fail if usbd_set_interface is in progress.
=> No need to block -- might block for a while, and this is
essentially a driver error rather than a legitimate reason to
block.
=> This should maybe be a kassert, but it's not clear that ugen(4)
doesn't have a user-triggerable path to that kassert, so let's
keep it as a graceful failure for now until someone can audit
ugen(4) and make an informed decision.
- No need for a separate interface pipe lock; just use the bus lock.
This is a little bit longer than before, but makes the bracketed
nature of the references a little clearer and introduces more
kasserts to detect mistakes with internal API usage.
---
sys/dev/usb/usb_subr.c | 142 ++++++++++++++++++++++++++++++++++-------
sys/dev/usb/usbdi.c | 61 +++++++++++++-----
sys/dev/usb/usbdivar.h | 9 ++-
3 files changed, 169 insertions(+), 43 deletions(-)
diff --git a/sys/dev/usb/usb_subr.c b/sys/dev/usb/usb_subr.c
index c92c05163ac3..ed414cb2fa71 100644
--- a/sys/dev/usb/usb_subr.c
+++ b/sys/dev/usb/usb_subr.c
@@ -415,8 +415,7 @@ usbd_iface_init(struct usbd_device *dev, int ifaceidx)
ifc->ui_index = 0;
ifc->ui_altindex = 0;
ifc->ui_endpoints = NULL;
- LIST_INIT(&ifc->ui_pipes);
- mutex_init(&ifc->ui_pipelock, MUTEX_DEFAULT, IPL_NONE);
+ ifc->ui_busy = 0;
}
static void
@@ -429,9 +428,100 @@ usbd_iface_fini(struct usbd_device *dev, int ifaceidx)
KASSERT(ifc->ui_index == 0);
KASSERT(ifc->ui_altindex == 0);
KASSERT(ifc->ui_endpoints == NULL);
- KASSERT(LIST_EMPTY(&ifc->ui_pipes));
+ KASSERTMSG(ifc->ui_busy == 0, "%"PRId64, ifc->ui_busy);
+}
+
+/*
+ * usbd_iface_lock/locked/unlock, usbd_iface_piperef/pipeunref
+ *
+ * We lock the interface while we are setting it, and we acquire a
+ * reference to the interface for each pipe opened on it.
+ *
+ * Setting the interface while pipes are open is forbidden, and
+ * opening pipes while the interface is being set is forbidden.
+ */
+
+bool
+usbd_iface_locked(struct usbd_interface *iface)
+{
+ bool locked;
+
+ mutex_enter(iface->ui_dev->ud_bus->ub_lock);
+ locked = (iface->ui_busy == -1);
+ mutex_exit(iface->ui_dev->ud_bus->ub_lock);
+
+ return locked;
+}
+
+static void
+usbd_iface_exlock(struct usbd_interface *iface)
+{
+
+ mutex_enter(iface->ui_dev->ud_bus->ub_lock);
+ KASSERTMSG(iface->ui_busy == 0, "interface is not idle,"
+ " busy=%"PRId64, iface->ui_busy);
+ iface->ui_busy = -1;
+ mutex_exit(iface->ui_dev->ud_bus->ub_lock);
+}
+
+usbd_status
+usbd_iface_lock(struct usbd_interface *iface)
+{
+ usbd_status err;
+
+ mutex_enter(iface->ui_dev->ud_bus->ub_lock);
+ KASSERTMSG(iface->ui_busy != -1, "interface is locked");
+ KASSERTMSG(iface->ui_busy >= 0, "%"PRId64, iface->ui_busy);
+ if (iface->ui_busy) {
+ err = USBD_IN_USE;
+ } else {
+ iface->ui_busy = -1;
+ err = 0;
+ }
+ mutex_exit(iface->ui_dev->ud_bus->ub_lock);
+
+ return err;
+}
+
+void
+usbd_iface_unlock(struct usbd_interface *iface)
+{
+
+ mutex_enter(iface->ui_dev->ud_bus->ub_lock);
+ KASSERTMSG(iface->ui_busy == -1, "interface is not locked,"
+ " busy=%"PRId64, iface->ui_busy);
+ iface->ui_busy = 0;
+ mutex_exit(iface->ui_dev->ud_bus->ub_lock);
+}
+
+usbd_status
+usbd_iface_piperef(struct usbd_interface *iface)
+{
+ usbd_status err;
+
+ mutex_enter(iface->ui_dev->ud_bus->ub_lock);
+ KASSERTMSG(iface->ui_busy >= -1, "%"PRId64, iface->ui_busy);
+ if (iface->ui_busy == -1) {
+ err = USBD_IN_USE;
+ } else {
+ iface->ui_busy++;
+ err = 0;
+ }
+ mutex_exit(iface->ui_dev->ud_bus->ub_lock);
+
+ return err;
+}
- mutex_destroy(&ifc->ui_pipelock);
+void
+usbd_iface_pipeunref(struct usbd_interface *iface)
+{
+
+ mutex_enter(iface->ui_dev->ud_bus->ub_lock);
+ KASSERTMSG(iface->ui_busy != -1, "interface is locked");
+ KASSERTMSG(iface->ui_busy != 0, "interface not in use");
+ KASSERTMSG(iface->ui_busy >= 1, "%"PRId64, iface->ui_busy);
+ iface->ui_busy--;
+ mutex_exit(iface->ui_dev->ud_bus->ub_lock);
}
usbd_status
@@ -447,7 +537,7 @@ usbd_fill_iface_data(struct usbd_device *dev, int ifaceidx, int altidx)
int endpt, nendpt;
KASSERT(ifc->ui_dev == dev);
- KASSERT(LIST_EMPTY(&ifc->ui_pipes));
+ KASSERT(usbd_iface_locked(ifc));
idesc = usbd_find_idesc(dev->ud_cdesc, ifaceidx, altidx);
if (idesc == NULL)
@@ -547,7 +637,7 @@ usbd_free_iface_data(struct usbd_device *dev, int ifcno)
KASSERT(ifc->ui_dev == dev);
KASSERT(ifc->ui_idesc != NULL);
- KASSERT(LIST_EMPTY(&ifc->ui_pipes));
+ KASSERT(usbd_iface_locked(ifc));
if (ifc->ui_endpoints) {
int nendpt = ifc->ui_idesc->bNumEndpoints;
@@ -608,7 +698,9 @@ usbd_set_config_index(struct usbd_device *dev, int index, int msg)
/* Free all configuration data structures. */
nifc = dev->ud_cdesc->bNumInterface;
for (ifcidx = 0; ifcidx < nifc; ifcidx++) {
+ usbd_iface_exlock(&dev->ud_ifaces[ifcidx]);
usbd_free_iface_data(dev, ifcidx);
+ usbd_iface_unlock(&dev->ud_ifaces[ifcidx]);
usbd_iface_fini(dev, ifcidx);
}
kmem_free(dev->ud_ifaces, nifc * sizeof(struct usbd_interface));
@@ -783,10 +875,14 @@ usbd_set_config_index(struct usbd_device *dev, int index, int msg)
dev->ud_config = cdp->bConfigurationValue;
for (ifcidx = 0; ifcidx < nifc; ifcidx++) {
usbd_iface_init(dev, ifcidx);
+ usbd_iface_exlock(&dev->ud_ifaces[ifcidx]);
err = usbd_fill_iface_data(dev, ifcidx, 0);
+ usbd_iface_unlock(&dev->ud_ifaces[ifcidx]);
if (err) {
while (--ifcidx >= 0) {
+ usbd_iface_exlock(&dev->ud_ifaces[ifcidx]);
usbd_free_iface_data(dev, ifcidx);
+ usbd_iface_unlock(&dev->ud_ifaces[ifcidx]);
usbd_iface_fini(dev, ifcidx);
}
kmem_free(dev->ud_ifaces,
@@ -827,12 +923,15 @@ usbd_setup_pipe_flags(struct usbd_device *dev, struct usbd_interface *iface,
USBHIST_FUNC();
USBHIST_CALLARGS(usbdebug, "dev=%#jx addr=%jd iface=%#jx ep=%#jx",
(uintptr_t)dev, dev->ud_addr, (uintptr_t)iface, (uintptr_t)ep);
- struct usbd_pipe *p;
+ struct usbd_pipe *p = NULL;
+ bool ep_acquired = false;
usbd_status err;
+ /* Block exclusive use of the endpoint by later pipes. */
err = usbd_endpoint_acquire(dev, ep, flags & USBD_EXCLUSIVE_USE);
if (err)
- return err;
+ goto out;
+ ep_acquired = true;
p = kmem_alloc(dev->ud_bus->ub_pipesize, KM_SLEEP);
DPRINTFN(1, "pipe=%#jx", (uintptr_t)p, 0, 0, 0);
@@ -848,24 +947,11 @@ usbd_setup_pipe_flags(struct usbd_device *dev, struct usbd_interface *iface,
p->up_flags = flags;
SIMPLEQ_INIT(&p->up_queue);
- if (iface) {
- mutex_enter(&iface->ui_pipelock);
- LIST_INSERT_HEAD(&iface->ui_pipes, p, up_next);
- mutex_exit(&iface->ui_pipelock);
- }
-
err = dev->ud_bus->ub_methods->ubm_open(p);
if (err) {
DPRINTF("endpoint=%#jx failed, error=%jd",
(uintptr_t)ep->ue_edesc->bEndpointAddress, err, 0, 0);
- if (iface) {
- mutex_enter(&iface->ui_pipelock);
- LIST_REMOVE(p, up_next);
- mutex_exit(&iface->ui_pipelock);
- }
- kmem_free(p, dev->ud_bus->ub_pipesize);
- usbd_endpoint_release(dev, ep);
- return err;
+ goto out;
}
KASSERT(p->up_methods->upm_start || p->up_serialise == false);
@@ -874,7 +960,15 @@ usbd_setup_pipe_flags(struct usbd_device *dev, struct usbd_interface *iface,
USB_TASKQ_MPSAFE);
DPRINTFN(1, "pipe=%#jx", (uintptr_t)p, 0, 0, 0);
*pipe = p;
- return USBD_NORMAL_COMPLETION;
+ p = NULL; /* handed off to caller */
+ ep_acquired = false; /* handed off to pipe */
+ err = USBD_NORMAL_COMPLETION;
+
+out: if (p)
+ kmem_free(p, dev->ud_bus->ub_pipesize);
+ if (ep_acquired)
+ usbd_endpoint_release(dev, ep);
+ return err;
}
usbd_status
@@ -1712,7 +1806,9 @@ usb_free_device(struct usbd_device *dev)
if (dev->ud_ifaces != NULL) {
nifc = dev->ud_cdesc->bNumInterface;
for (ifcidx = 0; ifcidx < nifc; ifcidx++) {
+ usbd_iface_exlock(&dev->ud_ifaces[ifcidx]);
usbd_free_iface_data(dev, ifcidx);
+ usbd_iface_unlock(&dev->ud_ifaces[ifcidx]);
usbd_iface_fini(dev, ifcidx);
}
kmem_free(dev->ud_ifaces,
diff --git a/sys/dev/usb/usbdi.c b/sys/dev/usb/usbdi.c
index ddb5e811a1d0..089957498ee5 100644
--- a/sys/dev/usb/usbdi.c
+++ b/sys/dev/usb/usbdi.c
@@ -225,6 +225,7 @@ usbd_open_pipe_ival(struct usbd_interface *iface, uint8_t address,
{
struct usbd_pipe *p;
struct usbd_endpoint *ep;
+ bool piperef = false;
usbd_status err;
int i;
@@ -232,22 +233,49 @@ usbd_open_pipe_ival(struct usbd_interface *iface, uint8_t address,
USBHIST_CALLARGS(usbdebug, "iface = %#jx address = %#jx flags = %#jx",
(uintptr_t)iface, address, flags, 0);
+ /*
+ * Block usbd_set_interface so we have a snapshot of the
+ * interface endpoints. They will remain stable until we drop
+ * the reference in usbd_close_pipe (or on failure here).
+ */
+ err = usbd_iface_piperef(iface);
+ if (err)
+ goto out;
+ piperef = true;
+
+ /* Find the endpoint at this address. */
for (i = 0; i < iface->ui_idesc->bNumEndpoints; i++) {
ep = &iface->ui_endpoints[i];
- if (ep->ue_edesc == NULL)
- return USBD_IOERROR;
+ if (ep->ue_edesc == NULL) {
+ err = USBD_IOERROR;
+ goto out;
+ }
if (ep->ue_edesc->bEndpointAddress == address)
- goto found;
+ break;
}
- return USBD_BAD_ADDRESS;
- found:
+ if (i == iface->ui_idesc->bNumEndpoints) {
+ err = USBD_BAD_ADDRESS;
+ goto out;
+ }
+
+ /* Set up the pipe with this endpoint. */
err = usbd_setup_pipe_flags(iface->ui_dev, iface, ep, ival, &p, flags);
if (err)
- return err;
+ goto out;
+
+ /* Success! */
*pipe = p;
+ p = NULL; /* handed off to caller */
+ piperef = false; /* handed off to pipe */
SDT_PROBE5(usb, device, pipe, open,
iface, address, flags, ival, p);
- return USBD_NORMAL_COMPLETION;
+ err = USBD_NORMAL_COMPLETION;
+
+out: if (p)
+ usbd_close_pipe(p);
+ if (piperef)
+ usbd_iface_pipeunref(iface);
+ return err;
}
usbd_status
@@ -316,12 +344,9 @@ usbd_close_pipe(struct usbd_pipe *pipe)
usbd_destroy_xfer(pipe->up_intrxfer);
usb_rem_task_wait(pipe->up_dev, &pipe->up_async_task, USB_TASKQ_DRIVER,
NULL);
- if (pipe->up_iface) {
- mutex_enter(&pipe->up_iface->ui_pipelock);
- LIST_REMOVE(pipe, up_next);
- mutex_exit(&pipe->up_iface->ui_pipelock);
- }
usbd_endpoint_release(pipe->up_dev, pipe->up_endpoint);
+ if (pipe->up_iface)
+ usbd_iface_pipeunref(pipe->up_iface);
kmem_free(pipe, pipe->up_dev->ud_bus->ub_pipesize);
return USBD_NORMAL_COMPLETION;
@@ -865,17 +890,17 @@ usbd_pipe2device_handle(struct usbd_pipe *pipe)
usbd_status
usbd_set_interface(struct usbd_interface *iface, int altidx)
{
+ bool locked = false;
usb_device_request_t req;
usbd_status err;
USBHIST_FUNC();
USBHIST_CALLARGS(usbdebug, "iface %#jx", (uintptr_t)iface, 0, 0, 0);
- mutex_enter(&iface->ui_pipelock);
- if (LIST_FIRST(&iface->ui_pipes) != NULL) {
- err = USBD_IN_USE;
+ err = usbd_iface_lock(iface);
+ if (err)
goto out;
- }
+ locked = true;
err = usbd_fill_iface_data(iface->ui_dev, iface->ui_index, altidx);
if (err)
@@ -888,7 +913,9 @@ usbd_set_interface(struct usbd_interface *iface, int altidx)
USETW(req.wLength, 0);
err = usbd_do_request(iface->ui_dev, &req, 0);
-out: mutex_exit(&iface->ui_pipelock);
+out: /* XXX back out iface data? */
+ if (locked)
+ usbd_iface_unlock(iface);
return err;
}
diff --git a/sys/dev/usb/usbdivar.h b/sys/dev/usb/usbdivar.h
index c275f6a2327f..6f036860990a 100644
--- a/sys/dev/usb/usbdivar.h
+++ b/sys/dev/usb/usbdivar.h
@@ -230,8 +230,7 @@ struct usbd_interface {
int ui_index;
int ui_altindex;
struct usbd_endpoint *ui_endpoints;
- kmutex_t ui_pipelock;
- LIST_HEAD(, usbd_pipe) ui_pipes;
+ int64_t ui_busy; /* #pipes, or -1 if setting */
};
struct usbd_pipe {
@@ -243,7 +242,6 @@ struct usbd_pipe {
bool up_serialise;
SIMPLEQ_HEAD(, usbd_xfer)
up_queue;
- LIST_ENTRY(usbd_pipe) up_next;
struct usb_task up_async_task;
struct usbd_xfer *up_intrxfer; /* used for repeating requests */
@@ -347,6 +345,11 @@ usbd_status usbd_reattach_device(device_t, struct usbd_device *,
int, const int *);
void usbd_remove_device(struct usbd_device *, struct usbd_port *);
+bool usbd_iface_locked(struct usbd_interface *);
+usbd_status usbd_iface_lock(struct usbd_interface *);
+void usbd_iface_unlock(struct usbd_interface *);
+usbd_status usbd_iface_piperef(struct usbd_interface *);
+void usbd_iface_pipeunref(struct usbd_interface *);
usbd_status usbd_fill_iface_data(struct usbd_device *, int, int);
void usb_free_device(struct usbd_device *);
From b65a497cacc9a1284b31c3ea09747b954ac9746a Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Sat, 12 Jun 2021 22:39:49 +0000
Subject: [PATCH 2/3] uhub(4): Trigger bus exploration after rescanning
children.
Otherwise, if uhub4 is attached at uhub1, then when we do
# drvctl -d uhub4
# drvctl -r -a usbdevif uhub1
the rescan never discovers devices attached recursively at uhub4, and
uhub4 leaks a config_pending_incr count so it can't be detached.
---
sys/dev/usb/uhub.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/sys/dev/usb/uhub.c b/sys/dev/usb/uhub.c
index 84b85d1f5254..ec7e64dd1e58 100644
--- a/sys/dev/usb/uhub.c
+++ b/sys/dev/usb/uhub.c
@@ -946,6 +946,10 @@ uhub_rescan(device_t self, const char *ifattr, const int *locators)
usbd_reattach_device(sc->sc_dev, dev, port, locators);
}
uhub_explore_exit(sc);
+
+ /* Arrange to recursively explore hubs we may have found. */
+ usb_needs_explore(sc->sc_hub);
+
return 0;
}
From 71736cb0628d9bd188825a520665e75c733eae6c Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Sat, 12 Jun 2021 22:41:22 +0000
Subject: [PATCH 3/3] autoconf(9): Sprinkle KASSERT(dev->dv_pending == 0) in
dealloc paths.
This would have made uhub's config_pending_incr leak more obvious by
crashing in KASSERT(dev->dv_pending == 0) early on, rather than
crashing in a tailq panic later on when the config_pending list gets
corrupted with use-after-free because nothing took the device off
dv_pending_list when attached.
(This is slightly academic now because config_detach blocks until
dev->dv_pending == 0, but it doesn't hurt and makes the intent
clearer.)
---
sys/kern/subr_autoconf.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/sys/kern/subr_autoconf.c b/sys/kern/subr_autoconf.c
index 4523d285b5c4..cd3701131ae9 100644
--- a/sys/kern/subr_autoconf.c
+++ b/sys/kern/subr_autoconf.c
@@ -1405,7 +1405,9 @@ config_devlink(device_t dev)
static void
config_devfree(device_t dev)
{
+
KASSERT(dev->dv_flags & DVF_PRIV_ALLOC);
+ KASSERTMSG(dev->dv_pending == 0, "%d", dev->dv_pending);
if (dev->dv_cfattach->ca_devsize > 0)
kmem_free(dev->dv_private, dev->dv_cfattach->ca_devsize);
@@ -1423,6 +1425,7 @@ config_devunlink(device_t dev, struct devicelist *garbage)
int i;
KASSERT(mutex_owned(&alldevs_lock));
+ KASSERTMSG(dev->dv_pending == 0, "%d", dev->dv_pending);
/* Unlink from device list. Link to garbage list. */
TAILQ_REMOVE(&alldevs, dev, dv_list);
@@ -1453,6 +1456,8 @@ config_devdelete(device_t dev)
struct device_garbage *dg = &dev->dv_garbage;
device_lock_t dvl = device_getlock(dev);
+ KASSERTMSG(dev->dv_pending == 0, "%d", dev->dv_pending);
+
if (dg->dg_devs != NULL)
kmem_free(dg->dg_devs, sizeof(device_t) * dg->dg_ndevs);