Commit 422dc0a4 authored by Oliver Neukum's avatar Oliver Neukum Committed by Greg Kroah-Hartman
Browse files

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: default avatarOliver Neukum <oneukum@suse.com>
Reported-by: default avatar <syzbot+422188bce66e76020e55@syzkaller.appspotmail.com>
Closes: https://syzkaller.appspot.com/bug?extid=422188bce66e76020e55
Fixes: 66e3e591 ("usb: Add driver for Altus Metrum ChaosKey device (v2)")
Rule: add
Link: https://lore.kernel.org/stable/20241002132201.552578-1-oneukum%40suse.com
Link: https://lore.kernel.org/r/20241002132201.552578-1-oneukum@suse.com


Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
parent e0aa9614

drivers/usb/misc/chaoskey.c

+24 −11
Original line number Diff line number Diff line
@@ -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);

	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

			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)