| Message ID | 20191009153848.8664-6-johan@kernel.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | aafb00a977cf7d81821f7c9d12e04c558c22dc3c |
| Headers | show |
| Series | USB: misc: fix disconnect bugs | expand |
On Wed, Oct 09, 2019 at 05:38:48PM +0200, Johan Hovold wrote: > 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. > > Fixes: aadd6472d904 ("USB: yurex.c: remove dbg() usage") > Fixes: 45714104b9e8 ("USB: yurex.c: remove err() usage") > Cc: stable <stable@vger.kernel.org> # 3.5: ef61eb43ada6 > Signed-off-by: Johan Hovold <johan@kernel.org> Greg, I noticed that you picked up all patches in this series except this last one. Was that one purpose or by mistake? Johan
On Thu, Oct 10, 2019 at 01:05:32PM +0200, Johan Hovold wrote: > On Wed, Oct 09, 2019 at 05:38:48PM +0200, Johan Hovold wrote: > > 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. > > > > Fixes: aadd6472d904 ("USB: yurex.c: remove dbg() usage") > > Fixes: 45714104b9e8 ("USB: yurex.c: remove err() usage") > > Cc: stable <stable@vger.kernel.org> # 3.5: ef61eb43ada6 > > Signed-off-by: Johan Hovold <johan@kernel.org> > > Greg, I noticed that you picked up all patches in this series except > this last one. > > Was that one purpose or by mistake? Mistake, thanks for catching that. Now queued up. greg k-h
diff --git a/drivers/usb/misc/yurex.c b/drivers/usb/misc/yurex.c index 8d52d4336c29..be0505b8b5d4 100644 --- a/drivers/usb/misc/yurex.c +++ b/drivers/usb/misc/yurex.c @@ -60,6 +60,7 @@ struct usb_yurex { struct kref kref; struct mutex io_mutex; + unsigned long disconnected:1; struct fasync_struct *async_queue; wait_queue_head_t waitq; @@ -107,6 +108,7 @@ static void yurex_delete(struct kref *kref) dev->int_buffer, dev->urb->transfer_dma); usb_free_urb(dev->urb); } + usb_put_intf(dev->interface); usb_put_dev(dev->udev); kfree(dev); } @@ -205,7 +207,7 @@ static int yurex_probe(struct usb_interface *interface, const struct usb_device_ init_waitqueue_head(&dev->waitq); dev->udev = usb_get_dev(interface_to_usbdev(interface)); - dev->interface = interface; + dev->interface = usb_get_intf(interface); /* set up the endpoint information */ iface_desc = interface->cur_altsetting; @@ -316,8 +318,9 @@ static void yurex_disconnect(struct usb_interface *interface) /* prevent more I/O from starting */ usb_poison_urb(dev->urb); + usb_poison_urb(dev->cntl_urb); mutex_lock(&dev->io_mutex); - dev->interface = NULL; + dev->disconnected = 1; mutex_unlock(&dev->io_mutex); /* wakeup waiters */ @@ -405,7 +408,7 @@ static ssize_t yurex_read(struct file *file, char __user *buffer, size_t count, dev = file->private_data; mutex_lock(&dev->io_mutex); - if (!dev->interface) { /* already disconnected */ + if (dev->disconnected) { /* already disconnected */ mutex_unlock(&dev->io_mutex); return -ENODEV; } @@ -440,7 +443,7 @@ static ssize_t yurex_write(struct file *file, const char __user *user_buffer, goto error; mutex_lock(&dev->io_mutex); - if (!dev->interface) { /* already disconnected */ + if (dev->disconnected) { /* already disconnected */ mutex_unlock(&dev->io_mutex); retval = -ENODEV; goto error;
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. Fixes: aadd6472d904 ("USB: yurex.c: remove dbg() usage") Fixes: 45714104b9e8 ("USB: yurex.c: remove err() usage") Cc: stable <stable@vger.kernel.org> # 3.5: ef61eb43ada6 Signed-off-by: Johan Hovold <johan@kernel.org> --- drivers/usb/misc/yurex.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)