Re: [PATCH] USB: chaoskey: fail open after removal

chaoskey_open() takes the lock only to increase the counter of openings. That means that the mutual exclusion with chaoskey_disconnect() cannot prevent an increase of the counter and chaoskey_open() returning a success.

If that race is hit, chaoskey_disconnect() will happily free all resources associated with the device after it has dropped the lock, as it has read the counter as zero.

To prevent this race chaoskey_open() has to check the presence of the device under the lock. However, the current per device lock cannot be used, because it is a part of the data structure to be freed. Hence an additional global mutex is needed. The issue is as old as the driver.

There were 3 deadlock reports uploaded by syzbot due to this patch. It seems like this patch should be fixed or reverted in its entirety.

Signed-off-by: Oliver Neukum oneukum@suse.com Reported-by: syzbot+422188bce66e76020e55@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=422188bce66e76020e55 Fixes: 66e3e591891da ("usb: Add driver for Altus Metrum ChaosKey device (v2)")

---

drivers/usb/misc/chaoskey.c | 35 ++++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/misc/chaoskey.c b/drivers/usb/misc/chaoskey.c index 6fb5140e29b9..e8b63df5f975 100644 --- a/drivers/usb/misc/chaoskey.c +++ b/drivers/usb/misc/chaoskey.c @@ -27,6 +27,8 @@ static struct usb_class_driver chaoskey_class; static int chaoskey_rng_read(struct hwrng *rng, void *data, size_t max, bool wait); +static DEFINE_MUTEX(chaoskey_list_lock);

#define usb_dbg(usb_if, format, arg...) \ dev_dbg(&(usb_if)->dev, format, ## arg) @@ -230,6 +232,7 @@ static void chaoskey_disconnect(struct usb_interface *interface) if (dev->hwrng_registered) hwrng_unregister(&dev->hwrng);

• mutex_lock(&chaoskey_list_lock); usb_deregister_dev(interface, &chaoskey_class);

usb_set_intfdata(interface, NULL); @@ -244,6 +247,7 @@ static void chaoskey_disconnect(struct usb_interface *interface) } else mutex_unlock(&dev->lock);

• mutex_unlock(&chaoskey_list_lock); usb_dbg(interface, "disconnect done");

} @@ -251,6 +255,7 @@ static int chaoskey_open(struct inode *inode, struct file *file) { struct chaoskey *dev; struct usb_interface *interface;

• int rv = 0;

/* get the interface from minor number and driver information */ interface = usb_find_interface(&chaoskey_driver, iminor(inode)); @@ -266,18 +271,23 @@ static int chaoskey_open(struct inode *inode, struct file *file) } file->private_data = dev;

• mutex_lock(&chaoskey_list_lock); mutex_lock(&dev->lock);

• ++dev->open;

• if (dev->present)

• ++dev->open;
  

• else

• rv = -ENODEV;
  

  mutex_unlock(&dev->lock);
• mutex_unlock(&chaoskey_list_lock);

• usb_dbg(interface, "open success");
• return 0;

• return rv;

} static int chaoskey_release(struct inode *inode, struct file *file) { struct chaoskey *dev = file->private_data; struct usb_interface *interface;

• int rv = 0;

if (dev == NULL) return -ENODEV; @@ -286,14 +296,15 @@ static int chaoskey_release(struct inode *inode, struct file *file) usb_dbg(interface, "release");

• mutex_lock(&chaoskey_list_lock); mutex_lock(&dev->lock);

usb_dbg(interface, "open count at release is %d", dev->open); if (dev->open <= 0) { usb_dbg(interface, "invalid open count (%d)", dev->open);

• mutex_unlock(&dev->lock);
  

• return -ENODEV;
  

• rv = -ENODEV;
  

• goto bail;
  

  }

--dev->open; @@ -302,13 +313,15 @@ static int chaoskey_release(struct inode *inode, struct file *file) if (dev->open == 0) { mutex_unlock(&dev->lock); chaoskey_free(dev);

• } else
  

• 	mutex_unlock(&dev->lock);
  

• } else

• mutex_unlock(&dev->lock);
  

• 	goto destruction;
  

• }
  

• }

+bail:

• mutex_unlock(&dev->lock);

+destruction:

• mutex_lock(&chaoskey_list_lock);

Shouldn't we use mutex_unlock here? I don't know if there's a special reason for writing it this way or if it's a mistake, but doing it this way causes a deadlock due to recursive locking.

Regards,

Jeongjun Park

usb_dbg(interface, "release success");

• return 0;

• return rv;

} static void chaos_read_callback(struct urb *urb) -- 2.46.1