Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit d4ead16f authored by Alan Stern's avatar Alan Stern Committed by Greg Kroah-Hartman
Browse files

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: default avatarAlan Stern <stern@rowland.harvard.edu>
Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@suse.de>
parent 55e5fdfa

drivers/usb/core/file.c

+13 −16
Original line number Diff line number Diff line
@@ -16,15 +16,15 @@ 
 */



#include <linux/module.h>

#include <linux/spinlock.h>

#include <linux/errno.h>

#include <linux/rwsem.h>

#include <linux/usb.h>



#include "usb.h"



#define MAX_USB_MINORS	256

static const struct file_operations *usb_minors[MAX_USB_MINORS];

static DEFINE_SPINLOCK(minor_lock);

static DECLARE_RWSEM(minor_rwsem);



static int usb_open(struct inode * inode, struct file * file)

{
@@ -33,14 +33,11 @@ static int usb_open(struct inode * inode, struct file * file) 
	int err = -ENODEV;

	const struct file_operations *old_fops, *new_fops = NULL;



	spin_lock (&minor_lock);

	down_read(&minor_rwsem);

	c = usb_minors[minor];



	if (!c || !(new_fops = fops_get(c))) {

		spin_unlock(&minor_lock);

		return err;

	}

	spin_unlock(&minor_lock);

	if (!c || !(new_fops = fops_get(c)))

		goto done;



	old_fops = file->f_op;

	file->f_op = new_fops;
@@ -52,6 +49,8 @@ static int usb_open(struct inode * inode, struct file * file) 
		file->f_op = fops_get(old_fops);

	}

	fops_put(old_fops);

 done:

	up_read(&minor_rwsem);

	return err;

}
@@ -166,7 +165,7 @@ int usb_register_dev(struct usb_interface *intf, 
	if (class_driver->fops == NULL)

		goto exit;



	spin_lock (&minor_lock);

	down_write(&minor_rwsem);

	for (minor = minor_base; minor < MAX_USB_MINORS; ++minor) {

		if (usb_minors[minor])

			continue;
@@ -176,7 +175,7 @@ int usb_register_dev(struct usb_interface *intf, 
		retval = 0;

		break;

	}

	spin_unlock (&minor_lock);

	up_write(&minor_rwsem);



	if (retval)

		goto exit;
@@ -197,9 +196,9 @@ int usb_register_dev(struct usb_interface *intf, 
	intf->usb_dev = device_create(usb_class->class, &intf->dev,

				      MKDEV(USB_MAJOR, minor), "%s", temp);

	if (IS_ERR(intf->usb_dev)) {

		spin_lock (&minor_lock);

		down_write(&minor_rwsem);

		usb_minors[intf->minor] = NULL;

		spin_unlock (&minor_lock);

		up_write(&minor_rwsem);

		retval = PTR_ERR(intf->usb_dev);

	}

exit:
@@ -236,9 +235,9 @@ void usb_deregister_dev(struct usb_interface *intf, 


	dbg ("removing %d minor", intf->minor);



	spin_lock (&minor_lock);

	down_write(&minor_rwsem);

	usb_minors[intf->minor] = NULL;

	spin_unlock (&minor_lock);

	up_write(&minor_rwsem);



	snprintf(name, BUS_ID_SIZE, class_driver->name, intf->minor - minor_base);

	device_destroy(usb_class->class, MKDEV(USB_MAJOR, intf->minor));
@@ -247,5 +246,3 @@ void usb_deregister_dev(struct usb_interface *intf, 
	destroy_usb_class();

}

EXPORT_SYMBOL(usb_deregister_dev);

drivers/usb/misc/adutux.c

+11 −20
Original line number Diff line number Diff line
@@ -108,8 +108,6 @@ struct adu_device { 
	struct urb*		interrupt_out_urb;

};



/* prevent races between open() and disconnect */

static DEFINE_MUTEX(disconnect_mutex);

static struct usb_driver adu_driver;



static void adu_debug_data(int level, const char *function, int size,
@@ -256,8 +254,6 @@ static int adu_open(struct inode *inode, struct file *file) 


	subminor = iminor(inode);



	mutex_lock(&disconnect_mutex);



	interface = usb_find_interface(&adu_driver, subminor);

	if (!interface) {

		err("%s - error, can't find device for minor %d",
@@ -306,7 +302,6 @@ static int adu_open(struct inode *inode, struct file *file) 
	up(&dev->sem);



exit_no_device:

	mutex_unlock(&disconnect_mutex);

	dbg(2,"%s : leave, return value %d ", __FUNCTION__, retval);



	return retval;
@@ -318,12 +313,6 @@ static int adu_release_internal(struct adu_device *dev) 


	dbg(2," %s : enter", __FUNCTION__);



	if (dev->udev == NULL) {

		/* the device was unplugged before the file was released */

		adu_delete(dev);

		goto exit;

	}



	/* decrement our usage count for the device */

	--dev->open_count;

	dbg(2," %s : open count %d", __FUNCTION__, dev->open_count);
@@ -332,7 +321,6 @@ static int adu_release_internal(struct adu_device *dev) 
		dev->open_count = 0;

	}



exit:

	dbg(2," %s : leave", __FUNCTION__);

	return retval;

}
@@ -367,8 +355,15 @@ static int adu_release(struct inode *inode, struct file *file) 
		goto exit;

	}



	if (dev->udev == NULL) {

		/* the device was unplugged before the file was released */

		up(&dev->sem);

		adu_delete(dev);

		dev = NULL;

	} else {

		/* do the work */

		retval = adu_release_internal(dev);

	}



exit:

	if (dev)
@@ -831,19 +826,17 @@ static void adu_disconnect(struct usb_interface *interface) 


	dbg(2," %s : enter", __FUNCTION__);



	mutex_lock(&disconnect_mutex); /* not interruptible */



	dev = usb_get_intfdata(interface);

	usb_set_intfdata(interface, NULL);



	down(&dev->sem); /* not interruptible */



	minor = dev->minor;



	/* give back our minor */

	usb_deregister_dev(interface, &adu_class);

	dev->minor = 0;



	down(&dev->sem); /* not interruptible */



	/* if the device is not opened, then we clean up right now */

	dbg(2," %s : open count %d", __FUNCTION__, dev->open_count);

	if (!dev->open_count) {
@@ -854,8 +847,6 @@ static void adu_disconnect(struct usb_interface *interface) 
		up(&dev->sem);

	}



	mutex_unlock(&disconnect_mutex);



	dev_info(&interface->dev, "ADU device adutux%d now disconnected",

		 (minor - ADU_MINOR_BASE));

drivers/usb/misc/auerswald.c

+3 −3
Original line number Diff line number Diff line
@@ -2034,12 +2034,12 @@ static void auerswald_disconnect (struct usb_interface *intf) 
	if (!cp)

		return;



	down (&cp->mutex);

	info ("device /dev/%s now disconnecting", cp->name);



	/* give back our USB minor number */

	usb_deregister_dev(intf, &auerswald_class);



	down (&cp->mutex);

	info ("device /dev/%s now disconnecting", cp->name);



	/* Stop the interrupt endpoint */

	auerswald_int_release (cp);

drivers/usb/misc/idmouse.c

+15 −39
Original line number Diff line number Diff line
@@ -119,9 +119,6 @@ static struct usb_driver idmouse_driver = { 
	.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 @@ static int idmouse_open(struct inode *inode, struct file *file) 
	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 @@ static int idmouse_open(struct inode *inode, struct file *file) 


	/* unlock this device */

	up(&dev->sem);



	/* unlock the disconnect semaphore */

	mutex_unlock(&disconnect_mutex);

	return result;

}
@@ -265,15 +253,10 @@ static int idmouse_release(struct inode *inode, struct file *file) 
{

	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 @@ static int idmouse_release(struct inode *inode, struct file *file) 
	/* are we really open? */

	if (dev->open <= 0) {

		up(&dev->sem);

		mutex_unlock(&disconnect_mutex);

		return -ENODEV;

	}
@@ -291,12 +273,9 @@ static int idmouse_release(struct inode *inode, struct file *file) 
		/* 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);

	mutex_unlock(&disconnect_mutex);

	}

	return 0;

}
@@ -391,30 +370,27 @@ static void idmouse_disconnect(struct usb_interface *interface) 
{

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

}

drivers/usb/misc/iowarrior.c

+8 −18
Original line number Diff line number Diff line
@@ -100,8 +100,6 @@ struct iowarrior { 
/*--------------*/

/*    globals   */

/*--------------*/

/* prevent races between open() and disconnect() */

static DECLARE_MUTEX(disconnect_sem);



/*

 *  USB spec identifies 5 second timeouts.
@@ -600,22 +598,18 @@ static int iowarrior_open(struct inode *inode, struct file *file) 


	subminor = iminor(inode);



	/* prevent disconnects */

	down(&disconnect_sem);



	interface = usb_find_interface(&iowarrior_driver, subminor);

	if (!interface) {

		err("%s - error, can't find device for minor %d", __FUNCTION__,

		    subminor);

		retval = -ENODEV;

		goto out;

		return -ENODEV;

	}



	dev = usb_get_intfdata(interface);

	if (!dev) {

		retval = -ENODEV;

		goto out;

	}

	if (!dev)

		return -ENODEV;



	mutex_lock(&dev->mutex);



	/* Only one process can open each device, no sharing. */

	if (dev->opened) {
@@ -636,7 +630,7 @@ static int iowarrior_open(struct inode *inode, struct file *file) 
	retval = 0;



out:

	up(&disconnect_sem);

	mutex_unlock(&dev->mutex);

	return retval;

}
@@ -868,19 +862,16 @@ static void iowarrior_disconnect(struct usb_interface *interface) 
	struct iowarrior *dev;

	int minor;



	/* prevent races with open() */

	down(&disconnect_sem);



	dev = usb_get_intfdata(interface);

	usb_set_intfdata(interface, NULL);



	mutex_lock(&dev->mutex);



	minor = dev->minor;



	/* give back our minor */

	usb_deregister_dev(interface, &iowarrior_class);



	mutex_lock(&dev->mutex);



	/* prevent device read, write and ioctl */

	dev->present = 0;
@@ -898,7 +889,6 @@ static void iowarrior_disconnect(struct usb_interface *interface) 
		/* no process is using the device, cleanup now */

		iowarrior_delete(dev);

	}

	up(&disconnect_sem);



	dev_info(&interface->dev, "I/O-Warror #%d now disconnected\n",

		 minor - IOWARRIOR_MINOR_BASE);