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