USB: misc: fix disconnect bugs

PATCH In review Johan Hovold 2 participants 8 messages Oct 9, 2019 – Oct 10, 2019

Subsystems (4)

Stable branch USB Lego USB Tower driver USB chaoskey driver

lore

Share & tools

lore

Johan Hovold

author

2019-10-09 15:38 UTC #0 lore raw

cover USB: misc: fix disconnect bugs

This series fixes a number of issues introduced primarily with the conversion to dev_err() and dev_dbg(), which failed to notice that the drivers where using their USB interface and device pointers as disconnected flags (leading to NULL derefs) and that they did not hold references to the structures (leading to use-after-free on release()).

I've already fixed up a few of these USB character device drivers separately, and the uss720 driver has similar bugs that remain to be fixed.

Johan

Johan Hovold (5):
USB: adutux: fix use-after-free on release
USB: chaoskey: fix use-after-free on release
USB: ldusb: fix NULL-derefs on driver unbind
USB: legousbtower: fix use-after-free on release
USB: yurex: fix NULL-derefs on disconnect

 drivers/usb/misc/adutux.c       |  3 ++-
 drivers/usb/misc/chaoskey.c     |  5 +++--
 drivers/usb/misc/ldusb.c        | 24 ++++++++++++------------
 drivers/usb/misc/legousbtower.c |  3 ++-
 drivers/usb/misc/yurex.c        | 11 +++++++----
 5 files changed, 26 insertions(+), 20 deletions(-)

Signature · 1 line

2.23.0

Johan Hovold

author

2019-10-09 15:38 UTC #1 lore raw

PATCH 1/5 USB: adutux: fix use-after-free on release

The driver was accessing its struct usb_device in its release() callback without holding a reference. This would lead to a use-after-free whenever the device was disconnected while the character device was still open.

Fixes 66d4bc30d128 ("USB: adutux: remove custom debug macro")

Cc stable <[email protected]> # 3.12

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/misc/adutux.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/usb/misc/adutux.c +2 -1

index f9efec719359..6f5edb9fc61e 100644
@@ -149,6 +149,7 @@ static void adu_delete(struct adu_device *dev)
 	kfree(dev->read_buffer_secondary);
 	kfree(dev->interrupt_in_buffer);
 	kfree(dev->interrupt_out_buffer);
+	usb_put_dev(dev->udev);
 	kfree(dev);
 }

@@ -664,7 +665,7 @@ static int adu_probe(struct usb_interface *interface,
 
 	mutex_init(&dev->mtx);
 	spin_lock_init(&dev->buflock);
-	dev->udev = udev;
+	dev->udev = usb_get_dev(udev);
 	init_waitqueue_head(&dev->read_wait);
 	init_waitqueue_head(&dev->write_wait);

Signature · 1 line

2.23.0

Johan Hovold

author

2019-10-09 15:38 UTC #2 lore raw

PATCH 2/5 USB: chaoskey: fix use-after-free on release

In reply to Johan Hovold

The driver was accessing its struct usb_interface in its release() callback without holding a reference. This would lead to a use-after-free whenever the device was disconnected while the character device was still open.

Fixes 66e3e591891d ("usb: Add driver for Altus Metrum ChaosKey device (v2)")

Cc stable <[email protected]> # 4.1

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/misc/chaoskey.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/usb/misc/chaoskey.c +3 -2

index cf5828ce927a..34e6cd6f40d3 100644
@@ -98,6 +98,7 @@ static void chaoskey_free(struct chaoskey *dev)
 		usb_free_urb(dev->urb);
 		kfree(dev->name);
 		kfree(dev->buf);
+		usb_put_intf(dev->interface);
 		kfree(dev);
 	}
 }

@@ -145,6 +146,8 @@ static int chaoskey_probe(struct usb_interface *interface,
 	if (dev == NULL)
 		goto out;
 
+	dev->interface = usb_get_intf(interface);
+
 	dev->buf = kmalloc(size, GFP_KERNEL);
 
 	if (dev->buf == NULL)

@@ -174,8 +177,6 @@ static int chaoskey_probe(struct usb_interface *interface,
 			goto out;
 	}
 
-	dev->interface = interface;
-
 	dev->in_ep = in_ep;
 
 	if (le16_to_cpu(udev->descriptor.idVendor) != ALEA_VENDOR_ID)

Signature · 1 line

2.23.0

Johan Hovold

author

2019-10-09 15:38 UTC #3 lore raw

PATCH 3/5 USB: ldusb: fix NULL-derefs on driver unbind

In reply to Johan Hovold

The driver was using its struct usb_interface 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, dev_warn 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 2824bd250f0b ("[PATCH] USB: add ldusb driver")

Cc stable <[email protected]> # 2.6.13

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/misc/ldusb.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

drivers/usb/misc/ldusb.c +12 ~~-12~~

index 6581774bdfa4..f3108d85e768 100644
@@ -153,6 +153,7 @@ MODULE_PARM_DESC(min_interrupt_out_interval, "Minimum interrupt out interval in
 struct ld_usb {
 	struct mutex		mutex;		/* locks this structure */
 	struct usb_interface	*intf;		/* save off the usb interface pointer */
+	unsigned long		disconnected:1;
 
 	int			open_count;	/* number of times this port has been opened */

@@ -192,12 +193,10 @@ static void ld_usb_abort_transfers(struct ld_usb *dev)
 	/* shutdown transfer */
 	if (dev->interrupt_in_running) {
 		dev->interrupt_in_running = 0;
-		if (dev->intf)
-			usb_kill_urb(dev->interrupt_in_urb);
+		usb_kill_urb(dev->interrupt_in_urb);
 	}
 	if (dev->interrupt_out_busy)
-		if (dev->intf)
-			usb_kill_urb(dev->interrupt_out_urb);
+		usb_kill_urb(dev->interrupt_out_urb);
 }
 
 /**

@@ -205,8 +204,6 @@ static void ld_usb_abort_transfers(struct ld_usb *dev)
  */
 static void ld_usb_delete(struct ld_usb *dev)
 {
-	ld_usb_abort_transfers(dev);
-
 	/* free data structures */
 	usb_free_urb(dev->interrupt_in_urb);
 	usb_free_urb(dev->interrupt_out_urb);

@@ -263,7 +260,7 @@ static void ld_usb_interrupt_in_callback(struct urb *urb)
 
 resubmit:
 	/* resubmit if we're still running */
-	if (dev->interrupt_in_running && !dev->buffer_overflow && dev->intf) {
+	if (dev->interrupt_in_running && !dev->buffer_overflow) {
 		retval = usb_submit_urb(dev->interrupt_in_urb, GFP_ATOMIC);
 		if (retval) {
 			dev_err(&dev->intf->dev,

@@ -392,7 +389,7 @@ static int ld_usb_release(struct inode *inode, struct file *file)
 		retval = -ENODEV;
 		goto unlock_exit;
 	}
-	if (dev->intf == NULL) {
+	if (dev->disconnected) {
 		/* the device was unplugged before the file was released */
 		mutex_unlock(&dev->mutex);
 		/* unlock here as ld_usb_delete frees dev */

@@ -423,7 +420,7 @@ static __poll_t ld_usb_poll(struct file *file, poll_table *wait)
 
 	dev = file->private_data;
 
-	if (!dev->intf)
+	if (dev->disconnected)
 		return EPOLLERR | EPOLLHUP;
 
 	poll_wait(file, &dev->read_wait, wait);

@@ -462,7 +459,7 @@ static ssize_t ld_usb_read(struct file *file, char __user *buffer, size_t count,
 	}
 
 	/* verify that the device wasn't unplugged */
-	if (dev->intf == NULL) {
+	if (dev->disconnected) {
 		retval = -ENODEV;
 		printk(KERN_ERR "ldusb: No device or device unplugged %d\n", retval);
 		goto unlock_exit;

@@ -542,7 +539,7 @@ static ssize_t ld_usb_write(struct file *file, const char __user *buffer,
 	}
 
 	/* verify that the device wasn't unplugged */
-	if (dev->intf == NULL) {
+	if (dev->disconnected) {
 		retval = -ENODEV;
 		printk(KERN_ERR "ldusb: No device or device unplugged %d\n", retval);
 		goto unlock_exit;

@@ -764,6 +761,9 @@ static void ld_usb_disconnect(struct usb_interface *intf)
 	/* give back our minor */
 	usb_deregister_dev(intf, &ld_usb_class);
 
+	usb_poison_urb(dev->interrupt_in_urb);
+	usb_poison_urb(dev->interrupt_out_urb);
+
 	mutex_lock(&dev->mutex);
 
 	/* if the device is not opened, then we clean up right now */

@@ -771,7 +771,7 @@ static void ld_usb_disconnect(struct usb_interface *intf)
 		mutex_unlock(&dev->mutex);
 		ld_usb_delete(dev);
 	} else {
-		dev->intf = 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

Johan Hovold

author

2019-10-09 15:38 UTC #4 lore raw

PATCH 4/5 USB: legousbtower: fix use-after-free on release

In reply to Johan Hovold

Fixes fef526cae700 ("USB: legousbtower: remove custom debug macro")

Cc stable <[email protected]> # 3.12

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/misc/legousbtower.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/usb/misc/legousbtower.c +2 -1

index 44d6a3381804..9d4c52a7ebe0 100644
@@ -296,6 +296,7 @@ static inline void tower_delete (struct lego_usb_tower *dev)
 	kfree (dev->read_buffer);
 	kfree (dev->interrupt_in_buffer);
 	kfree (dev->interrupt_out_buffer);
+	usb_put_dev(dev->udev);
 	kfree (dev);
 }

@@ -810,7 +811,7 @@ static int tower_probe (struct usb_interface *interface, const struct usb_device
 
 	mutex_init(&dev->lock);
 
-	dev->udev = udev;
+	dev->udev = usb_get_dev(udev);
 	dev->open_count = 0;
 	dev->disconnected = 0;

Signature · 1 line

2.23.0

Johan Hovold

author

2019-10-09 15:38 UTC #5 lore raw

PATCH 5/5 USB: yurex: fix NULL-derefs on disconnect

In reply to Johan Hovold

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 <[email protected]> # 3.5: ef61eb43ada6

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/misc/yurex.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

drivers/usb/misc/yurex.c +7 -4

index 8d52d4336c29..be0505b8b5d4 100644
@@ -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;

Signature · 1 line

2.23.0

Johan Hovold

author

2019-10-10 11:05 UTC #6 lore raw

Re: [PATCH 5/5] USB: yurex: fix NULL-derefs on disconnect

Quoted 12 lines · Johan Hovold

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.

Quoted 8 lines

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 <[email protected]> # 3.5: ef61eb43ada6
Signed-off-by Johan Hovold <[email protected]>

Greg, I noticed that you picked up all patches in this series except this last one.

Was that one purpose or by mistake?

Johan

Greg Kroah-Hartman

maintainer

2019-10-10 12:24 UTC #7 lore raw

Re: [PATCH 5/5] USB: yurex: fix NULL-derefs on disconnect

Quoted 22 lines · Johan Hovold

Quoted 12 lines · Johan Hovold
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.
Quoted 8 lines
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 <[email protected]> # 3.5: ef61eb43ada6
Signed-off-by Johan Hovold <[email protected]>

Quoted 4 lines

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