From edc4746f253d907d048de680a621e121517f484b Mon Sep 17 00:00:00 2001 From: Johan Hovold Date: Wed, 9 Oct 2019 12:48:41 +0200 Subject: USB: iowarrior: fix use-after-free on disconnect A recent fix addressing a deadlock on disconnect introduced a new bug by moving the present flag out of the critical section protected by the driver-data mutex. This could lead to a racing release() freeing the driver data before disconnect() is done with it. Due to insufficient locking a related use-after-free could be triggered also before the above mentioned commit. Specifically, the driver needs to hold the driver-data mutex also while checking the opened flag at disconnect(). Fixes: c468a8aa790e ("usb: iowarrior: fix deadlock on disconnect") Fixes: 946b960d13c1 ("USB: add driver for iowarrior devices.") Cc: stable # 2.6.21 Reported-by: syzbot+0761012cebf7bdb38137@syzkaller.appspotmail.com Signed-off-by: Johan Hovold Link: https://lore.kernel.org/r/20191009104846.5925-2-johan@kernel.org Signed-off-by: Greg Kroah-Hartman --- drivers/usb/misc/iowarrior.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'drivers/usb/misc/iowarrior.c') diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c index f5bed9f29e56..4fe1d3267b3c 100644 --- a/drivers/usb/misc/iowarrior.c +++ b/drivers/usb/misc/iowarrior.c @@ -866,8 +866,6 @@ static void iowarrior_disconnect(struct usb_interface *interface) dev = usb_get_intfdata(interface); mutex_lock(&iowarrior_open_disc_lock); usb_set_intfdata(interface, NULL); - /* prevent device read, write and ioctl */ - dev->present = 0; minor = dev->minor; mutex_unlock(&iowarrior_open_disc_lock); @@ -878,8 +876,7 @@ static void iowarrior_disconnect(struct usb_interface *interface) mutex_lock(&dev->mutex); /* prevent device read, write and ioctl */ - - mutex_unlock(&dev->mutex); + dev->present = 0; if (dev->opened) { /* There is a process that holds a filedescriptor to the device , @@ -889,8 +886,10 @@ static void iowarrior_disconnect(struct usb_interface *interface) usb_kill_urb(dev->int_in_urb); wake_up_interruptible(&dev->read_wait); wake_up_interruptible(&dev->write_wait); + mutex_unlock(&dev->mutex); } else { /* no process is using the device, cleanup now */ + mutex_unlock(&dev->mutex); iowarrior_delete(dev); } -- cgit From 80cd5479b525093a56ef768553045741af61b250 Mon Sep 17 00:00:00 2001 From: Johan Hovold Date: Wed, 9 Oct 2019 12:48:42 +0200 Subject: USB: iowarrior: fix use-after-free on release The driver was accessing its struct usb_interface from its release() callback without holding a reference. This would lead to a use-after-free whenever debugging was enabled and the device was disconnected while its character device was open. Fixes: 549e83500b80 ("USB: iowarrior: Convert local dbg macro to dev_dbg") Cc: stable # 3.16 Signed-off-by: Johan Hovold Link: https://lore.kernel.org/r/20191009104846.5925-3-johan@kernel.org Signed-off-by: Greg Kroah-Hartman --- drivers/usb/misc/iowarrior.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'drivers/usb/misc/iowarrior.c') diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c index 4fe1d3267b3c..6841267820c6 100644 --- a/drivers/usb/misc/iowarrior.c +++ b/drivers/usb/misc/iowarrior.c @@ -243,6 +243,7 @@ static inline void iowarrior_delete(struct iowarrior *dev) kfree(dev->int_in_buffer); usb_free_urb(dev->int_in_urb); kfree(dev->read_queue); + usb_put_intf(dev->interface); kfree(dev); } @@ -764,7 +765,7 @@ static int iowarrior_probe(struct usb_interface *interface, init_waitqueue_head(&dev->write_wait); dev->udev = udev; - dev->interface = interface; + dev->interface = usb_get_intf(interface); iface_desc = interface->cur_altsetting; dev->product_id = le16_to_cpu(udev->descriptor.idProduct); -- cgit From b5f8d46867ca233d773408ffbe691a8062ed718f Mon Sep 17 00:00:00 2001 From: Johan Hovold Date: Wed, 9 Oct 2019 12:48:43 +0200 Subject: USB: iowarrior: fix use-after-free after driver unbind Make sure to stop also the asynchronous write URBs on disconnect() to avoid use-after-free in the completion handler after driver unbind. Fixes: 946b960d13c1 ("USB: add driver for iowarrior devices.") Cc: stable # 2.6.21: 51a2f077c44e ("USB: introduce usb_anchor") Signed-off-by: Johan Hovold Link: https://lore.kernel.org/r/20191009104846.5925-4-johan@kernel.org Signed-off-by: Greg Kroah-Hartman --- drivers/usb/misc/iowarrior.c | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'drivers/usb/misc/iowarrior.c') diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c index 6841267820c6..f405fa734bcc 100644 --- a/drivers/usb/misc/iowarrior.c +++ b/drivers/usb/misc/iowarrior.c @@ -87,6 +87,7 @@ struct iowarrior { char chip_serial[9]; /* the serial number string of the chip connected */ int report_size; /* number of bytes in a report */ u16 product_id; + struct usb_anchor submitted; }; /*--------------*/ @@ -425,11 +426,13 @@ static ssize_t iowarrior_write(struct file *file, retval = -EFAULT; goto error; } + usb_anchor_urb(int_out_urb, &dev->submitted); retval = usb_submit_urb(int_out_urb, GFP_KERNEL); if (retval) { dev_dbg(&dev->interface->dev, "submit error %d for urb nr.%d\n", retval, atomic_read(&dev->write_busy)); + usb_unanchor_urb(int_out_urb); goto error; } /* submit was ok */ @@ -770,6 +773,8 @@ static int iowarrior_probe(struct usb_interface *interface, iface_desc = interface->cur_altsetting; dev->product_id = le16_to_cpu(udev->descriptor.idProduct); + init_usb_anchor(&dev->submitted); + res = usb_find_last_int_in_endpoint(iface_desc, &dev->int_in_endpoint); if (res) { dev_err(&interface->dev, "no interrupt-in endpoint found\n"); @@ -885,6 +890,7 @@ static void iowarrior_disconnect(struct usb_interface *interface) Deleting the device is postponed until close() was called. */ usb_kill_urb(dev->int_in_urb); + usb_kill_anchored_urbs(&dev->submitted); wake_up_interruptible(&dev->read_wait); wake_up_interruptible(&dev->write_wait); mutex_unlock(&dev->mutex); -- cgit From 7c5b971d623fdb40c03205e99f9ef68002b34726 Mon Sep 17 00:00:00 2001 From: Johan Hovold Date: Wed, 9 Oct 2019 12:48:44 +0200 Subject: USB: iowarrior: drop redundant disconnect mutex Drop the redundant disconnect mutex which was introduced after the open-disconnect race had been addressed generally in USB core by commit d4ead16f50f9 ("USB: prevent char device open/deregister race"). Specifically, the rw-semaphore in core guarantees that all calls to open() will have completed and that no new calls to open() will occur after usb_deregister_dev() returns. Hence there is no need use the driver data as an inverted disconnected flag. Signed-off-by: Johan Hovold Link: https://lore.kernel.org/r/20191009104846.5925-5-johan@kernel.org Signed-off-by: Greg Kroah-Hartman --- drivers/usb/misc/iowarrior.c | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) (limited to 'drivers/usb/misc/iowarrior.c') diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c index f405fa734bcc..d844c2098e42 100644 --- a/drivers/usb/misc/iowarrior.c +++ b/drivers/usb/misc/iowarrior.c @@ -58,7 +58,6 @@ MODULE_LICENSE("GPL"); static DEFINE_MUTEX(iowarrior_mutex); static struct usb_driver iowarrior_driver; -static DEFINE_MUTEX(iowarrior_open_disc_lock); /*--------------*/ /* data */ @@ -601,16 +600,13 @@ static int iowarrior_open(struct inode *inode, struct file *file) return -ENODEV; } - mutex_lock(&iowarrior_open_disc_lock); dev = usb_get_intfdata(interface); if (!dev) { - mutex_unlock(&iowarrior_open_disc_lock); mutex_unlock(&iowarrior_mutex); return -ENODEV; } mutex_lock(&dev->mutex); - mutex_unlock(&iowarrior_open_disc_lock); /* Only one process can open each device, no sharing. */ if (dev->opened) { @@ -842,7 +838,6 @@ static int iowarrior_probe(struct usb_interface *interface, if (retval) { /* something prevented us from registering this driver */ dev_err(&interface->dev, "Not able to get a minor for this device.\n"); - usb_set_intfdata(interface, NULL); goto error; } @@ -866,16 +861,8 @@ error: */ static void iowarrior_disconnect(struct usb_interface *interface) { - struct iowarrior *dev; - int minor; - - dev = usb_get_intfdata(interface); - mutex_lock(&iowarrior_open_disc_lock); - usb_set_intfdata(interface, NULL); - - minor = dev->minor; - mutex_unlock(&iowarrior_open_disc_lock); - /* give back our minor - this will call close() locks need to be dropped at this point*/ + struct iowarrior *dev = usb_get_intfdata(interface); + int minor = dev->minor; usb_deregister_dev(interface, &iowarrior_class); -- cgit From 8d33e828f72c216ae264ad39088a595d9a6cc95c Mon Sep 17 00:00:00 2001 From: Johan Hovold Date: Wed, 9 Oct 2019 12:48:45 +0200 Subject: USB: iowarrior: drop redundant iowarrior mutex Drop the redundant iowarrior mutex introduced by commit 925ce689bb31 ("USB: autoconvert trivial BKL users to private mutex") which replaced an earlier BKL use. The lock serialised calls to open() against other open() and ioctl(), but neither is needed. Signed-off-by: Johan Hovold Link: https://lore.kernel.org/r/20191009104846.5925-6-johan@kernel.org Signed-off-by: Greg Kroah-Hartman --- drivers/usb/misc/iowarrior.c | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) (limited to 'drivers/usb/misc/iowarrior.c') diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c index d844c2098e42..ad29ef51e53f 100644 --- a/drivers/usb/misc/iowarrior.c +++ b/drivers/usb/misc/iowarrior.c @@ -54,9 +54,6 @@ MODULE_AUTHOR(DRIVER_AUTHOR); MODULE_DESCRIPTION(DRIVER_DESC); MODULE_LICENSE("GPL"); -/* Module parameters */ -static DEFINE_MUTEX(iowarrior_mutex); - static struct usb_driver iowarrior_driver; /*--------------*/ @@ -480,8 +477,6 @@ static long iowarrior_ioctl(struct file *file, unsigned int cmd, if (!buffer) return -ENOMEM; - /* lock this object */ - mutex_lock(&iowarrior_mutex); mutex_lock(&dev->mutex); /* verify that the device wasn't unplugged */ @@ -574,7 +569,6 @@ static long iowarrior_ioctl(struct file *file, unsigned int cmd, error_out: /* unlock the device */ mutex_unlock(&dev->mutex); - mutex_unlock(&iowarrior_mutex); kfree(buffer); return retval; } @@ -589,22 +583,18 @@ static int iowarrior_open(struct inode *inode, struct file *file) int subminor; int retval = 0; - mutex_lock(&iowarrior_mutex); subminor = iminor(inode); interface = usb_find_interface(&iowarrior_driver, subminor); if (!interface) { - mutex_unlock(&iowarrior_mutex); printk(KERN_ERR "%s - error, can't find device for minor %d\n", __func__, subminor); return -ENODEV; } dev = usb_get_intfdata(interface); - if (!dev) { - mutex_unlock(&iowarrior_mutex); + if (!dev) return -ENODEV; - } mutex_lock(&dev->mutex); @@ -628,7 +618,6 @@ static int iowarrior_open(struct inode *inode, struct file *file) out: mutex_unlock(&dev->mutex); - mutex_unlock(&iowarrior_mutex); return retval; } -- cgit From ebb2fe57a51c630e0f852becbbdd295ad5d60514 Mon Sep 17 00:00:00 2001 From: Johan Hovold Date: Wed, 9 Oct 2019 12:48:46 +0200 Subject: USB: iowarrior: use pr_err() Replace the one remaining printk with pr_err(). Signed-off-by: Johan Hovold Link: https://lore.kernel.org/r/20191009104846.5925-7-johan@kernel.org Signed-off-by: Greg Kroah-Hartman --- drivers/usb/misc/iowarrior.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/usb/misc/iowarrior.c') diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c index ad29ef51e53f..dce44fbf031f 100644 --- a/drivers/usb/misc/iowarrior.c +++ b/drivers/usb/misc/iowarrior.c @@ -587,7 +587,7 @@ static int iowarrior_open(struct inode *inode, struct file *file) interface = usb_find_interface(&iowarrior_driver, subminor); if (!interface) { - printk(KERN_ERR "%s - error, can't find device for minor %d\n", + pr_err("%s - error, can't find device for minor %d\n", __func__, subminor); return -ENODEV; } -- cgit