USB: iowarrior: disconnect fixes and locking cleanups
Subsystems (2)
cover USB: iowarrior: disconnect fixes and locking cleanups
This series fixes a use-after-free bug introduced by a recent disconnect-deadlock fix that was reported by syzbot. Turns out there was already a related bug in the driver, and the first patch addresses both issues.
While looking at the code I found two more use-after-free bugs, which the next two patches fix.
The next two clean up the driver by dropping two redundant locks.
Tested using a mockup device.
Johan
Johan Hovold (6):
USB: iowarrior: fix use-after-free on disconnect
USB: iowarrior: fix use-after-free on release
USB: iowarrior: fix use-after-free after driver unbind
USB: iowarrior: drop redundant disconnect mutex
USB: iowarrior: drop redundant iowarrior mutex
USB: iowarrior: use pr_err()
drivers/usb/misc/iowarrior.c | 48 +++++++++++------------------------- 1 file changed, 15 insertions(+), 33 deletions(-)
Signature · 1 line
2.23.0
PATCH 1/6 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().
--- drivers/usb/misc/iowarrior.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/usb/misc/iowarrior.c +3 -4
@@ -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); }
Signature · 1 line
2.23.0
PATCH 2/6 USB: iowarrior: fix use-after-free on release
In reply to Johan HovoldThe 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.
--- drivers/usb/misc/iowarrior.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/misc/iowarrior.c +2 -1
@@ -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);
Signature · 1 line
2.23.0
PATCH 3/6 USB: iowarrior: fix use-after-free after driver unbind
In reply to Johan HovoldMake sure to stop also the asynchronous write URBs on disconnect() to avoid use-after-free in the completion handler after driver unbind.
--- drivers/usb/misc/iowarrior.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/usb/misc/iowarrior.c +6 -0
@@ -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);
Signature · 1 line
2.23.0
PATCH 4/6 USB: iowarrior: drop redundant disconnect mutex
In reply to Johan HovoldDrop 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.
--- drivers/usb/misc/iowarrior.c | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-)
drivers/usb/misc/iowarrior.c +2 -15
@@ -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 @@ static int iowarrior_probe(struct usb_interface *interface,*/ 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);
Signature · 1 line
2.23.0
PATCH 5/6 USB: iowarrior: drop redundant iowarrior mutex
In reply to Johan HovoldDrop 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.
--- drivers/usb/misc/iowarrior.c | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-)
drivers/usb/misc/iowarrior.c +1 -12
@@ -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; }
Signature · 1 line
2.23.0
PATCH 6/6 USB: iowarrior: use pr_err()
In reply to Johan HovoldReplace the one remaining printk with pr_err().
--- drivers/usb/misc/iowarrior.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/misc/iowarrior.c +1 -1
@@ -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 ; }
Signature · 1 line
2.23.0
Greg Kroah-Hartman
maintainerQuoted 11 lines · Johan Hovold
This series fixes a use-after-free bug introduced by a recent disconnect-deadlock fix that was reported by syzbot. Turns out there was already a related bug in the driver, and the first patch addresses both issues.
While looking at the code I found two more use-after-free bugs, which the next two patches fix.
The next two clean up the driver by dropping two redundant locks.
Tested using a mockup device.
Thanks for these patches, now queued up. I have one of these devices (their new one) and need to fix the driver up to work with it, but I'll start on that on top of these fixes :)
thanks,
greg k-h