* [PATCH 0/4] USB: legousbtower: misc fixes
@ 2019-09-19 8:18 Johan Hovold
2019-09-19 8:18 ` [PATCH 1/4] USB: legousbtower: fix slab info leak at probe Johan Hovold
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Johan Hovold @ 2019-09-19 8:18 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-usb, Alan Stern, Oliver Neukum, Johan Hovold
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(-)
--
2.23.0
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/4] USB: legousbtower: fix slab info leak at probe
2019-09-19 8:18 [PATCH 0/4] USB: legousbtower: misc fixes Johan Hovold
@ 2019-09-19 8:18 ` Johan Hovold
2019-09-19 8:18 ` [PATCH 2/4] USB: legousbtower: fix deadlock on disconnect Johan Hovold
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Johan Hovold @ 2019-09-19 8:18 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-usb, Alan Stern, Oliver Neukum, Johan Hovold, stable
Make sure to check for short transfers when retrieving the version
information at probe to avoid leaking uninitialised slab data when
logging it.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/usb/misc/legousbtower.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 006cf13b2199..1db07d4dc738 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -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;
}
--
2.23.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/4] USB: legousbtower: fix deadlock on disconnect
2019-09-19 8:18 [PATCH 0/4] USB: legousbtower: misc fixes Johan Hovold
2019-09-19 8:18 ` [PATCH 1/4] USB: legousbtower: fix slab info leak at probe Johan Hovold
@ 2019-09-19 8:18 ` Johan Hovold
2019-09-19 8:18 ` [PATCH 3/4] USB: legousbtower: fix potential NULL-deref " Johan Hovold
2019-09-19 8:18 ` [PATCH 4/4] USB: legousbtower: fix open after failed reset request Johan Hovold
3 siblings, 0 replies; 5+ messages in thread
From: Johan Hovold @ 2019-09-19 8:18 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-usb, Alan Stern, Oliver Neukum, Johan Hovold, stable,
syzbot+f9549f5ee8a5416f0b95
Fix 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").
Fixes: 78663ecc344b ("USB: disconnect open race in legousbtower")
Cc: stable <stable@vger.kernel.org> # 2.6.24
Reported-by: syzbot+f9549f5ee8a5416f0b95@syzkaller.appspotmail.com
Tested-by: syzbot+f9549f5ee8a5416f0b95@syzkaller.appspotmail.com
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/usb/misc/legousbtower.c | 19 ++-----------------
1 file changed, 2 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 1db07d4dc738..773e4188f336 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -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_device
if (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) {
--
2.23.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/4] USB: legousbtower: fix potential NULL-deref on disconnect
2019-09-19 8:18 [PATCH 0/4] USB: legousbtower: misc fixes Johan Hovold
2019-09-19 8:18 ` [PATCH 1/4] USB: legousbtower: fix slab info leak at probe Johan Hovold
2019-09-19 8:18 ` [PATCH 2/4] USB: legousbtower: fix deadlock on disconnect Johan Hovold
@ 2019-09-19 8:18 ` Johan Hovold
2019-09-19 8:18 ` [PATCH 4/4] USB: legousbtower: fix open after failed reset request Johan Hovold
3 siblings, 0 replies; 5+ messages in thread
From: Johan Hovold @ 2019-09-19 8:18 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-usb, Alan Stern, Oliver Neukum, Johan Hovold, stable
The 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.
Fixes: 9d974b2a06e3 ("USB: legousbtower.c: remove err() usage")
Fixes: fef526cae700 ("USB: legousbtower: remove custom debug macro")
Fixes: 4dae99638097 ("USB: legotower: remove custom debug macro and module parameter")
Cc: stable <stable@vger.kernel.org> # 3.5
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/usb/misc/legousbtower.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 773e4188f336..4fa999882635 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -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);
--
2.23.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 4/4] USB: legousbtower: fix open after failed reset request
2019-09-19 8:18 [PATCH 0/4] USB: legousbtower: misc fixes Johan Hovold
` (2 preceding siblings ...)
2019-09-19 8:18 ` [PATCH 3/4] USB: legousbtower: fix potential NULL-deref " Johan Hovold
@ 2019-09-19 8:18 ` Johan Hovold
3 siblings, 0 replies; 5+ messages in thread
From: Johan Hovold @ 2019-09-19 8:18 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-usb, Alan Stern, Oliver Neukum, Johan Hovold
The 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.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/usb/misc/legousbtower.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 4fa999882635..44d6a3381804 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -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);
--
2.23.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-09-19 8:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-19 8:18 [PATCH 0/4] USB: legousbtower: misc fixes Johan Hovold
2019-09-19 8:18 ` [PATCH 1/4] USB: legousbtower: fix slab info leak at probe Johan Hovold
2019-09-19 8:18 ` [PATCH 2/4] USB: legousbtower: fix deadlock on disconnect Johan Hovold
2019-09-19 8:18 ` [PATCH 3/4] USB: legousbtower: fix potential NULL-deref " Johan Hovold
2019-09-19 8:18 ` [PATCH 4/4] USB: legousbtower: fix open after failed reset request Johan Hovold
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).