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)")
---
 drivers/usb/misc/chaoskey.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

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
>