From f97da128ab86c87322269826c22c5c1429f8cef8 Mon Sep 17 00:00:00 2001 From: Hans Petter Selasky Date: Fri, 17 Jan 2014 10:35:18 +0000 Subject: [PATCH] Fix a possible memory use after free and leak situation associated with USB device detach when using character device handles. This also includes LibUSB. It turns out that "usb_close()" cannot always get a reference to clean up its USB transfers and such, if called during the kernel USB device detach. Analysis by: hselasky @ Reported by: Juergen Lock MFC after: 1 week --- sys/dev/usb/usb_dev.c | 26 ++++++++++---------------- sys/dev/usb/usb_device.c | 24 ++++++++++++++---------- 2 files changed, 24 insertions(+), 26 deletions(-) diff --git a/sys/dev/usb/usb_dev.c b/sys/dev/usb/usb_dev.c index c5ee368fc9ba..6a3080f1fccc 100644 --- a/sys/dev/usb/usb_dev.c +++ b/sys/dev/usb/usb_dev.c @@ -207,6 +207,11 @@ usb_ref_device(struct usb_cdev_privdata *cpd, DPRINTFN(2, "no device at %u\n", cpd->dev_index); goto error; } + if (cpd->udev->state == USB_STATE_DETACHED && + (need_uref != 2)) { + DPRINTFN(2, "device is detached\n"); + goto error; + } if (cpd->udev->refcount == USB_DEV_REF_MAX) { DPRINTFN(2, "no dev ref\n"); goto error; @@ -922,23 +927,12 @@ usb_close(void *arg) DPRINTFN(2, "cpd=%p\n", cpd); - err = usb_ref_device(cpd, &refs, 0); - if (err) + err = usb_ref_device(cpd, &refs, + 2 /* uref and allow detached state */); + if (err) { + DPRINTFN(0, "Cannot grab USB reference when " + "closing USB file handle\n"); goto done; - - /* - * If this function is not called directly from the root HUB - * thread, there is usually a need to lock the enumeration - * lock. Check this. - */ - if (!usbd_enum_is_locked(cpd->udev)) { - - DPRINTFN(2, "Locking enumeration\n"); - - /* reference device */ - err = usb_usb_ref_device(cpd, &refs); - if (err) - goto done; } if (cpd->fflags & FREAD) { usb_fifo_close(refs.rxfifo, cpd->fflags); diff --git a/sys/dev/usb/usb_device.c b/sys/dev/usb/usb_device.c index 08c3cd08db32..0e9176c0aa4d 100644 --- a/sys/dev/usb/usb_device.c +++ b/sys/dev/usb/usb_device.c @@ -2070,6 +2070,8 @@ usb_free_device(struct usb_device *udev, uint8_t flag) DPRINTFN(4, "udev=%p port=%d\n", udev, udev->port_no); bus = udev->bus; + + /* set DETACHED state to prevent any further references */ usb_set_device_state(udev, USB_STATE_DETACHED); #if USB_HAVE_DEVCTL @@ -2085,16 +2087,7 @@ usb_free_device(struct usb_device *udev, uint8_t flag) usb_free_symlink(udev->ugen_symlink); udev->ugen_symlink = NULL; } -#endif - /* - * Unregister our device first which will prevent any further - * references: - */ - usb_bus_port_set_device(bus, udev->parent_hub ? - udev->parent_hub->hub->ports + udev->port_index : NULL, - NULL, USB_ROOT_HUB_ADDR); -#if USB_HAVE_UGEN /* wait for all pending references to go away: */ mtx_lock(&usb_ref_lock); udev->refcount--; @@ -2114,6 +2107,11 @@ usb_free_device(struct usb_device *udev, uint8_t flag) /* the following will get the device unconfigured in software */ usb_unconfigure(udev, USB_UNCFG_FLAG_FREE_EP0); + /* final device unregister after all character devices are closed */ + usb_bus_port_set_device(bus, udev->parent_hub ? + udev->parent_hub->hub->ports + udev->port_index : NULL, + NULL, USB_ROOT_HUB_ADDR); + /* unsetup any leftover default USB transfers */ usbd_transfer_unsetup(udev->ctrl_xfer, USB_CTRL_XFER_MAX); @@ -2647,8 +2645,14 @@ usb_set_device_state(struct usb_device *udev, enum usb_dev_state state) DPRINTF("udev %p state %s -> %s\n", udev, usb_statestr(udev->state), usb_statestr(state)); - udev->state = state; +#if USB_HAVE_UGEN + mtx_lock(&usb_ref_lock); +#endif + udev->state = state; +#if USB_HAVE_UGEN + mtx_unlock(&usb_ref_lock); +#endif if (udev->bus->methods->device_state_change != NULL) (udev->bus->methods->device_state_change) (udev); }