> 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@xxxxxxxx> > Reported-by: syzbot+422188bce66e76020e55@xxxxxxxxxxxxxxxxxxxxxxxxx > 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 >
