| Message ID | 20241002132201.552578-1-oneukum@suse.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 422dc0a4d12d0b80dd3aab3fe5943f665ba8f041 |
| Headers | show |
| Series | USB: chaoskey: fail open after removal | expand |
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(&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 >
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)
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(-)