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)
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#opti...
Rule: add the tag "Cc: stable@vger.kernel.org" in the sign-off area to have the patch automatically included in the stable tree. Subject: [PATCH] USB: chaoskey: fail open after removal Link: https://lore.kernel.org/stable/20241002132201.552578-1-oneukum%40suse.com
On Wed, Oct 02, 2024 at 03:21:41PM +0200, Oliver Neukum wrote:
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.
I'll take this, but really, the driver should not care about how many times it is opened. That change can happen later, I'll try to dig up the device I have for this driver so that I can test it out...
thanks,
greg k-h
On 04.10.24 15:17, Greg KH wrote:
I'll take this, but really, the driver should not care about how many times it is opened. That change can happen later, I'll try to dig up the device I have for this driver so that I can test it out...
Hi,
I agree. I'll send a patch for you to test.
Regards Oliver
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(&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);
} elsemutex_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
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Jeongjun Park -
kernel test robot -
Oliver Neukum