USB: prevent char device open/deregister race
This patch (as908) adds central protection in usbcore for the
prototypical race between opening and unregistering a char device.
The spinlock used to protect the minor-numbers array is replaced with
an rwsem, which can remain locked across a call to a driver's open()
method. This guarantees that open() and deregister() will be mutually
exclusive.
The private locks currently used in several individual drivers for
this purpose are no longer necessary, and the patch removes them. The
following USB drivers are affected: usblcd, idmouse, auerswald,
legousbtower, sisusbvga/sisusb, ldusb, adutux, iowarrior, and
usb-skeleton.
As a side effect of this change, usb_deregister_dev() must not be
called while holding a lock that is acquired by open(). Unfortunately
a number of drivers do this, but luckily the solution is simple: call
usb_deregister_dev() before acquiring the lock.
In addition to these changes (and their consequent code
simplifications), the patch fixes a use-after-free bug in adutux and a
race between open() and release() in iowarrior.
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
diff --git a/drivers/usb/misc/idmouse.c b/drivers/usb/misc/idmouse.c
index 8d0e360..e6fd024 100644
--- a/drivers/usb/misc/idmouse.c
+++ b/drivers/usb/misc/idmouse.c
@@ -119,9 +119,6 @@
.id_table = idmouse_table,
};
-/* prevent races between open() and disconnect() */
-static DEFINE_MUTEX(disconnect_mutex);
-
static int idmouse_create_image(struct usb_idmouse *dev)
{
int bytes_read;
@@ -211,21 +208,15 @@
struct usb_interface *interface;
int result;
- /* prevent disconnects */
- mutex_lock(&disconnect_mutex);
-
/* get the interface from minor number and driver information */
interface = usb_find_interface (&idmouse_driver, iminor (inode));
- if (!interface) {
- mutex_unlock(&disconnect_mutex);
+ if (!interface)
return -ENODEV;
- }
+
/* get the device information block from the interface */
dev = usb_get_intfdata(interface);
- if (!dev) {
- mutex_unlock(&disconnect_mutex);
+ if (!dev)
return -ENODEV;
- }
/* lock this device */
down(&dev->sem);
@@ -255,9 +246,6 @@
/* unlock this device */
up(&dev->sem);
-
- /* unlock the disconnect semaphore */
- mutex_unlock(&disconnect_mutex);
return result;
}
@@ -265,15 +253,10 @@
{
struct usb_idmouse *dev;
- /* prevent a race condition with open() */
- mutex_lock(&disconnect_mutex);
-
dev = file->private_data;
- if (dev == NULL) {
- mutex_unlock(&disconnect_mutex);
+ if (dev == NULL)
return -ENODEV;
- }
/* lock our device */
down(&dev->sem);
@@ -281,7 +264,6 @@
/* are we really open? */
if (dev->open <= 0) {
up(&dev->sem);
- mutex_unlock(&disconnect_mutex);
return -ENODEV;
}
@@ -291,12 +273,9 @@
/* the device was unplugged before the file was released */
up(&dev->sem);
idmouse_delete(dev);
- mutex_unlock(&disconnect_mutex);
- return 0;
+ } else {
+ up(&dev->sem);
}
-
- up(&dev->sem);
- mutex_unlock(&disconnect_mutex);
return 0;
}
@@ -391,30 +370,27 @@
{
struct usb_idmouse *dev;
- /* prevent races with open() */
- mutex_lock(&disconnect_mutex);
-
/* get device structure */
dev = usb_get_intfdata(interface);
usb_set_intfdata(interface, NULL);
- /* lock it */
- down(&dev->sem);
-
/* give back our minor */
usb_deregister_dev(interface, &idmouse_class);
+ /* lock it */
+ down(&dev->sem);
+
/* prevent device read, write and ioctl */
dev->present = 0;
- /* unlock */
- up(&dev->sem);
-
/* if the device is opened, idmouse_release will clean this up */
- if (!dev->open)
+ if (!dev->open) {
+ up(&dev->sem);
idmouse_delete(dev);
-
- mutex_unlock(&disconnect_mutex);
+ } else {
+ /* unlock */
+ up(&dev->sem);
+ }
info("%s disconnected", DRIVER_DESC);
}