The driver was using its struct usb_interface pointer as an inverted disconnected flag, but was setting it to NULL without making sure all code paths that used it were done with it. Before commit ef61eb43ada6 ("USB: yurex: Fix protection fault after device removal") this included the interrupt-in completion handler, but there are further accesses in dev_err and dev_dbg statements in yurex_write() and the driver-data destructor (sic!). Fix this by unconditionally stopping also the control URB at disconnect and by using a dedicated disconnected flag. Note that we need to take a reference to the struct usb_interface to avoid a use-after-free in the destructor whenever the device was disconnected while the character device was still open.
Unified · Split
| @@ -60,6 +60,7 @@ struct usb_yurex { · 59 unmodified lines | |||
| 60 | 60 | ||
| 61 | struct kref kref; | 61 | struct kref kref; |
| 62 | struct mutex io_mutex; | 62 | struct mutex io_mutex; |
| 63 | unsigned long disconnected:1; | ||
| 63 | struct fasync_struct *async_queue; | 64 | struct fasync_struct *async_queue; |
| 64 | wait_queue_head_t waitq; | 65 | wait_queue_head_t waitq; |
| 65 | 66 | ||
| @@ -107,6 +108,7 @@ static void yurex_delete(struct kref *kref) · 41 unmodified lines | |||
| 107 | dev->int_buffer, dev->urb->transfer_dma); | 108 | dev->int_buffer, dev->urb->transfer_dma); |
| 108 | usb_free_urb(dev->urb); | 109 | usb_free_urb(dev->urb); |
| 109 | } | 110 | } |
| 111 | usb_put_intf(dev->interface); | ||
| 110 | usb_put_dev(dev->udev); | 112 | usb_put_dev(dev->udev); |
| 111 | kfree(dev); | 113 | kfree(dev); |
| 112 | } | 114 | } |
| @@ -205,7 +207,7 @@ static int yurex_probe(struct usb_interface *interface, const struct usb_device_ · 92 unmodified lines | |||
| 205 | init_waitqueue_head(&dev->waitq); | 207 | init_waitqueue_head(&dev->waitq); |
| 206 | 208 | ||
| 207 | dev->udev = usb_get_dev(interface_to_usbdev(interface)); | 209 | dev->udev = usb_get_dev(interface_to_usbdev(interface)); |
| 208 | dev->interface = interface; | 210 | dev->interface = usb_get_intf(interface); |
| 209 | 211 | ||
| 210 | /* set up the endpoint information */ | 212 | /* set up the endpoint information */ |
| 211 | iface_desc = interface->cur_altsetting; | 213 | iface_desc = interface->cur_altsetting; |
| @@ -316,8 +318,9 @@ static void yurex_disconnect(struct usb_interface *interface) · 104 unmodified lines | |||
| 316 | 318 | ||
| 317 | /* prevent more I/O from starting */ | 319 | /* prevent more I/O from starting */ |
| 318 | usb_poison_urb(dev->urb); | 320 | usb_poison_urb(dev->urb); |
| 321 | usb_poison_urb(dev->cntl_urb); | ||
| 319 | mutex_lock(&dev->io_mutex); | 322 | mutex_lock(&dev->io_mutex); |
| 320 | dev->interface = NULL; | 323 | dev->disconnected = 1; |
| 321 | mutex_unlock(&dev->io_mutex); | 324 | mutex_unlock(&dev->io_mutex); |
| 322 | 325 | ||
| 323 | /* wakeup waiters */ | 326 | /* wakeup waiters */ |
| @@ -405,7 +408,7 @@ static ssize_t yurex_read(struct file *file, char __user *buffer, size_t count, · 81 unmodified lines | |||
| 405 | dev = file->private_data; | 408 | dev = file->private_data; |
| 406 | 409 | ||
| 407 | mutex_lock(&dev->io_mutex); | 410 | mutex_lock(&dev->io_mutex); |
| 408 | if (!dev->interface) { /* already disconnected */ | 411 | if (dev->disconnected) { /* already disconnected */ |
| 409 | mutex_unlock(&dev->io_mutex); | 412 | mutex_unlock(&dev->io_mutex); |
| 410 | return -ENODEV; | 413 | return -ENODEV; |
| 411 | } | 414 | } |
| @@ -440,7 +443,7 @@ static ssize_t yurex_write(struct file *file, const char __user *user_buffer, · 28 unmodified lines | |||
| 440 | goto error; | 443 | goto error; |
| 441 | 444 | ||
| 442 | mutex_lock(&dev->io_mutex); | 445 | mutex_lock(&dev->io_mutex); |
| 443 | if (!dev->interface) { /* already disconnected */ | 446 | if (dev->disconnected) { /* already disconnected */ |
| 444 | mutex_unlock(&dev->io_mutex); | 447 | mutex_unlock(&dev->io_mutex); |
| 445 | retval = -ENODEV; | 448 | retval = -ENODEV; |
| 446 | goto error; | 449 | goto error; |