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.
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); usb_dbg(interface, "release success"); - return 0; + return rv; }
static void chaos_read_callback(struct urb *urb)