All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] USB: HID: Fix race between disconnect and hiddev_ioctl
@ 2010-12-06 14:45 Valentine Barshak
  2010-12-06 14:51 ` [PATCH 1/2] " Valentine Barshak
  2010-12-06 15:16   ` Valentine Barshak
  0 siblings, 2 replies; 6+ messages in thread
From: Valentine Barshak @ 2010-12-06 14:45 UTC (permalink / raw)
  To: Jiri Kosina; +Cc: linux-usb, linux-input, linux-kernel

If a USB HID device is disconneceted, the hiddev_ioctl could try
to access invalid hiddev->hid pointer.

The following two patches attempt to fix race condition between
disconnect and hiddev_ioctl and make hiddev_ioctl always return
-ENODEV on a disconnected device.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH 1/2] USB: HID: Fix race between disconnect and hiddev_ioctl
  2010-12-06 14:45 [PATCH 0/2] USB: HID: Fix race between disconnect and hiddev_ioctl Valentine Barshak
@ 2010-12-06 14:51 ` Valentine Barshak
  2010-12-07 14:47   ` Jiri Kosina
  2010-12-06 15:16   ` Valentine Barshak
  1 sibling, 1 reply; 6+ messages in thread
From: Valentine Barshak @ 2010-12-06 14:51 UTC (permalink / raw)
  To: Jiri Kosina; +Cc: linux-usb, linux-input, linux-kernel

A USB HID device can be disconnected at any time.
If this happens right before or while hiddev_ioctl is in progress,
the hiddev_ioctl tries to access invalid hiddev->hid pointer.
When the hid device is disconnected, the hiddev_disconnect()
ends up with a call to hid_device_release() which frees
hid_device, but doesn't set the hiddev->hid pointer to NULL.
If the deallocated memory region has been re-used by the kernel,
this can cause a crash or memory corruption.

Since disconnect can happen at any time, we can't initialize
struct hid_device *hid = hiddev->hid at the beginning of ioctl
and then use it.

This change checks hiddev->exist flag while holding
the existancelock and uses hid_device only if it exists.

Signed-off-by: Valentine Barshak <vbarshak@mvista.com>
---
 drivers/hid/usbhid/hiddev.c |  168 +++++++++++++++++++++++++++++++++----------
 1 files changed, 131 insertions(+), 37 deletions(-)

diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 984feb3..cbb6974 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -585,7 +585,7 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct hiddev_list *list = file->private_data;
 	struct hiddev *hiddev = list->hiddev;
-	struct hid_device *hid = hiddev->hid;
+	struct hid_device *hid;
 	struct usb_device *dev;
 	struct hiddev_collection_info cinfo;
 	struct hiddev_report_info rinfo;
@@ -593,26 +593,33 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	struct hiddev_devinfo dinfo;
 	struct hid_report *report;
 	struct hid_field *field;
-	struct usbhid_device *usbhid = hid->driver_data;
+	struct usbhid_device *usbhid;
 	void __user *user_arg = (void __user *)arg;
 	int i, r;
-	
+
 	/* Called without BKL by compat methods so no BKL taken */
 
 	/* FIXME: Who or what stop this racing with a disconnect ?? */
-	if (!hiddev->exist || !hid)
+	if (!hiddev->exist)
 		return -EIO;
 
-	dev = hid_to_usb_dev(hid);
-
 	switch (cmd) {
 
 	case HIDIOCGVERSION:
 		return put_user(HID_VERSION, (int __user *)arg);
 
 	case HIDIOCAPPLICATION:
-		if (arg < 0 || arg >= hid->maxapplication)
-			return -EINVAL;
+		mutex_lock(&hiddev->existancelock);
+		if (!hiddev->exist) {
+			r = -ENODEV;
+			goto ret_unlock;
+		}
+
+		hid = hiddev->hid;
+		if (arg < 0 || arg >= hid->maxapplication) {
+			r = -EINVAL;
+			goto ret_unlock;
+		}
 
 		for (i = 0; i < hid->maxcollection; i++)
 			if (hid->collection[i].type ==
@@ -620,11 +627,22 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 				break;
 
 		if (i == hid->maxcollection)
-			return -EINVAL;
-
-		return hid->collection[i].usage;
+			r = -EINVAL;
+		else
+			r = hid->collection[i].usage;
+		goto ret_unlock;
 
 	case HIDIOCGDEVINFO:
+		mutex_lock(&hiddev->existancelock);
+		if (!hiddev->exist) {
+			r = -ENODEV;
+			goto ret_unlock;
+		}
+
+		hid = hiddev->hid;
+		dev = hid_to_usb_dev(hid);
+		usbhid = hid->driver_data;
+
 		dinfo.bustype = BUS_USB;
 		dinfo.busnum = dev->bus->busnum;
 		dinfo.devnum = dev->devnum;
@@ -633,6 +651,8 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		dinfo.product = le16_to_cpu(dev->descriptor.idProduct);
 		dinfo.version = le16_to_cpu(dev->descriptor.bcdDevice);
 		dinfo.num_applications = hid->maxapplication;
+		mutex_unlock(&hiddev->existancelock);
+
 		if (copy_to_user(user_arg, &dinfo, sizeof(dinfo)))
 			return -EFAULT;
 
@@ -666,6 +686,7 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			r = hiddev_ioctl_string(hiddev, cmd, user_arg);
 		else
 			r = -ENODEV;
+ret_unlock:
 		mutex_unlock(&hiddev->existancelock);
 		return r;
 
@@ -675,6 +696,7 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			mutex_unlock(&hiddev->existancelock);
 			return -ENODEV;
 		}
+		hid = hiddev->hid;
 		usbhid_init_reports(hid);
 		mutex_unlock(&hiddev->existancelock);
 
@@ -687,14 +709,21 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		if (rinfo.report_type == HID_REPORT_TYPE_OUTPUT)
 			return -EINVAL;
 
-		if ((report = hiddev_lookup_report(hid, &rinfo)) == NULL)
-			return -EINVAL;
-
 		mutex_lock(&hiddev->existancelock);
-		if (hiddev->exist) {
-			usbhid_submit_report(hid, report, USB_DIR_IN);
-			usbhid_wait_io(hid);
+		if (!hiddev->exist) {
+			r = -ENODEV;
+			goto ret_unlock;
+		}
+
+		hid = hiddev->hid;
+		report = hiddev_lookup_report(hid, &rinfo);
+		if (report == NULL) {
+			r = -EINVAL;
+			goto ret_unlock;
 		}
+
+		usbhid_submit_report(hid, report, USB_DIR_IN);
+		usbhid_wait_io(hid);
 		mutex_unlock(&hiddev->existancelock);
 
 		return 0;
@@ -706,14 +735,21 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		if (rinfo.report_type == HID_REPORT_TYPE_INPUT)
 			return -EINVAL;
 
-		if ((report = hiddev_lookup_report(hid, &rinfo)) == NULL)
-			return -EINVAL;
-
 		mutex_lock(&hiddev->existancelock);
-		if (hiddev->exist) {
-			usbhid_submit_report(hid, report, USB_DIR_OUT);
-			usbhid_wait_io(hid);
+		if (!hiddev->exist) {
+			r = -ENODEV;
+			goto ret_unlock;
+		}
+
+		hid = hiddev->hid;
+		report = hiddev_lookup_report(hid, &rinfo);
+		if (report == NULL) {
+			r = -EINVAL;
+			goto ret_unlock;
 		}
+
+		usbhid_submit_report(hid, report, USB_DIR_OUT);
+		usbhid_wait_io(hid);
 		mutex_unlock(&hiddev->existancelock);
 
 		return 0;
@@ -722,10 +758,21 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		if (copy_from_user(&rinfo, user_arg, sizeof(rinfo)))
 			return -EFAULT;
 
-		if ((report = hiddev_lookup_report(hid, &rinfo)) == NULL)
-			return -EINVAL;
+		mutex_lock(&hiddev->existancelock);
+		if (!hiddev->exist) {
+			r = -ENODEV;
+			goto ret_unlock;
+		}
+
+		hid = hiddev->hid;
+		report = hiddev_lookup_report(hid, &rinfo);
+		if (report == NULL) {
+			r = -EINVAL;
+			goto ret_unlock;
+		}
 
 		rinfo.num_fields = report->maxfield;
+		mutex_unlock(&hiddev->existancelock);
 
 		if (copy_to_user(user_arg, &rinfo, sizeof(rinfo)))
 			return -EFAULT;
@@ -737,11 +784,23 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			return -EFAULT;
 		rinfo.report_type = finfo.report_type;
 		rinfo.report_id = finfo.report_id;
-		if ((report = hiddev_lookup_report(hid, &rinfo)) == NULL)
-			return -EINVAL;
+		mutex_lock(&hiddev->existancelock);
+		if (!hiddev->exist) {
+			r = -ENODEV;
+			goto ret_unlock;
+		}
 
-		if (finfo.field_index >= report->maxfield)
-			return -EINVAL;
+		hid = hiddev->hid;
+		report = hiddev_lookup_report(hid, &rinfo);
+		if (report == NULL) {
+			r = -EINVAL;
+			goto ret_unlock;
+		}
+
+		if (finfo.field_index >= report->maxfield) {
+			r = -EINVAL;
+			goto ret_unlock;
+		}
 
 		field = report->field[finfo.field_index];
 		memset(&finfo, 0, sizeof(finfo));
@@ -759,6 +818,7 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		finfo.physical_maximum = field->physical_maximum;
 		finfo.unit_exponent = field->unit_exponent;
 		finfo.unit = field->unit;
+		mutex_unlock(&hiddev->existancelock);
 
 		if (copy_to_user(user_arg, &finfo, sizeof(finfo)))
 			return -EFAULT;
@@ -784,12 +844,22 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		if (copy_from_user(&cinfo, user_arg, sizeof(cinfo)))
 			return -EFAULT;
 
-		if (cinfo.index >= hid->maxcollection)
-			return -EINVAL;
+		mutex_lock(&hiddev->existancelock);
+		if (!hiddev->exist) {
+			r = -ENODEV;
+			goto ret_unlock;
+		}
+
+		hid = hiddev->hid;
+		if (cinfo.index >= hid->maxcollection) {
+			r = -EINVAL;
+			goto ret_unlock;
+		}
 
 		cinfo.type = hid->collection[cinfo.index].type;
 		cinfo.usage = hid->collection[cinfo.index].usage;
 		cinfo.level = hid->collection[cinfo.index].level;
+		mutex_lock(&hiddev->existancelock);
 
 		if (copy_to_user(user_arg, &cinfo, sizeof(cinfo)))
 			return -EFAULT;
@@ -802,24 +872,48 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 		if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGNAME(0))) {
 			int len;
-			if (!hid->name)
-				return 0;
+
+			mutex_lock(&hiddev->existancelock);
+			if (!hiddev->exist) {
+				r = -ENODEV;
+				goto ret_unlock;
+			}
+
+			hid = hiddev->hid;
+			if (!hid->name) {
+				r = 0;
+				goto ret_unlock;
+			}
+
 			len = strlen(hid->name) + 1;
 			if (len > _IOC_SIZE(cmd))
 				 len = _IOC_SIZE(cmd);
-			return copy_to_user(user_arg, hid->name, len) ?
+			r = copy_to_user(user_arg, hid->name, len) ?
 				-EFAULT : len;
+			goto ret_unlock;
 		}
 
 		if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGPHYS(0))) {
 			int len;
-			if (!hid->phys)
-				return 0;
+
+			mutex_lock(&hiddev->existancelock);
+			if (!hiddev->exist) {
+				r = -ENODEV;
+				goto ret_unlock;
+			}
+
+			hid = hiddev->hid;
+			if (!hid->phys) {
+				r = 0;
+				goto ret_unlock;
+			}
+
 			len = strlen(hid->phys) + 1;
 			if (len > _IOC_SIZE(cmd))
 				len = _IOC_SIZE(cmd);
-			return copy_to_user(user_arg, hid->phys, len) ?
+			r = copy_to_user(user_arg, hid->phys, len) ?
 				-EFAULT : len;
+			goto ret_unlock;
 		}
 	}
 	return -EINVAL;
-- 
1.6.0.6


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/2] USB: HID: Consolidate device existence checks in hiddev_ioctl
@ 2010-12-06 15:16   ` Valentine Barshak
  0 siblings, 0 replies; 6+ messages in thread
From: Valentine Barshak @ 2010-12-06 15:16 UTC (permalink / raw)
  To: Jiri Kosina; +Cc: linux-usb, linux-input, linux-kernel

Currently, if the device has been removed before hiddev_ioctl(),
the -EIO is returned. If it's removed while hiddev_ioctl() is in
progress, some commands are still processed fine, others
return -ENODEV. This change takes the "existancelock" before
processing ioctl commands and releases it at the end.
If the device has been removed, always returns -ENODEV.

Signed-off-by: Valentine Barshak <vbarshak@mvista.com>
---
 drivers/hid/usbhid/hiddev.c |  287 +++++++++++++++---------------------------
 1 files changed, 103 insertions(+), 184 deletions(-)

diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index cbb6974..d099cad 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -586,221 +586,167 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	struct hiddev_list *list = file->private_data;
 	struct hiddev *hiddev = list->hiddev;
 	struct hid_device *hid;
-	struct usb_device *dev;
 	struct hiddev_collection_info cinfo;
 	struct hiddev_report_info rinfo;
 	struct hiddev_field_info finfo;
 	struct hiddev_devinfo dinfo;
 	struct hid_report *report;
 	struct hid_field *field;
-	struct usbhid_device *usbhid;
 	void __user *user_arg = (void __user *)arg;
-	int i, r;
+	int i, r = -EINVAL;
 
 	/* Called without BKL by compat methods so no BKL taken */
 
-	/* FIXME: Who or what stop this racing with a disconnect ?? */
-	if (!hiddev->exist)
-		return -EIO;
+	mutex_lock(&hiddev->existancelock);
+	if (!hiddev->exist) {
+		r = -ENODEV;
+		goto ret_unlock;
+	}
+
+	hid = hiddev->hid;
 
 	switch (cmd) {
 
 	case HIDIOCGVERSION:
-		return put_user(HID_VERSION, (int __user *)arg);
+		r = put_user(HID_VERSION, (int __user *)arg) ?
+			-EFAULT : 0;
+		break;
 
 	case HIDIOCAPPLICATION:
-		mutex_lock(&hiddev->existancelock);
-		if (!hiddev->exist) {
-			r = -ENODEV;
-			goto ret_unlock;
-		}
-
-		hid = hiddev->hid;
-		if (arg < 0 || arg >= hid->maxapplication) {
-			r = -EINVAL;
-			goto ret_unlock;
-		}
+		if (arg < 0 || arg >= hid->maxapplication)
+			break;
 
 		for (i = 0; i < hid->maxcollection; i++)
 			if (hid->collection[i].type ==
 			    HID_COLLECTION_APPLICATION && arg-- == 0)
 				break;
 
-		if (i == hid->maxcollection)
-			r = -EINVAL;
-		else
+		if (i < hid->maxcollection)
 			r = hid->collection[i].usage;
-		goto ret_unlock;
+		break;
 
 	case HIDIOCGDEVINFO:
-		mutex_lock(&hiddev->existancelock);
-		if (!hiddev->exist) {
-			r = -ENODEV;
-			goto ret_unlock;
+		{
+			struct usb_device *dev = hid_to_usb_dev(hid);
+			struct usbhid_device *usbhid = hid->driver_data;
+
+			dinfo.bustype = BUS_USB;
+			dinfo.busnum = dev->bus->busnum;
+			dinfo.devnum = dev->devnum;
+			dinfo.ifnum = usbhid->ifnum;
+			dinfo.vendor = le16_to_cpu(dev->descriptor.idVendor);
+			dinfo.product = le16_to_cpu(dev->descriptor.idProduct);
+			dinfo.version = le16_to_cpu(dev->descriptor.bcdDevice);
+			dinfo.num_applications = hid->maxapplication;
+
+			r = copy_to_user(user_arg, &dinfo, sizeof(dinfo)) ?
+				-EFAULT : 0;
+			break;
 		}
 
-		hid = hiddev->hid;
-		dev = hid_to_usb_dev(hid);
-		usbhid = hid->driver_data;
-
-		dinfo.bustype = BUS_USB;
-		dinfo.busnum = dev->bus->busnum;
-		dinfo.devnum = dev->devnum;
-		dinfo.ifnum = usbhid->ifnum;
-		dinfo.vendor = le16_to_cpu(dev->descriptor.idVendor);
-		dinfo.product = le16_to_cpu(dev->descriptor.idProduct);
-		dinfo.version = le16_to_cpu(dev->descriptor.bcdDevice);
-		dinfo.num_applications = hid->maxapplication;
-		mutex_unlock(&hiddev->existancelock);
-
-		if (copy_to_user(user_arg, &dinfo, sizeof(dinfo)))
-			return -EFAULT;
-
-		return 0;
-
 	case HIDIOCGFLAG:
-		if (put_user(list->flags, (int __user *)arg))
-			return -EFAULT;
-
-		return 0;
+		r = put_user(list->flags, (int __user *)arg) ?
+			-EFAULT : 0;
+		break;
 
 	case HIDIOCSFLAG:
 		{
 			int newflags;
-			if (get_user(newflags, (int __user *)arg))
-				return -EFAULT;
+
+			if (get_user(newflags, (int __user *)arg)) {
+				r = -EFAULT;
+				break;
+			}
 
 			if ((newflags & ~HIDDEV_FLAGS) != 0 ||
 			    ((newflags & HIDDEV_FLAG_REPORT) != 0 &&
 			     (newflags & HIDDEV_FLAG_UREF) == 0))
-				return -EINVAL;
+				break;
 
 			list->flags = newflags;
 
-			return 0;
+			r = 0;
+			break;
 		}
 
 	case HIDIOCGSTRING:
-		mutex_lock(&hiddev->existancelock);
-		if (hiddev->exist)
-			r = hiddev_ioctl_string(hiddev, cmd, user_arg);
-		else
-			r = -ENODEV;
-ret_unlock:
-		mutex_unlock(&hiddev->existancelock);
-		return r;
+		r = hiddev_ioctl_string(hiddev, cmd, user_arg);
+		break;
 
 	case HIDIOCINITREPORT:
-		mutex_lock(&hiddev->existancelock);
-		if (!hiddev->exist) {
-			mutex_unlock(&hiddev->existancelock);
-			return -ENODEV;
-		}
-		hid = hiddev->hid;
 		usbhid_init_reports(hid);
-		mutex_unlock(&hiddev->existancelock);
-
-		return 0;
+		r = 0;
+		break;
 
 	case HIDIOCGREPORT:
-		if (copy_from_user(&rinfo, user_arg, sizeof(rinfo)))
-			return -EFAULT;
+		if (copy_from_user(&rinfo, user_arg, sizeof(rinfo))) {
+			r = -EFAULT;
+			break;
+		}
 
 		if (rinfo.report_type == HID_REPORT_TYPE_OUTPUT)
-			return -EINVAL;
+			break;
 
-		mutex_lock(&hiddev->existancelock);
-		if (!hiddev->exist) {
-			r = -ENODEV;
-			goto ret_unlock;
-		}
-
-		hid = hiddev->hid;
 		report = hiddev_lookup_report(hid, &rinfo);
-		if (report == NULL) {
-			r = -EINVAL;
-			goto ret_unlock;
-		}
+		if (report == NULL)
+			break;
 
 		usbhid_submit_report(hid, report, USB_DIR_IN);
 		usbhid_wait_io(hid);
-		mutex_unlock(&hiddev->existancelock);
 
-		return 0;
+		r = 0;
+		break;
 
 	case HIDIOCSREPORT:
-		if (copy_from_user(&rinfo, user_arg, sizeof(rinfo)))
-			return -EFAULT;
+		if (copy_from_user(&rinfo, user_arg, sizeof(rinfo))) {
+			r = -EFAULT;
+			break;
+		}
 
 		if (rinfo.report_type == HID_REPORT_TYPE_INPUT)
-			return -EINVAL;
-
-		mutex_lock(&hiddev->existancelock);
-		if (!hiddev->exist) {
-			r = -ENODEV;
-			goto ret_unlock;
-		}
+			break;
 
-		hid = hiddev->hid;
 		report = hiddev_lookup_report(hid, &rinfo);
-		if (report == NULL) {
-			r = -EINVAL;
-			goto ret_unlock;
-		}
+		if (report == NULL)
+			break;
 
 		usbhid_submit_report(hid, report, USB_DIR_OUT);
 		usbhid_wait_io(hid);
-		mutex_unlock(&hiddev->existancelock);
 
-		return 0;
+		r = 0;
+		break;
 
 	case HIDIOCGREPORTINFO:
-		if (copy_from_user(&rinfo, user_arg, sizeof(rinfo)))
-			return -EFAULT;
-
-		mutex_lock(&hiddev->existancelock);
-		if (!hiddev->exist) {
-			r = -ENODEV;
-			goto ret_unlock;
+		if (copy_from_user(&rinfo, user_arg, sizeof(rinfo))) {
+			r = -EFAULT;
+			break;
 		}
 
-		hid = hiddev->hid;
 		report = hiddev_lookup_report(hid, &rinfo);
-		if (report == NULL) {
-			r = -EINVAL;
-			goto ret_unlock;
-		}
+		if (report == NULL)
+			break;
 
 		rinfo.num_fields = report->maxfield;
-		mutex_unlock(&hiddev->existancelock);
-
-		if (copy_to_user(user_arg, &rinfo, sizeof(rinfo)))
-			return -EFAULT;
 
-		return 0;
+		r = copy_to_user(user_arg, &rinfo, sizeof(rinfo)) ?
+			-EFAULT : 0;
+		break;
 
 	case HIDIOCGFIELDINFO:
-		if (copy_from_user(&finfo, user_arg, sizeof(finfo)))
-			return -EFAULT;
+		if (copy_from_user(&finfo, user_arg, sizeof(finfo))) {
+			r = -EFAULT;
+			break;
+		}
+
 		rinfo.report_type = finfo.report_type;
 		rinfo.report_id = finfo.report_id;
-		mutex_lock(&hiddev->existancelock);
-		if (!hiddev->exist) {
-			r = -ENODEV;
-			goto ret_unlock;
-		}
 
-		hid = hiddev->hid;
 		report = hiddev_lookup_report(hid, &rinfo);
-		if (report == NULL) {
-			r = -EINVAL;
-			goto ret_unlock;
-		}
+		if (report == NULL)
+			break;
 
-		if (finfo.field_index >= report->maxfield) {
-			r = -EINVAL;
-			goto ret_unlock;
-		}
+		if (finfo.field_index >= report->maxfield)
+			break;
 
 		field = report->field[finfo.field_index];
 		memset(&finfo, 0, sizeof(finfo));
@@ -818,12 +764,10 @@ ret_unlock:
 		finfo.physical_maximum = field->physical_maximum;
 		finfo.unit_exponent = field->unit_exponent;
 		finfo.unit = field->unit;
-		mutex_unlock(&hiddev->existancelock);
 
-		if (copy_to_user(user_arg, &finfo, sizeof(finfo)))
-			return -EFAULT;
-
-		return 0;
+		r = copy_to_user(user_arg, &finfo, sizeof(finfo)) ?
+			-EFAULT : 0;
+		break;
 
 	case HIDIOCGUCODE:
 		/* fall through */
@@ -832,57 +776,36 @@ ret_unlock:
 	case HIDIOCGUSAGES:
 	case HIDIOCSUSAGES:
 	case HIDIOCGCOLLECTIONINDEX:
-		mutex_lock(&hiddev->existancelock);
-		if (hiddev->exist)
-			r = hiddev_ioctl_usage(hiddev, cmd, user_arg);
-		else
-			r = -ENODEV;
-		mutex_unlock(&hiddev->existancelock);
-		return r;
+		r = hiddev_ioctl_usage(hiddev, cmd, user_arg);
+		break;
 
 	case HIDIOCGCOLLECTIONINFO:
-		if (copy_from_user(&cinfo, user_arg, sizeof(cinfo)))
-			return -EFAULT;
-
-		mutex_lock(&hiddev->existancelock);
-		if (!hiddev->exist) {
-			r = -ENODEV;
-			goto ret_unlock;
+		if (copy_from_user(&cinfo, user_arg, sizeof(cinfo))) {
+			r = -EFAULT;
+			break;
 		}
 
-		hid = hiddev->hid;
-		if (cinfo.index >= hid->maxcollection) {
-			r = -EINVAL;
-			goto ret_unlock;
-		}
+		if (cinfo.index >= hid->maxcollection)
+			break;
 
 		cinfo.type = hid->collection[cinfo.index].type;
 		cinfo.usage = hid->collection[cinfo.index].usage;
 		cinfo.level = hid->collection[cinfo.index].level;
-		mutex_lock(&hiddev->existancelock);
 
-		if (copy_to_user(user_arg, &cinfo, sizeof(cinfo)))
-			return -EFAULT;
-		return 0;
+		r = copy_to_user(user_arg, &cinfo, sizeof(cinfo)) ?
+			-EFAULT : 0;
+		break;
 
 	default:
-
 		if (_IOC_TYPE(cmd) != 'H' || _IOC_DIR(cmd) != _IOC_READ)
-			return -EINVAL;
+			break;
 
 		if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGNAME(0))) {
 			int len;
 
-			mutex_lock(&hiddev->existancelock);
-			if (!hiddev->exist) {
-				r = -ENODEV;
-				goto ret_unlock;
-			}
-
-			hid = hiddev->hid;
 			if (!hid->name) {
 				r = 0;
-				goto ret_unlock;
+				break;
 			}
 
 			len = strlen(hid->name) + 1;
@@ -890,22 +813,15 @@ ret_unlock:
 				 len = _IOC_SIZE(cmd);
 			r = copy_to_user(user_arg, hid->name, len) ?
 				-EFAULT : len;
-			goto ret_unlock;
+			break;
 		}
 
 		if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGPHYS(0))) {
 			int len;
 
-			mutex_lock(&hiddev->existancelock);
-			if (!hiddev->exist) {
-				r = -ENODEV;
-				goto ret_unlock;
-			}
-
-			hid = hiddev->hid;
 			if (!hid->phys) {
 				r = 0;
-				goto ret_unlock;
+				break;
 			}
 
 			len = strlen(hid->phys) + 1;
@@ -913,10 +829,13 @@ ret_unlock:
 				len = _IOC_SIZE(cmd);
 			r = copy_to_user(user_arg, hid->phys, len) ?
 				-EFAULT : len;
-			goto ret_unlock;
+			break;
 		}
 	}
-	return -EINVAL;
+
+ret_unlock:
+	mutex_unlock(&hiddev->existancelock);
+	return r;
 }
 
 #ifdef CONFIG_COMPAT
-- 
1.6.0.6


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/2] USB: HID: Consolidate device existence checks in hiddev_ioctl
@ 2010-12-06 15:16   ` Valentine Barshak
  0 siblings, 0 replies; 6+ messages in thread
From: Valentine Barshak @ 2010-12-06 15:16 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA,
	linux-input-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

Currently, if the device has been removed before hiddev_ioctl(),
the -EIO is returned. If it's removed while hiddev_ioctl() is in
progress, some commands are still processed fine, others
return -ENODEV. This change takes the "existancelock" before
processing ioctl commands and releases it at the end.
If the device has been removed, always returns -ENODEV.

Signed-off-by: Valentine Barshak <vbarshak-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>
---
 drivers/hid/usbhid/hiddev.c |  287 +++++++++++++++---------------------------
 1 files changed, 103 insertions(+), 184 deletions(-)

diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index cbb6974..d099cad 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -586,221 +586,167 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	struct hiddev_list *list = file->private_data;
 	struct hiddev *hiddev = list->hiddev;
 	struct hid_device *hid;
-	struct usb_device *dev;
 	struct hiddev_collection_info cinfo;
 	struct hiddev_report_info rinfo;
 	struct hiddev_field_info finfo;
 	struct hiddev_devinfo dinfo;
 	struct hid_report *report;
 	struct hid_field *field;
-	struct usbhid_device *usbhid;
 	void __user *user_arg = (void __user *)arg;
-	int i, r;
+	int i, r = -EINVAL;
 
 	/* Called without BKL by compat methods so no BKL taken */
 
-	/* FIXME: Who or what stop this racing with a disconnect ?? */
-	if (!hiddev->exist)
-		return -EIO;
+	mutex_lock(&hiddev->existancelock);
+	if (!hiddev->exist) {
+		r = -ENODEV;
+		goto ret_unlock;
+	}
+
+	hid = hiddev->hid;
 
 	switch (cmd) {
 
 	case HIDIOCGVERSION:
-		return put_user(HID_VERSION, (int __user *)arg);
+		r = put_user(HID_VERSION, (int __user *)arg) ?
+			-EFAULT : 0;
+		break;
 
 	case HIDIOCAPPLICATION:
-		mutex_lock(&hiddev->existancelock);
-		if (!hiddev->exist) {
-			r = -ENODEV;
-			goto ret_unlock;
-		}
-
-		hid = hiddev->hid;
-		if (arg < 0 || arg >= hid->maxapplication) {
-			r = -EINVAL;
-			goto ret_unlock;
-		}
+		if (arg < 0 || arg >= hid->maxapplication)
+			break;
 
 		for (i = 0; i < hid->maxcollection; i++)
 			if (hid->collection[i].type ==
 			    HID_COLLECTION_APPLICATION && arg-- == 0)
 				break;
 
-		if (i == hid->maxcollection)
-			r = -EINVAL;
-		else
+		if (i < hid->maxcollection)
 			r = hid->collection[i].usage;
-		goto ret_unlock;
+		break;
 
 	case HIDIOCGDEVINFO:
-		mutex_lock(&hiddev->existancelock);
-		if (!hiddev->exist) {
-			r = -ENODEV;
-			goto ret_unlock;
+		{
+			struct usb_device *dev = hid_to_usb_dev(hid);
+			struct usbhid_device *usbhid = hid->driver_data;
+
+			dinfo.bustype = BUS_USB;
+			dinfo.busnum = dev->bus->busnum;
+			dinfo.devnum = dev->devnum;
+			dinfo.ifnum = usbhid->ifnum;
+			dinfo.vendor = le16_to_cpu(dev->descriptor.idVendor);
+			dinfo.product = le16_to_cpu(dev->descriptor.idProduct);
+			dinfo.version = le16_to_cpu(dev->descriptor.bcdDevice);
+			dinfo.num_applications = hid->maxapplication;
+
+			r = copy_to_user(user_arg, &dinfo, sizeof(dinfo)) ?
+				-EFAULT : 0;
+			break;
 		}
 
-		hid = hiddev->hid;
-		dev = hid_to_usb_dev(hid);
-		usbhid = hid->driver_data;
-
-		dinfo.bustype = BUS_USB;
-		dinfo.busnum = dev->bus->busnum;
-		dinfo.devnum = dev->devnum;
-		dinfo.ifnum = usbhid->ifnum;
-		dinfo.vendor = le16_to_cpu(dev->descriptor.idVendor);
-		dinfo.product = le16_to_cpu(dev->descriptor.idProduct);
-		dinfo.version = le16_to_cpu(dev->descriptor.bcdDevice);
-		dinfo.num_applications = hid->maxapplication;
-		mutex_unlock(&hiddev->existancelock);
-
-		if (copy_to_user(user_arg, &dinfo, sizeof(dinfo)))
-			return -EFAULT;
-
-		return 0;
-
 	case HIDIOCGFLAG:
-		if (put_user(list->flags, (int __user *)arg))
-			return -EFAULT;
-
-		return 0;
+		r = put_user(list->flags, (int __user *)arg) ?
+			-EFAULT : 0;
+		break;
 
 	case HIDIOCSFLAG:
 		{
 			int newflags;
-			if (get_user(newflags, (int __user *)arg))
-				return -EFAULT;
+
+			if (get_user(newflags, (int __user *)arg)) {
+				r = -EFAULT;
+				break;
+			}
 
 			if ((newflags & ~HIDDEV_FLAGS) != 0 ||
 			    ((newflags & HIDDEV_FLAG_REPORT) != 0 &&
 			     (newflags & HIDDEV_FLAG_UREF) == 0))
-				return -EINVAL;
+				break;
 
 			list->flags = newflags;
 
-			return 0;
+			r = 0;
+			break;
 		}
 
 	case HIDIOCGSTRING:
-		mutex_lock(&hiddev->existancelock);
-		if (hiddev->exist)
-			r = hiddev_ioctl_string(hiddev, cmd, user_arg);
-		else
-			r = -ENODEV;
-ret_unlock:
-		mutex_unlock(&hiddev->existancelock);
-		return r;
+		r = hiddev_ioctl_string(hiddev, cmd, user_arg);
+		break;
 
 	case HIDIOCINITREPORT:
-		mutex_lock(&hiddev->existancelock);
-		if (!hiddev->exist) {
-			mutex_unlock(&hiddev->existancelock);
-			return -ENODEV;
-		}
-		hid = hiddev->hid;
 		usbhid_init_reports(hid);
-		mutex_unlock(&hiddev->existancelock);
-
-		return 0;
+		r = 0;
+		break;
 
 	case HIDIOCGREPORT:
-		if (copy_from_user(&rinfo, user_arg, sizeof(rinfo)))
-			return -EFAULT;
+		if (copy_from_user(&rinfo, user_arg, sizeof(rinfo))) {
+			r = -EFAULT;
+			break;
+		}
 
 		if (rinfo.report_type == HID_REPORT_TYPE_OUTPUT)
-			return -EINVAL;
+			break;
 
-		mutex_lock(&hiddev->existancelock);
-		if (!hiddev->exist) {
-			r = -ENODEV;
-			goto ret_unlock;
-		}
-
-		hid = hiddev->hid;
 		report = hiddev_lookup_report(hid, &rinfo);
-		if (report == NULL) {
-			r = -EINVAL;
-			goto ret_unlock;
-		}
+		if (report == NULL)
+			break;
 
 		usbhid_submit_report(hid, report, USB_DIR_IN);
 		usbhid_wait_io(hid);
-		mutex_unlock(&hiddev->existancelock);
 
-		return 0;
+		r = 0;
+		break;
 
 	case HIDIOCSREPORT:
-		if (copy_from_user(&rinfo, user_arg, sizeof(rinfo)))
-			return -EFAULT;
+		if (copy_from_user(&rinfo, user_arg, sizeof(rinfo))) {
+			r = -EFAULT;
+			break;
+		}
 
 		if (rinfo.report_type == HID_REPORT_TYPE_INPUT)
-			return -EINVAL;
-
-		mutex_lock(&hiddev->existancelock);
-		if (!hiddev->exist) {
-			r = -ENODEV;
-			goto ret_unlock;
-		}
+			break;
 
-		hid = hiddev->hid;
 		report = hiddev_lookup_report(hid, &rinfo);
-		if (report == NULL) {
-			r = -EINVAL;
-			goto ret_unlock;
-		}
+		if (report == NULL)
+			break;
 
 		usbhid_submit_report(hid, report, USB_DIR_OUT);
 		usbhid_wait_io(hid);
-		mutex_unlock(&hiddev->existancelock);
 
-		return 0;
+		r = 0;
+		break;
 
 	case HIDIOCGREPORTINFO:
-		if (copy_from_user(&rinfo, user_arg, sizeof(rinfo)))
-			return -EFAULT;
-
-		mutex_lock(&hiddev->existancelock);
-		if (!hiddev->exist) {
-			r = -ENODEV;
-			goto ret_unlock;
+		if (copy_from_user(&rinfo, user_arg, sizeof(rinfo))) {
+			r = -EFAULT;
+			break;
 		}
 
-		hid = hiddev->hid;
 		report = hiddev_lookup_report(hid, &rinfo);
-		if (report == NULL) {
-			r = -EINVAL;
-			goto ret_unlock;
-		}
+		if (report == NULL)
+			break;
 
 		rinfo.num_fields = report->maxfield;
-		mutex_unlock(&hiddev->existancelock);
-
-		if (copy_to_user(user_arg, &rinfo, sizeof(rinfo)))
-			return -EFAULT;
 
-		return 0;
+		r = copy_to_user(user_arg, &rinfo, sizeof(rinfo)) ?
+			-EFAULT : 0;
+		break;
 
 	case HIDIOCGFIELDINFO:
-		if (copy_from_user(&finfo, user_arg, sizeof(finfo)))
-			return -EFAULT;
+		if (copy_from_user(&finfo, user_arg, sizeof(finfo))) {
+			r = -EFAULT;
+			break;
+		}
+
 		rinfo.report_type = finfo.report_type;
 		rinfo.report_id = finfo.report_id;
-		mutex_lock(&hiddev->existancelock);
-		if (!hiddev->exist) {
-			r = -ENODEV;
-			goto ret_unlock;
-		}
 
-		hid = hiddev->hid;
 		report = hiddev_lookup_report(hid, &rinfo);
-		if (report == NULL) {
-			r = -EINVAL;
-			goto ret_unlock;
-		}
+		if (report == NULL)
+			break;
 
-		if (finfo.field_index >= report->maxfield) {
-			r = -EINVAL;
-			goto ret_unlock;
-		}
+		if (finfo.field_index >= report->maxfield)
+			break;
 
 		field = report->field[finfo.field_index];
 		memset(&finfo, 0, sizeof(finfo));
@@ -818,12 +764,10 @@ ret_unlock:
 		finfo.physical_maximum = field->physical_maximum;
 		finfo.unit_exponent = field->unit_exponent;
 		finfo.unit = field->unit;
-		mutex_unlock(&hiddev->existancelock);
 
-		if (copy_to_user(user_arg, &finfo, sizeof(finfo)))
-			return -EFAULT;
-
-		return 0;
+		r = copy_to_user(user_arg, &finfo, sizeof(finfo)) ?
+			-EFAULT : 0;
+		break;
 
 	case HIDIOCGUCODE:
 		/* fall through */
@@ -832,57 +776,36 @@ ret_unlock:
 	case HIDIOCGUSAGES:
 	case HIDIOCSUSAGES:
 	case HIDIOCGCOLLECTIONINDEX:
-		mutex_lock(&hiddev->existancelock);
-		if (hiddev->exist)
-			r = hiddev_ioctl_usage(hiddev, cmd, user_arg);
-		else
-			r = -ENODEV;
-		mutex_unlock(&hiddev->existancelock);
-		return r;
+		r = hiddev_ioctl_usage(hiddev, cmd, user_arg);
+		break;
 
 	case HIDIOCGCOLLECTIONINFO:
-		if (copy_from_user(&cinfo, user_arg, sizeof(cinfo)))
-			return -EFAULT;
-
-		mutex_lock(&hiddev->existancelock);
-		if (!hiddev->exist) {
-			r = -ENODEV;
-			goto ret_unlock;
+		if (copy_from_user(&cinfo, user_arg, sizeof(cinfo))) {
+			r = -EFAULT;
+			break;
 		}
 
-		hid = hiddev->hid;
-		if (cinfo.index >= hid->maxcollection) {
-			r = -EINVAL;
-			goto ret_unlock;
-		}
+		if (cinfo.index >= hid->maxcollection)
+			break;
 
 		cinfo.type = hid->collection[cinfo.index].type;
 		cinfo.usage = hid->collection[cinfo.index].usage;
 		cinfo.level = hid->collection[cinfo.index].level;
-		mutex_lock(&hiddev->existancelock);
 
-		if (copy_to_user(user_arg, &cinfo, sizeof(cinfo)))
-			return -EFAULT;
-		return 0;
+		r = copy_to_user(user_arg, &cinfo, sizeof(cinfo)) ?
+			-EFAULT : 0;
+		break;
 
 	default:
-
 		if (_IOC_TYPE(cmd) != 'H' || _IOC_DIR(cmd) != _IOC_READ)
-			return -EINVAL;
+			break;
 
 		if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGNAME(0))) {
 			int len;
 
-			mutex_lock(&hiddev->existancelock);
-			if (!hiddev->exist) {
-				r = -ENODEV;
-				goto ret_unlock;
-			}
-
-			hid = hiddev->hid;
 			if (!hid->name) {
 				r = 0;
-				goto ret_unlock;
+				break;
 			}
 
 			len = strlen(hid->name) + 1;
@@ -890,22 +813,15 @@ ret_unlock:
 				 len = _IOC_SIZE(cmd);
 			r = copy_to_user(user_arg, hid->name, len) ?
 				-EFAULT : len;
-			goto ret_unlock;
+			break;
 		}
 
 		if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGPHYS(0))) {
 			int len;
 
-			mutex_lock(&hiddev->existancelock);
-			if (!hiddev->exist) {
-				r = -ENODEV;
-				goto ret_unlock;
-			}
-
-			hid = hiddev->hid;
 			if (!hid->phys) {
 				r = 0;
-				goto ret_unlock;
+				break;
 			}
 
 			len = strlen(hid->phys) + 1;
@@ -913,10 +829,13 @@ ret_unlock:
 				len = _IOC_SIZE(cmd);
 			r = copy_to_user(user_arg, hid->phys, len) ?
 				-EFAULT : len;
-			goto ret_unlock;
+			break;
 		}
 	}
-	return -EINVAL;
+
+ret_unlock:
+	mutex_unlock(&hiddev->existancelock);
+	return r;
 }
 
 #ifdef CONFIG_COMPAT
-- 
1.6.0.6

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] USB: HID: Fix race between disconnect and hiddev_ioctl
  2010-12-06 14:51 ` [PATCH 1/2] " Valentine Barshak
@ 2010-12-07 14:47   ` Jiri Kosina
  2010-12-07 15:02     ` Valentine Barshak
  0 siblings, 1 reply; 6+ messages in thread
From: Jiri Kosina @ 2010-12-07 14:47 UTC (permalink / raw)
  To: Valentine Barshak; +Cc: linux-usb, linux-input, linux-kernel

On Mon, 6 Dec 2010, Valentine Barshak wrote:

> A USB HID device can be disconnected at any time.
> If this happens right before or while hiddev_ioctl is in progress,
> the hiddev_ioctl tries to access invalid hiddev->hid pointer.
> When the hid device is disconnected, the hiddev_disconnect()
> ends up with a call to hid_device_release() which frees
> hid_device, but doesn't set the hiddev->hid pointer to NULL.
> If the deallocated memory region has been re-used by the kernel,
> this can cause a crash or memory corruption.
> 
> Since disconnect can happen at any time, we can't initialize
> struct hid_device *hid = hiddev->hid at the beginning of ioctl
> and then use it.
> 
> This change checks hiddev->exist flag while holding
> the existancelock and uses hid_device only if it exists.

The code duplication isn't really particularly nice, but I don't see any 
way around it that wouldn't complicate things even more.

I will be applying the patches, thanks.

(BTW your patches didn't reach mail mailbox, I had to dig them out from 
mailinglist -- did you receive bounces by any chance?).

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] USB: HID: Fix race between disconnect and hiddev_ioctl
  2010-12-07 14:47   ` Jiri Kosina
@ 2010-12-07 15:02     ` Valentine Barshak
  0 siblings, 0 replies; 6+ messages in thread
From: Valentine Barshak @ 2010-12-07 15:02 UTC (permalink / raw)
  To: Jiri Kosina; +Cc: linux-usb, linux-input, linux-kernel

Jiri Kosina wrote:
> On Mon, 6 Dec 2010, Valentine Barshak wrote:
>
>   
>> A USB HID device can be disconnected at any time.
>> If this happens right before or while hiddev_ioctl is in progress,
>> the hiddev_ioctl tries to access invalid hiddev->hid pointer.
>> When the hid device is disconnected, the hiddev_disconnect()
>> ends up with a call to hid_device_release() which frees
>> hid_device, but doesn't set the hiddev->hid pointer to NULL.
>> If the deallocated memory region has been re-used by the kernel,
>> this can cause a crash or memory corruption.
>>
>> Since disconnect can happen at any time, we can't initialize
>> struct hid_device *hid = hiddev->hid at the beginning of ioctl
>> and then use it.
>>
>> This change checks hiddev->exist flag while holding
>> the existancelock and uses hid_device only if it exists.
>>     
>
> The code duplication isn't really particularly nice, but I don't see any 
> way around it that wouldn't complicate things even more.
>
> I will be applying the patches, thanks.
>   
Thanks!
> (BTW your patches didn't reach mail mailbox, I had to dig them out from 
> mailinglist -- did you receive bounces by any chance?).
>   
Yes, they did bounce.
Thanks,
Val.
> Thanks,
>
>   


^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-12-07 15:11 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-12-06 14:45 [PATCH 0/2] USB: HID: Fix race between disconnect and hiddev_ioctl Valentine Barshak
2010-12-06 14:51 ` [PATCH 1/2] " Valentine Barshak
2010-12-07 14:47   ` Jiri Kosina
2010-12-07 15:02     ` Valentine Barshak
2010-12-06 15:16 ` [PATCH 2/2] USB: HID: Consolidate device existence checks in hiddev_ioctl Valentine Barshak
2010-12-06 15:16   ` Valentine Barshak

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.