Bug#631287: BUG during access to hiddev (APC UPS)
Hi Stefan et al,
I'd like to ask a favor. The following kernel bug is stalled for lack
of access to a USB APC UPS. Do you know of anyone who could reproduce
it and confirm the fix?
Dmitry Eremin-Solenikov wrote[1]:
> After upgrading to squeeze I've started receiving the following BUG in dmesg
> when apcupsd tries to access hid device for my UPS (APC SmartUPS 3000,
> connected via USB).
[...]
> usb 2-2: USB disconnect, address 4
> [...]/drivers/hid/usbhid/hid-core.c: usb_submit_urb(ctrl) failed
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP: [<f843db27>] hiddev_ioctl+0x26/0x56f [usbhid]
And later:
> Unfortunately I cannot try reproducing this error, as I don't have access
> to this hardware anymore. The problem was easily reproducible though
> by using apcupsd together with any USB APC ups.
If someone has a USB APC UPS, we would like to learn the following
from her:
1. confirm that this bug affects your system, using the kernel from
squeeze
2. confirm that it is fixed using the kernel from sid. (The only
packages from outside squeeze needed for this test aside from the
kernel image itself are linux-base and initramfs-tools.)
3. test the relevant patches (attached) on top of a 2.6.32.y kernel,
as described at [2].
Any subset would be useful already. Thanks much for keeping the
userspace support well maintained.
Looking forward to your thoughts,
Jonathan
[1] http://bugs.debian.org/631287
[2] http://kernel-handbook.alioth.debian.org/ch-common-tasks.html
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.8.rc3
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.8.rc3
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.8.rc3
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.8.rc3
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.8.rc3
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.8.rc3
Reply to: