[PATCH] USB: chaoskey: fail open after removal

[PATCH] USB: chaoskey: fail open after removal

[Oliver Neukum]

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)
-- 
2.46.1

Re: [PATCH] USB: chaoskey: fail open after removal

[kernel test robot]

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#option-1

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

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] USB: chaoskey: fail open after removal

[Greg KH]

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

Re: [PATCH] USB: chaoskey: fail open after removal

[Oliver Neukum]

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

Re: [PATCH] USB: chaoskey: fail open after removal

[Jeongjun Park]

> 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
>