USB: legousbtower: misc fixes
Subsystems (3)
cover USB: legousbtower: misc fixes
This series fixes a few issues found in the legousbtower driver. The potential deadlock issue was reported by syzbot, and the rest was found through inspection.
I have bunch of clean ups for this driver as well that I'll post once these are in Linus's tree.
Johan
Johan Hovold (4):
USB: legousbtower: fix slab info leak at probe
USB: legousbtower: fix deadlock on disconnect
USB: legousbtower: fix potential NULL-deref on disconnect
USB: legousbtower: fix open after failed reset request
drivers/usb/misc/legousbtower.c | 55 ++++++++++++++------------------- 1 file changed, 23 insertions(+), 32 deletions(-)
Signature · 1 line
2.23.0
PATCH 1/4 USB: legousbtower: fix slab info leak at probe
Make sure to check for short transfers when retrieving the version information at probe to avoid leaking uninitialised slab data when logging it.
--- drivers/usb/misc/legousbtower.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/usb/misc/legousbtower.c +4 -2
@@ -891,8 +891,10 @@ static int tower_probe (struct usb_interface *interface, const struct usb_device get_version_reply,sizeof (*get_version_reply), 1000); -if (result <0 ) { -dev_err (idev,"LEGO USB Tower get version control request failed\n" ); +if (result <sizeof (*get_version_reply)) { +if (result >=0 ) + result = -EIO ; +dev_err (idev,"get version request failed: %d\n" , result); retval = result; goto error; }
Signature · 1 line
2.23.0
PATCH 2/4 USB: legousbtower: fix deadlock on disconnect
In reply to Johan HovoldFix a potential deadlock if disconnect races with open.
Since commit d4ead16f50f9 ("USB: prevent char device open/deregister race") core holds an rw-semaphore while open is called and when releasing the minor number during deregistration. This can lead to an ABBA deadlock if a driver takes a lock in open which it also holds during deregistration.
This effectively reverts commit 78663ecc344b ("USB: disconnect open race in legousbtower") which needlessly introduced this issue after a generic fix for this race had been added to core by commit d4ead16f50f9 ("USB:
prevent char device open/deregister race").
--- drivers/usb/misc/legousbtower.c | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-)
drivers/usb/misc/legousbtower.c +2 -17
@@ -179,7 +179,6 @@ static const struct usb_device_id tower_table[] = { };MODULE_DEVICE_TABLE (usb, tower_table); -static DEFINE_MUTEX (open_disc_mutex);#define LEGO_USB_TOWER_MINOR_BASE 160
@@ -332,18 +331,14 @@ static int tower_open (struct inode *inode, struct file *file) goto exit; } -mutex_lock (&open_disc_mutex); dev =usb_get_intfdata (interface); -if (!dev) { -mutex_unlock (&open_disc_mutex); retval = -ENODEV ; goto exit; }/* lock this device */ if (mutex_lock_interruptible (&dev->lock)) { -mutex_unlock (&open_disc_mutex); retval = -ERESTARTSYS ; goto exit; }
@@ -351,12 +346,10 @@ static int tower_open (struct inode *inode, struct file *file)/* allow opening only once */ if (dev->open_count) { -mutex_unlock (&open_disc_mutex); retval = -EBUSY ; goto unlock_exit; } dev->open_count =1 ; -mutex_unlock (&open_disc_mutex);/* reset the tower */ result = usb_control_msg (dev->udev,
@@ -423,10 +416,9 @@ static int tower_release (struct inode *inode, struct file *file)if (dev ==NULL ) { retval = -ENODEV ; - goto exit_nolock; + goto exit; } -mutex_lock (&open_disc_mutex);if (mutex_lock_interruptible (&dev->lock)) { retval = -ERESTARTSYS ; goto exit;
@@ -456,10 +448,7 @@ static int tower_release (struct inode *inode, struct file *file) unlock_exit:mutex_unlock (&dev->lock); - exit: -mutex_unlock (&open_disc_mutex); -exit_nolock:return retval; }
@@ -912,7 +901,6 @@ static int tower_probe (struct usb_interface *interface, const struct usb_deviceif (retval) {/* something prevented us from registering this driver */ dev_err (idev,"Not able to get a minor for this device.\n" ); -usb_set_intfdata (interface,NULL ); goto error; } dev->minor = interface->minor;
@@ -944,16 +932,13 @@ static void tower_disconnect (struct usb_interface *interface)int minor; dev =usb_get_intfdata (interface); -mutex_lock (&open_disc_mutex); -usb_set_intfdata (interface,NULL ); minor = dev->minor; -/* give back our minor */ +/* give back our minor and prevent further open() */ usb_deregister_dev (interface, &tower_class);mutex_lock (&dev->lock); -mutex_unlock (&open_disc_mutex);/* if the device is not opened, then we clean up right now */ if (!dev->open_count) {
Signature · 1 line
2.23.0
PATCH 3/4 USB: legousbtower: fix potential NULL-deref on disconnect
In reply to Johan HovoldThe driver is using its struct usb_device pointer as an inverted disconnected flag, but was setting it to NULL before making sure all completion handlers had run. This could lead to a NULL-pointer dereference in a number of dev_dbg and dev_err statements in the completion handlers which relies on said pointer.
Fix this by unconditionally stopping all I/O and preventing resubmissions by poisoning the interrupt URBs at disconnect and using a dedicated disconnected flag.
This also makes sure that all I/O has completed by the time the disconnect callback returns.
--- drivers/usb/misc/legousbtower.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-)
drivers/usb/misc/legousbtower.c +15 -11
@@ -190,6 +190,7 @@ struct lego_usb_tower {unsigned char minor;/* the starting minor number for this device */ int open_count;/* number of times this port has been opened */ + unsigned long disconnected:1;char * read_buffer;size_t read_buffer_length;/* this much came in */
@@ -289,8 +290,6 @@ static inline void lego_usb_tower_debug_data(struct device *dev,*/ static inline void tower_delete (struct lego_usb_tower *dev) { -tower_abort_transfers (dev); -/* free data structures */ usb_free_urb (dev->interrupt_in_urb);usb_free_urb (dev->interrupt_out_urb);
@@ -430,7 +429,8 @@ static int tower_release (struct inode *inode, struct file *file) retval = -ENODEV ; goto unlock_exit; } -if (dev->udev ==NULL ) { + +if (dev->disconnected) {/* the device was unplugged before the file was released */ /* unlock here as tower_delete frees dev */
@@ -466,10 +466,9 @@ static void tower_abort_transfers (struct lego_usb_tower *dev)if (dev->interrupt_in_running) { dev->interrupt_in_running =0 ;mb (); -if (dev->udev) -usb_kill_urb (dev->interrupt_in_urb); +usb_kill_urb (dev->interrupt_in_urb); } -if (dev->interrupt_out_busy && dev->udev) +if (dev->interrupt_out_busy)usb_kill_urb (dev->interrupt_out_urb); }
@@ -505,7 +504,7 @@ static __poll_t tower_poll (struct file *file, poll_table *wait) dev = file->private_data; -if (!dev->udev) +if (dev->disconnected)return EPOLLERR |EPOLLHUP ;poll_wait (file, &dev->read_wait, wait);
@@ -552,7 +551,7 @@ static ssize_t tower_read (struct file *file, char __user *buffer, size_t count, }/* verify that the device wasn't unplugged */ -if (dev->udev ==NULL ) { +if (dev->disconnected) { retval = -ENODEV ;pr_err ("No device or device unplugged %d\n" , retval); goto unlock_exit;
@@ -638,7 +637,7 @@ static ssize_t tower_write (struct file *file, const char __user *buffer, size_t }/* verify that the device wasn't unplugged */ -if (dev->udev ==NULL ) { +if (dev->disconnected) { retval = -ENODEV ;pr_err ("No device or device unplugged %d\n" , retval); goto unlock_exit;
@@ -748,7 +747,7 @@ static void tower_interrupt_in_callback (struct urb *urb) resubmit:/* resubmit if we're still running */ -if (dev->interrupt_in_running && dev->udev) { +if (dev->interrupt_in_running) { retval =usb_submit_urb (dev->interrupt_in_urb,GFP_ATOMIC );if (retval) dev_err(&dev->udev->dev,
@@ -813,6 +812,7 @@ static int tower_probe (struct usb_interface *interface, const struct usb_device dev->udev = udev; dev->open_count =0 ; + dev->disconnected =0 ; dev->read_buffer =NULL ; dev->read_buffer_length =0 ;
@@ -938,6 +938,10 @@ static void tower_disconnect (struct usb_interface *interface)/* give back our minor and prevent further open() */ usb_deregister_dev (interface, &tower_class); +/* stop I/O */ +usb_poison_urb (dev->interrupt_in_urb); +usb_poison_urb (dev->interrupt_out_urb); +mutex_lock (&dev->lock);/* if the device is not opened, then we clean up right now */
@@ -945,7 +949,7 @@ static void tower_disconnect (struct usb_interface *interface)mutex_unlock (&dev->lock);tower_delete (dev); }else { - dev->udev =NULL ; + dev->disconnected =1 ;/* wake up pollers */ wake_up_interruptible_all (&dev->read_wait);wake_up_interruptible_all (&dev->write_wait);
Signature · 1 line
2.23.0
PATCH 4/4 USB: legousbtower: fix open after failed reset request
In reply to Johan HovoldThe driver would return with a nonzero open count in case the reset control request failed. This would prevent any further attempts to open the char dev until the device was disconnected.
Fix this by incrementing the open count only on successful open.
--- drivers/usb/misc/legousbtower.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/usb/misc/legousbtower.c +2 -2
@@ -348,7 +348,6 @@ static int tower_open (struct inode *inode, struct file *file) retval = -EBUSY ; goto unlock_exit; } - dev->open_count =1 ;/* reset the tower */ result = usb_control_msg (dev->udev,
@@ -388,13 +387,14 @@ static int tower_open (struct inode *inode, struct file *file)dev_err (&dev->udev->dev,"Couldn't submit interrupt_in_urb %d\n" , retval); dev->interrupt_in_running =0 ; - dev->open_count =0 ; goto unlock_exit; }/* save device in the file's private structure */ file->private_data = dev; + dev->open_count =1 ; + unlock_exit:mutex_unlock (&dev->lock);
Signature · 1 line
2.23.0