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.

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)")
Rule: add
Link: https://lore.kernel.org/stable/20241002132201.552578-1-oneukum%40suse.com
Link: https://lore.kernel.org/r/20241002132201.552578-1-oneukum@suse.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/usb/misc/chaoskey.c b/drivers/usb/misc/chaoskey.c
index 6fb5140e29b9dd641be37ddd42034e4f6466a12f..e8b63df5f9759807c747b825497214089720463b 100644 (file)
--- 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)