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.
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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.
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,
>
>