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/sisusbvga/sisusb.c b/drivers/usb/misc/sisusbvga/sisusb.c
index 6f8b134..9f37ba4 100644
--- a/drivers/usb/misc/sisusbvga/sisusb.c
+++ b/drivers/usb/misc/sisusbvga/sisusb.c
@@ -72,8 +72,6 @@
static struct usb_driver sisusb_driver;
-DEFINE_MUTEX(disconnect_mutex);
-
static void
sisusb_free_buffers(struct sisusb_usb_data *sisusb)
{
@@ -2511,31 +2509,24 @@
struct usb_interface *interface;
int subminor = iminor(inode);
- mutex_lock(&disconnect_mutex);
-
if (!(interface = usb_find_interface(&sisusb_driver, subminor))) {
printk(KERN_ERR "sisusb[%d]: Failed to find interface\n",
subminor);
- mutex_unlock(&disconnect_mutex);
return -ENODEV;
}
- if (!(sisusb = usb_get_intfdata(interface))) {
- mutex_unlock(&disconnect_mutex);
+ if (!(sisusb = usb_get_intfdata(interface)))
return -ENODEV;
- }
mutex_lock(&sisusb->lock);
if (!sisusb->present || !sisusb->ready) {
mutex_unlock(&sisusb->lock);
- mutex_unlock(&disconnect_mutex);
return -ENODEV;
}
if (sisusb->isopen) {
mutex_unlock(&sisusb->lock);
- mutex_unlock(&disconnect_mutex);
return -EBUSY;
}
@@ -2543,7 +2534,6 @@
if (sisusb->sisusb_dev->speed == USB_SPEED_HIGH) {
if (sisusb_init_gfxdevice(sisusb, 0)) {
mutex_unlock(&sisusb->lock);
- mutex_unlock(&disconnect_mutex);
printk(KERN_ERR
"sisusbvga[%d]: Failed to initialize "
"device\n",
@@ -2552,7 +2542,6 @@
}
} else {
mutex_unlock(&sisusb->lock);
- mutex_unlock(&disconnect_mutex);
printk(KERN_ERR
"sisusbvga[%d]: Device not attached to "
"USB 2.0 hub\n",
@@ -2570,8 +2559,6 @@
mutex_unlock(&sisusb->lock);
- mutex_unlock(&disconnect_mutex);
-
return 0;
}
@@ -2601,12 +2588,8 @@
struct sisusb_usb_data *sisusb;
int myminor;
- mutex_lock(&disconnect_mutex);
-
- if (!(sisusb = (struct sisusb_usb_data *)file->private_data)) {
- mutex_unlock(&disconnect_mutex);
+ if (!(sisusb = (struct sisusb_usb_data *)file->private_data))
return -ENODEV;
- }
mutex_lock(&sisusb->lock);
@@ -2626,8 +2609,6 @@
/* decrement the usage count on our device */
kref_put(&sisusb->kref, sisusb_delete);
- mutex_unlock(&disconnect_mutex);
-
return 0;
}
@@ -3383,12 +3364,9 @@
sisusb_console_exit(sisusb);
#endif
- /* The above code doesn't need the disconnect
- * semaphore to be down; its meaning is to
- * protect all other routines from the disconnect
- * case, not the other way round.
- */
- mutex_lock(&disconnect_mutex);
+ minor = sisusb->minor;
+
+ usb_deregister_dev(intf, &usb_sisusb_class);
mutex_lock(&sisusb->lock);
@@ -3396,12 +3374,8 @@
if (!sisusb_wait_all_out_complete(sisusb))
sisusb_kill_all_busy(sisusb);
- minor = sisusb->minor;
-
usb_set_intfdata(intf, NULL);
- usb_deregister_dev(intf, &usb_sisusb_class);
-
#ifdef SISUSB_OLD_CONFIG_COMPAT
if (sisusb->ioctl32registered) {
int ret;
@@ -3426,8 +3400,6 @@
/* decrement our usage count */
kref_put(&sisusb->kref, sisusb_delete);
- mutex_unlock(&disconnect_mutex);
-
printk(KERN_INFO "sisusbvga[%d]: Disconnected\n", minor);
}
diff --git a/drivers/usb/misc/sisusbvga/sisusb_con.c b/drivers/usb/misc/sisusbvga/sisusb_con.c
index 5947afb..8d0edc8 100644
--- a/drivers/usb/misc/sisusbvga/sisusb_con.c
+++ b/drivers/usb/misc/sisusbvga/sisusb_con.c
@@ -214,18 +214,13 @@
* are set up/restored.
*/
- mutex_lock(&disconnect_mutex);
-
- if (!(sisusb = sisusb_get_sisusb(c->vc_num))) {
- mutex_unlock(&disconnect_mutex);
+ if (!(sisusb = sisusb_get_sisusb(c->vc_num)))
return;
- }
mutex_lock(&sisusb->lock);
if (!sisusb_sisusb_valid(sisusb)) {
mutex_unlock(&sisusb->lock);
- mutex_unlock(&disconnect_mutex);
return;
}
@@ -264,8 +259,6 @@
mutex_unlock(&sisusb->lock);
- mutex_unlock(&disconnect_mutex);
-
if (init) {
c->vc_cols = cols;
c->vc_rows = rows;
@@ -284,12 +277,8 @@
* and others, ie not under our control.
*/
- mutex_lock(&disconnect_mutex);
-
- if (!(sisusb = sisusb_get_sisusb(c->vc_num))) {
- mutex_unlock(&disconnect_mutex);
+ if (!(sisusb = sisusb_get_sisusb(c->vc_num)))
return;
- }
mutex_lock(&sisusb->lock);
@@ -314,8 +303,6 @@
/* decrement the usage count on our sisusb */
kref_put(&sisusb->kref, sisusb_delete);
-
- mutex_unlock(&disconnect_mutex);
}
/* interface routine */
@@ -1490,14 +1477,11 @@
{
int i, ret, minor = sisusb->minor;
- mutex_lock(&disconnect_mutex);
-
mutex_lock(&sisusb->lock);
/* Erm.. that should not happen */
if (sisusb->haveconsole || !sisusb->SiS_Pr) {
mutex_unlock(&sisusb->lock);
- mutex_unlock(&disconnect_mutex);
return 1;
}
@@ -1508,14 +1492,12 @@
first > MAX_NR_CONSOLES ||
last > MAX_NR_CONSOLES) {
mutex_unlock(&sisusb->lock);
- mutex_unlock(&disconnect_mutex);
return 1;
}
/* If gfxcore not initialized or no consoles given, quit graciously */
if (!sisusb->gfxinit || first < 1 || last < 1) {
mutex_unlock(&sisusb->lock);
- mutex_unlock(&disconnect_mutex);
return 0;
}
@@ -1526,7 +1508,6 @@
/* Set up text mode (and upload default font) */
if (sisusb_reset_text_mode(sisusb, 1)) {
mutex_unlock(&sisusb->lock);
- mutex_unlock(&disconnect_mutex);
printk(KERN_ERR
"sisusbvga[%d]: Failed to set up text mode\n",
minor);
@@ -1550,7 +1531,6 @@
/* Allocate screen buffer */
if (!(sisusb->scrbuf = (unsigned long)vmalloc(sisusb->scrbuf_size))) {
mutex_unlock(&sisusb->lock);
- mutex_unlock(&disconnect_mutex);
printk(KERN_ERR
"sisusbvga[%d]: Failed to allocate screen buffer\n",
minor);
@@ -1558,7 +1538,6 @@
}
mutex_unlock(&sisusb->lock);
- mutex_unlock(&disconnect_mutex);
/* Now grab the desired console(s) */
ret = take_over_console(&sisusb_con, first - 1, last - 1, 0);
diff --git a/drivers/usb/misc/sisusbvga/sisusb_init.h b/drivers/usb/misc/sisusbvga/sisusb_init.h
index f05f832..864bc0e 100644
--- a/drivers/usb/misc/sisusbvga/sisusb_init.h
+++ b/drivers/usb/misc/sisusbvga/sisusb_init.h
@@ -808,8 +808,6 @@
{ 0x2b,0xc2, 35} /* 0x71 768@576@60 */
};
-extern struct mutex disconnect_mutex;
-
int SiSUSBSetMode(struct SiS_Private *SiS_Pr, unsigned short ModeNo);
int SiSUSBSetVESAMode(struct SiS_Private *SiS_Pr, unsigned short VModeNo);