Bug#631287: BUG during access to hiddev (APC UPS) (Bug#631287: Current status & new info)
found 631287 linux-2.6/2.6.32-35
quit
Hi Joachim,
Joachim Oehler wrote:
> We're also currently encountering this particular bug, find dmesg output
> below.
> The kernel currently installed is:
>
> # uname -a
> Linux server 2.6.32-5-686 #1 SMP Mon Jun 13 04:13:06 UTC 2011 i686
> GNU/Linux
[...]
> [15109891.033138] generic-usb 0003:051D:0002.0004: hiddev0,hidraw0: USB HID v1.00 Device [American Power Conversion Back-UPS 500 FW: 6.4.I USB FW: c1 ] on usb-0000:00:1d.1-2/input0
> [15110038.520079] usb 3-2: USB disconnect, address 28
> [15110038.567356] BUG: unable to handle kernel NULL pointer dereference at (null)
> [15110038.567501] IP: [<d1620af7>] hiddev_ioctl+0x26/0x56f [usbhid]
Very nice.
[...]
> I already prepared the installation of 3.2.x from squeeze-backports but I
> have still to boot that kernel:
Thanks! I look forward to hearing the result.
I'm attaching patches for 2.6.32.y that carry out the fix Ben
suggested before (plus ba18311dff79). See [1] if you would like to
test them.
Hope that helps,
Jonathan
[1] http://kernel-handbook.alioth.debian.org/ch-common-tasks.html#s4.2.2
or the corresponding page in the debian-kernel-handbook package
From: Chris Ball <cjb@laptop.org>
Date: Thu, 12 Aug 2010 19:07:40 -0400
Subject: HID: hiddev: protect against disconnect/NULL-dereference race
commit 7032269e87ade34cc12891675371fa2ac150a620 upstream
One of our users reports consistently hitting a NULL dereference that
resolves to the "hid_to_usb_dev(hid);" call in hiddev_ioctl(), when
disconnecting a Lego WeDo USB HID device from an OLPC XO running
Scratch software. There's a FIXME comment and a guard against the
dereference, but that happens farther down the function than the
initial dereference does.
This patch moves the call to be below the guard, and the user reports
that it fixes the problem for him. OLPC bug report:
http://dev.laptop.org/ticket/10174
Signed-off-by: Chris Ball <cjb@laptop.org>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
drivers/hid/usbhid/hiddev.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 8b6ee247bfe4..cb3f35594bfa 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -594,7 +594,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 usb_device *dev = hid_to_usb_dev(hid);
+ struct usb_device *dev;
struct hiddev_collection_info cinfo;
struct hiddev_report_info rinfo;
struct hiddev_field_info finfo;
@@ -608,9 +608,11 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
/* Called without BKL by compat methods so no BKL taken */
/* FIXME: Who or what stop this racing with a disconnect ?? */
- if (!hiddev->exist)
+ if (!hiddev->exist || !hid)
return -EIO;
+ dev = hid_to_usb_dev(hid);
+
switch (cmd) {
case HIDIOCGVERSION:
--
1.7.9
From: Valentine Barshak <vbarshak@mvista.com>
Date: Mon, 6 Dec 2010 17:51:41 +0300
Subject: HID: Fix race between disconnect and hiddev_ioctl
commit 1a8e8fab790ea7af81b8f964fdec706ad1ec2271 upstream
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>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.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 cb3f35594bfa..b26ebcb23e8c 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -593,7 +593,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;
@@ -601,26 +601,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 ==
@@ -628,11 +635,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;
@@ -641,6 +659,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;
@@ -674,6 +694,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;
@@ -683,6 +704,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);
@@ -695,14 +717,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;
@@ -714,14 +743,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;
@@ -730,10 +766,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;
@@ -745,11 +792,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));
@@ -767,6 +826,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;
@@ -792,12 +852,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;
@@ -810,24 +880,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.7.9
From: Valentine Barshak <vbarshak@mvista.com>
Date: Mon, 6 Dec 2010 18:16:11 +0300
Subject: HID: Consolidate device existence checks in hiddev_ioctl
commit 33d6eb570b1f3fe5ba93cef465c5be66535c2c9a upstream
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>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.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 b26ebcb23e8c..f4e9200b02fc 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -594,221 +594,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;
+ 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_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));
@@ -826,12 +772,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 */
@@ -840,57 +784,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;
@@ -898,22 +821,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;
@@ -921,10 +837,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.7.9
From: Jiri Kosina <jkosina@suse.cz>
Date: Fri, 20 May 2011 10:50:13 +0200
Subject: HID: hiddev: fix race between hiddev_disconnect and hiddev_release
commit 6cb4b040795c555c7ab4b1ba29b0dba2b5a42beb upstream
When hiddev_disconnect() runs with chardev open, it will proceed with
usbhid_close(). When userspace in parallel runs the hiddev_release(),
it sees !hiddev->exists (as it has been already set so by
hiddev_disconnect()) and kfrees hiddev while hiddev_disconnect() hasn't
finished yet.
Serialize the access to hiddev->exists and hiddev->open by existancelock.
Reported-by: mike-@cinci.rr.com
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
drivers/hid/usbhid/hiddev.c | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index f4e9200b02fc..d89368a7533c 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -245,6 +245,7 @@ static int hiddev_release(struct inode * inode, struct file * file)
list_del(&list->node);
spin_unlock_irqrestore(&list->hiddev->list_lock, flags);
+ mutex_lock(&list->hiddev->existancelock);
if (!--list->hiddev->open) {
if (list->hiddev->exist) {
usbhid_close(list->hiddev->hid);
@@ -255,6 +256,7 @@ static int hiddev_release(struct inode * inode, struct file * file)
}
kfree(list);
+ mutex_unlock(&list->hiddev->existancelock);
return 0;
}
@@ -302,18 +304,22 @@ static int hiddev_open(struct inode *inode, struct file *file)
list_add_tail(&list->node, &hiddev_table[i]->list);
spin_unlock_irq(&list->hiddev->list_lock);
+ mutex_lock(&list->hiddev->existancelock);
if (!list->hiddev->open++)
if (list->hiddev->exist) {
struct hid_device *hid = hiddev_table[i]->hid;
res = usbhid_get_power(hid);
if (res < 0) {
res = -EIO;
- goto bail;
+ goto bail_unlock;
}
usbhid_open(hid);
}
+ mutex_unlock(&list->hiddev->existancelock);
return 0;
+bail_unlock:
+ mutex_unlock(&list->hiddev->existancelock);
bail:
file->private_data = NULL;
kfree(list);
@@ -942,7 +948,6 @@ void hiddev_disconnect(struct hid_device *hid)
mutex_lock(&hiddev->existancelock);
hiddev->exist = 0;
- mutex_unlock(&hiddev->existancelock);
hiddev_table[hiddev->hid->minor - HIDDEV_MINOR_BASE] = NULL;
usb_deregister_dev(usbhid->intf, &hiddev_class);
@@ -953,6 +958,7 @@ void hiddev_disconnect(struct hid_device *hid)
} else {
kfree(hiddev);
}
+ mutex_unlock(&hiddev->existancelock);
}
/* Currently this driver is a USB driver. It's not a conventional one in
--
1.7.9
From: Jiri Kosina <jkosina@suse.cz>
Date: Tue, 24 May 2011 11:43:18 +0200
Subject: HID: hiddev: fix potential use-after-free
commit 7f77897ef2b6a5ee4eb8bc24fe8b1f3eab254328 upstream
Commit 6cb4b040795 ("HID: hiddev: fix race between hiddev_disconnect
and hiddev_release") made it possible to access hiddev (for unlocking
the existance mutex) once hiddev has been kfreed.
Change the order so that this can not happen (always unlock the mutex first,
it is needed only to protect access to ->exist and ->open).
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
drivers/hid/usbhid/hiddev.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index d89368a7533c..c6efd3c20145 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -953,12 +953,13 @@ void hiddev_disconnect(struct hid_device *hid)
usb_deregister_dev(usbhid->intf, &hiddev_class);
if (hiddev->open) {
+ mutex_unlock(&hiddev->existancelock);
usbhid_close(hiddev->hid);
wake_up_interruptible(&hiddev->wait);
} else {
+ mutex_unlock(&hiddev->existancelock);
kfree(hiddev);
}
- mutex_unlock(&hiddev->existancelock);
}
/* Currently this driver is a USB driver. It's not a conventional one in
--
1.7.9
From: Dan Carpenter <error27@gmail.com>
Date: Thu, 26 May 2011 11:49:16 +0300
Subject: HID: hiddev: fix use after free in hiddev_release
commit 5c699d7d3f94ee1dd934edea889b32f8279a4e65 upstream
There are a couple use after free bugs here.
Signed-off-by: Dan Carpenter <error27@gmail.com>
[jkosina@suse.cz: removed already fixed hunk]
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
drivers/hid/usbhid/hiddev.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index c6efd3c20145..b4b87e831f14 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -251,12 +251,15 @@ static int hiddev_release(struct inode * inode, struct file * file)
usbhid_close(list->hiddev->hid);
usbhid_put_power(list->hiddev->hid);
} else {
+ mutex_unlock(&list->hiddev->existancelock);
kfree(list->hiddev);
+ kfree(list);
+ return 0;
}
}
- kfree(list);
mutex_unlock(&list->hiddev->existancelock);
+ kfree(list);
return 0;
}
--
1.7.9
From: Ming Lei <tom.leiming@gmail.com>
Date: Thu, 12 Jan 2012 17:42:22 +0800
Subject: HID: usbhid: fix dead lock between open and disconect
commit ba18311dff7933ccb9c41bbbb1ad3d70840069b5 upstream.
There is no reason to hold hiddev->existancelock before
calling usb_deregister_dev, so move it out of the lock.
The patch fixes the lockdep warning below.
[ 5733.386271] ======================================================
[ 5733.386274] [ INFO: possible circular locking dependency detected ]
[ 5733.386278] 3.2.0-custom-next-20120111+ #1 Not tainted
[ 5733.386281] -------------------------------------------------------
[ 5733.386284] khubd/186 is trying to acquire lock:
[ 5733.386288] (minor_rwsem){++++.+}, at: [<ffffffffa0011a04>] usb_deregister_dev+0x37/0x9e [usbcore]
[ 5733.386311]
[ 5733.386312] but task is already holding lock:
[ 5733.386315] (&hiddev->existancelock){+.+...}, at: [<ffffffffa0094d17>] hiddev_disconnect+0x26/0x87 [usbhid]
[ 5733.386328]
[ 5733.386329] which lock already depends on the new lock.
[ 5733.386330]
[ 5733.386333]
[ 5733.386334] the existing dependency chain (in reverse order) is:
[ 5733.386336]
[ 5733.386337] -> #1 (&hiddev->existancelock){+.+...}:
[ 5733.386346] [<ffffffff81082d26>] lock_acquire+0xcb/0x10e
[ 5733.386357] [<ffffffff813df961>] __mutex_lock_common+0x60/0x465
[ 5733.386366] [<ffffffff813dfe4d>] mutex_lock_nested+0x36/0x3b
[ 5733.386371] [<ffffffffa0094ad6>] hiddev_open+0x113/0x193 [usbhid]
[ 5733.386378] [<ffffffffa0011971>] usb_open+0x66/0xc2 [usbcore]
[ 5733.386390] [<ffffffff8111a8b5>] chrdev_open+0x12b/0x154
[ 5733.386402] [<ffffffff811159a8>] __dentry_open.isra.16+0x20b/0x355
[ 5733.386408] [<ffffffff811165dc>] nameidata_to_filp+0x43/0x4a
[ 5733.386413] [<ffffffff81122ed5>] do_last+0x536/0x570
[ 5733.386419] [<ffffffff8112300b>] path_openat+0xce/0x301
[ 5733.386423] [<ffffffff81123327>] do_filp_open+0x33/0x81
[ 5733.386427] [<ffffffff8111664d>] do_sys_open+0x6a/0xfc
[ 5733.386431] [<ffffffff811166fb>] sys_open+0x1c/0x1e
[ 5733.386434] [<ffffffff813e7c79>] system_call_fastpath+0x16/0x1b
[ 5733.386441]
[ 5733.386441] -> #0 (minor_rwsem){++++.+}:
[ 5733.386448] [<ffffffff8108255d>] __lock_acquire+0xa80/0xd74
[ 5733.386454] [<ffffffff81082d26>] lock_acquire+0xcb/0x10e
[ 5733.386458] [<ffffffff813e01f5>] down_write+0x44/0x77
[ 5733.386464] [<ffffffffa0011a04>] usb_deregister_dev+0x37/0x9e [usbcore]
[ 5733.386475] [<ffffffffa0094d2d>] hiddev_disconnect+0x3c/0x87 [usbhid]
[ 5733.386483] [<ffffffff8132df51>] hid_disconnect+0x3f/0x54
[ 5733.386491] [<ffffffff8132dfb4>] hid_device_remove+0x4e/0x7a
[ 5733.386496] [<ffffffff812c0957>] __device_release_driver+0x81/0xcd
[ 5733.386502] [<ffffffff812c09c3>] device_release_driver+0x20/0x2d
[ 5733.386507] [<ffffffff812c0564>] bus_remove_device+0x114/0x128
[ 5733.386512] [<ffffffff812bdd6f>] device_del+0x131/0x183
[ 5733.386519] [<ffffffff8132def3>] hid_destroy_device+0x1e/0x3d
[ 5733.386525] [<ffffffffa00916b0>] usbhid_disconnect+0x36/0x42 [usbhid]
[ 5733.386530] [<ffffffffa000fb60>] usb_unbind_interface+0x57/0x11f [usbcore]
[ 5733.386542] [<ffffffff812c0957>] __device_release_driver+0x81/0xcd
[ 5733.386547] [<ffffffff812c09c3>] device_release_driver+0x20/0x2d
[ 5733.386552] [<ffffffff812c0564>] bus_remove_device+0x114/0x128
[ 5733.386557] [<ffffffff812bdd6f>] device_del+0x131/0x183
[ 5733.386562] [<ffffffffa000de61>] usb_disable_device+0xa8/0x1d8 [usbcore]
[ 5733.386573] [<ffffffffa0006bd2>] usb_disconnect+0xab/0x11f [usbcore]
[ 5733.386583] [<ffffffffa0008aa0>] hub_thread+0x73b/0x1157 [usbcore]
[ 5733.386593] [<ffffffff8105dc0f>] kthread+0x95/0x9d
[ 5733.386601] [<ffffffff813e90b4>] kernel_thread_helper+0x4/0x10
[ 5733.386607]
[ 5733.386608] other info that might help us debug this:
[ 5733.386609]
[ 5733.386612] Possible unsafe locking scenario:
[ 5733.386613]
[ 5733.386615] CPU0 CPU1
[ 5733.386618] ---- ----
[ 5733.386620] lock(&hiddev->existancelock);
[ 5733.386625] lock(minor_rwsem);
[ 5733.386630] lock(&hiddev->existancelock);
[ 5733.386635] lock(minor_rwsem);
[ 5733.386639]
[ 5733.386640] *** DEADLOCK ***
[ 5733.386641]
[ 5733.386644] 6 locks held by khubd/186:
[ 5733.386646] #0: (&__lockdep_no_validate__){......}, at: [<ffffffffa00084af>] hub_thread+0x14a/0x1157 [usbcore]
[ 5733.386661] #1: (&__lockdep_no_validate__){......}, at: [<ffffffffa0006b77>] usb_disconnect+0x50/0x11f [usbcore]
[ 5733.386677] #2: (hcd->bandwidth_mutex){+.+.+.}, at: [<ffffffffa0006bc8>] usb_disconnect+0xa1/0x11f [usbcore]
[ 5733.386693] #3: (&__lockdep_no_validate__){......}, at: [<ffffffff812c09bb>] device_release_driver+0x18/0x2d
[ 5733.386704] #4: (&__lockdep_no_validate__){......}, at: [<ffffffff812c09bb>] device_release_driver+0x18/0x2d
[ 5733.386714] #5: (&hiddev->existancelock){+.+...}, at: [<ffffffffa0094d17>] hiddev_disconnect+0x26/0x87 [usbhid]
[ 5733.386727]
[ 5733.386727] stack backtrace:
[ 5733.386731] Pid: 186, comm: khubd Not tainted 3.2.0-custom-next-20120111+ #1
[ 5733.386734] Call Trace:
[ 5733.386741] [<ffffffff81062881>] ? up+0x34/0x3b
[ 5733.386747] [<ffffffff813d9ef3>] print_circular_bug+0x1f8/0x209
[ 5733.386752] [<ffffffff8108255d>] __lock_acquire+0xa80/0xd74
[ 5733.386756] [<ffffffff810808b4>] ? trace_hardirqs_on_caller+0x15d/0x1a3
[ 5733.386763] [<ffffffff81043a3f>] ? vprintk+0x3f4/0x419
[ 5733.386774] [<ffffffffa0011a04>] ? usb_deregister_dev+0x37/0x9e [usbcore]
[ 5733.386779] [<ffffffff81082d26>] lock_acquire+0xcb/0x10e
[ 5733.386789] [<ffffffffa0011a04>] ? usb_deregister_dev+0x37/0x9e [usbcore]
[ 5733.386797] [<ffffffff813e01f5>] down_write+0x44/0x77
[ 5733.386807] [<ffffffffa0011a04>] ? usb_deregister_dev+0x37/0x9e [usbcore]
[ 5733.386818] [<ffffffffa0011a04>] usb_deregister_dev+0x37/0x9e [usbcore]
[ 5733.386825] [<ffffffffa0094d2d>] hiddev_disconnect+0x3c/0x87 [usbhid]
[ 5733.386830] [<ffffffff8132df51>] hid_disconnect+0x3f/0x54
[ 5733.386834] [<ffffffff8132dfb4>] hid_device_remove+0x4e/0x7a
[ 5733.386839] [<ffffffff812c0957>] __device_release_driver+0x81/0xcd
[ 5733.386844] [<ffffffff812c09c3>] device_release_driver+0x20/0x2d
[ 5733.386848] [<ffffffff812c0564>] bus_remove_device+0x114/0x128
[ 5733.386854] [<ffffffff812bdd6f>] device_del+0x131/0x183
[ 5733.386859] [<ffffffff8132def3>] hid_destroy_device+0x1e/0x3d
[ 5733.386865] [<ffffffffa00916b0>] usbhid_disconnect+0x36/0x42 [usbhid]
[ 5733.386876] [<ffffffffa000fb60>] usb_unbind_interface+0x57/0x11f [usbcore]
[ 5733.386882] [<ffffffff812c0957>] __device_release_driver+0x81/0xcd
[ 5733.386886] [<ffffffff812c09c3>] device_release_driver+0x20/0x2d
[ 5733.386890] [<ffffffff812c0564>] bus_remove_device+0x114/0x128
[ 5733.386895] [<ffffffff812bdd6f>] device_del+0x131/0x183
[ 5733.386905] [<ffffffffa000de61>] usb_disable_device+0xa8/0x1d8 [usbcore]
[ 5733.386916] [<ffffffffa0006bd2>] usb_disconnect+0xab/0x11f [usbcore]
[ 5733.386921] [<ffffffff813dff82>] ? __mutex_unlock_slowpath+0x130/0x141
[ 5733.386929] [<ffffffffa0008aa0>] hub_thread+0x73b/0x1157 [usbcore]
[ 5733.386935] [<ffffffff8106a51d>] ? finish_task_switch+0x78/0x150
[ 5733.386941] [<ffffffff8105e396>] ? __init_waitqueue_head+0x4c/0x4c
[ 5733.386950] [<ffffffffa0008365>] ? usb_remote_wakeup+0x56/0x56 [usbcore]
[ 5733.386955] [<ffffffff8105dc0f>] kthread+0x95/0x9d
[ 5733.386961] [<ffffffff813e90b4>] kernel_thread_helper+0x4/0x10
[ 5733.386966] [<ffffffff813e24b8>] ? retint_restore_args+0x13/0x13
[ 5733.386970] [<ffffffff8105db7a>] ? __init_kthread_worker+0x55/0x55
[ 5733.386974] [<ffffffff813e90b0>] ? gs_change+0x13/0x13
Signed-off-by: Ming Lei <ming.lei@canonical.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
drivers/hid/usbhid/hiddev.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index b4b87e831f14..0ac56996731d 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -949,11 +949,12 @@ void hiddev_disconnect(struct hid_device *hid)
struct hiddev *hiddev = hid->hiddev;
struct usbhid_device *usbhid = hid->driver_data;
+ usb_deregister_dev(usbhid->intf, &hiddev_class);
+
mutex_lock(&hiddev->existancelock);
hiddev->exist = 0;
hiddev_table[hiddev->hid->minor - HIDDEV_MINOR_BASE] = NULL;
- usb_deregister_dev(usbhid->intf, &hiddev_class);
if (hiddev->open) {
mutex_unlock(&hiddev->existancelock);
--
1.7.9
Reply to: