Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/5] USB: misc: fix disconnect bugs
@ 2019-10-09 15:38 Johan Hovold
  2019-10-09 15:38 ` [PATCH 1/5] USB: adutux: fix use-after-free on release Johan Hovold
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Johan Hovold @ 2019-10-09 15:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-usb, Keith Packard, Juergen Stuber, Johan Hovold

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(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/5] USB: adutux: fix use-after-free on release
  2019-10-09 15:38 [PATCH 0/5] USB: misc: fix disconnect bugs Johan Hovold
@ 2019-10-09 15:38 ` Johan Hovold
  2019-10-09 15:38 ` [PATCH 2/5] USB: chaoskey: " Johan Hovold
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Johan Hovold @ 2019-10-09 15:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-usb, Keith Packard, Juergen Stuber, Johan Hovold, stable

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 <stable@vger.kernel.org>     # 3.12
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/misc/adutux.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index f9efec719359..6f5edb9fc61e 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -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);
 
-- 
2.23.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/5] USB: chaoskey: fix use-after-free on release
  2019-10-09 15:38 [PATCH 0/5] USB: misc: fix disconnect bugs Johan Hovold
  2019-10-09 15:38 ` [PATCH 1/5] USB: adutux: fix use-after-free on release Johan Hovold
@ 2019-10-09 15:38 ` " Johan Hovold
  2019-10-09 15:38 ` [PATCH 3/5] USB: ldusb: fix NULL-derefs on driver unbind Johan Hovold
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Johan Hovold @ 2019-10-09 15:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-usb, Keith Packard, Juergen Stuber, Johan Hovold, stable

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 <stable@vger.kernel.org>     # 4.1
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/misc/chaoskey.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/misc/chaoskey.c b/drivers/usb/misc/chaoskey.c
index cf5828ce927a..34e6cd6f40d3 100644
--- a/drivers/usb/misc/chaoskey.c
+++ b/drivers/usb/misc/chaoskey.c
@@ -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)
-- 
2.23.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 3/5] USB: ldusb: fix NULL-derefs on driver unbind
  2019-10-09 15:38 [PATCH 0/5] USB: misc: fix disconnect bugs Johan Hovold
  2019-10-09 15:38 ` [PATCH 1/5] USB: adutux: fix use-after-free on release Johan Hovold
  2019-10-09 15:38 ` [PATCH 2/5] USB: chaoskey: " Johan Hovold
@ 2019-10-09 15:38 ` Johan Hovold
  2019-10-09 15:38 ` [PATCH 4/5] USB: legousbtower: fix use-after-free on release Johan Hovold
  2019-10-09 15:38 ` [PATCH 5/5] USB: yurex: fix NULL-derefs on disconnect Johan Hovold
  4 siblings, 0 replies; 8+ messages in thread
From: Johan Hovold @ 2019-10-09 15:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-usb, Keith Packard, Juergen Stuber, Johan Hovold, stable

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 <stable@vger.kernel.org>     # 2.6.13
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/misc/ldusb.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/misc/ldusb.c b/drivers/usb/misc/ldusb.c
index 6581774bdfa4..f3108d85e768 100644
--- a/drivers/usb/misc/ldusb.c
+++ b/drivers/usb/misc/ldusb.c
@@ -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);
-- 
2.23.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 4/5] USB: legousbtower: fix use-after-free on release
  2019-10-09 15:38 [PATCH 0/5] USB: misc: fix disconnect bugs Johan Hovold
                   ` (2 preceding siblings ...)
  2019-10-09 15:38 ` [PATCH 3/5] USB: ldusb: fix NULL-derefs on driver unbind Johan Hovold
@ 2019-10-09 15:38 ` Johan Hovold
  2019-10-09 15:38 ` [PATCH 5/5] USB: yurex: fix NULL-derefs on disconnect Johan Hovold
  4 siblings, 0 replies; 8+ messages in thread
From: Johan Hovold @ 2019-10-09 15:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-usb, Keith Packard, Juergen Stuber, Johan Hovold, stable

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: fef526cae700 ("USB: legousbtower: remove custom debug macro")
Cc: stable <stable@vger.kernel.org>     # 3.12
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/misc/legousbtower.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 44d6a3381804..9d4c52a7ebe0 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -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;
 
-- 
2.23.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 5/5] USB: yurex: fix NULL-derefs on disconnect
  2019-10-09 15:38 [PATCH 0/5] USB: misc: fix disconnect bugs Johan Hovold
                   ` (3 preceding siblings ...)
  2019-10-09 15:38 ` [PATCH 4/5] USB: legousbtower: fix use-after-free on release Johan Hovold
@ 2019-10-09 15:38 ` Johan Hovold
  2019-10-10 11:05   ` Johan Hovold
  4 siblings, 1 reply; 8+ messages in thread
From: Johan Hovold @ 2019-10-09 15:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-usb, Keith Packard, Juergen Stuber, Johan Hovold, stable

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 <stable@vger.kernel.org>     # 3.5: ef61eb43ada6
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/misc/yurex.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/misc/yurex.c b/drivers/usb/misc/yurex.c
index 8d52d4336c29..be0505b8b5d4 100644
--- a/drivers/usb/misc/yurex.c
+++ b/drivers/usb/misc/yurex.c
@@ -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;
-- 
2.23.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 5/5] USB: yurex: fix NULL-derefs on disconnect
  2019-10-09 15:38 ` [PATCH 5/5] USB: yurex: fix NULL-derefs on disconnect Johan Hovold
@ 2019-10-10 11:05   ` Johan Hovold
  2019-10-10 12:24     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 8+ messages in thread
From: Johan Hovold @ 2019-10-10 11:05 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-usb, Keith Packard, Juergen Stuber, Johan Hovold, stable

On Wed, Oct 09, 2019 at 05:38:48PM +0200, Johan Hovold wrote:
> 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 <stable@vger.kernel.org>     # 3.5: ef61eb43ada6
> Signed-off-by: Johan Hovold <johan@kernel.org>

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

Was that one purpose or by mistake?

Johan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 5/5] USB: yurex: fix NULL-derefs on disconnect
  2019-10-10 11:05   ` Johan Hovold
@ 2019-10-10 12:24     ` Greg Kroah-Hartman
  0 siblings, 0 replies; 8+ messages in thread
From: Greg Kroah-Hartman @ 2019-10-10 12:24 UTC (permalink / raw)
  To: Johan Hovold; +Cc: linux-usb, Keith Packard, Juergen Stuber, stable

On Thu, Oct 10, 2019 at 01:05:32PM +0200, Johan Hovold wrote:
> On Wed, Oct 09, 2019 at 05:38:48PM +0200, Johan Hovold wrote:
> > 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 <stable@vger.kernel.org>     # 3.5: ef61eb43ada6
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> 
> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-09 15:38 [PATCH 0/5] USB: misc: fix disconnect bugs Johan Hovold
2019-10-09 15:38 ` [PATCH 1/5] USB: adutux: fix use-after-free on release Johan Hovold
2019-10-09 15:38 ` [PATCH 2/5] USB: chaoskey: " Johan Hovold
2019-10-09 15:38 ` [PATCH 3/5] USB: ldusb: fix NULL-derefs on driver unbind Johan Hovold
2019-10-09 15:38 ` [PATCH 4/5] USB: legousbtower: fix use-after-free on release Johan Hovold
2019-10-09 15:38 ` [PATCH 5/5] USB: yurex: fix NULL-derefs on disconnect Johan Hovold
2019-10-10 11:05   ` Johan Hovold
2019-10-10 12:24     ` Greg Kroah-Hartman

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org linux-usb@archiver.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/ public-inbox