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
On Fri, Dec 03, 2010 at 08:27:46PM +0300, 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.
Why didn't you take the lock and check hiddev->exist at the beginning of
ioctl handler instead of pushing it down into individual command
handlers? I guess it would slow down HIDIOCGVERSION but I think we could
pay this price for code that is more clear ;)
--
Dmitry
Dmitry Torokhov wrote:
> On Fri, Dec 03, 2010 at 08:27:46PM +0300, 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.
>>
>
> Why didn't you take the lock and check hiddev->exist at the beginning of
> ioctl handler instead of pushing it down into individual command
> handlers? I guess it would slow down HIDIOCGVERSION but I think we could
> pay this price for code that is more clear ;)
>
>
Well, some of the commands were already using the lock, while a couple
of them doesn't seem to need it.
I've just added locking to the other commands that needed it. I guess I
didn't want to rework the whole stuff
in order not to forget to unlock and return.
But I agree, the code would look a bit cleaner though if did as you say.
Thanks,
Val.
Am Samstag, 4. Dezember 2010, 00:16:12 schrieb Dmitry Torokhov:
> > 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.
>
> Why didn't you take the lock and check hiddev->exist at the beginning of
> ioctl handler instead of pushing it down into individual command
> handlers? I guess it would slow down HIDIOCGVERSION but I think we could
> pay this price for code that is more clear ;)
Strictly speaking you'd change the semantics. Right now you can execute
the ioctl even if you know you are holding an fd to a disconnected device
open.
Regards
Oliver
On Sat, Dec 04, 2010 at 09:22:23PM +0100, Oliver Neukum wrote:
> Am Samstag, 4. Dezember 2010, 00:16:12 schrieb Dmitry Torokhov:
> > > 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.
> >
> > Why didn't you take the lock and check hiddev->exist at the beginning of
> > ioctl handler instead of pushing it down into individual command
> > handlers? I guess it would slow down HIDIOCGVERSION but I think we could
> > pay this price for code that is more clear ;)
>
> Strictly speaking you'd change the semantics. Right now you can execute
> the ioctl even if you know you are holding an fd to a disconnected device
> open.
No, I do not think I would. I do not believe that the availability for
HIDIOCGVERSION on disconnected device is spelled out in API/ABI spec. We
only know that ioctl will either succeed or appropriate error code is
returned. The fact that right now HIDIOCGVERSION is available on
disconnected devices is just an implementation detail subject to change.
--
Dmitry
Dmitry Torokhov wrote:
> On Sat, Dec 04, 2010 at 09:22:23PM +0100, Oliver Neukum wrote:
>
>> Am Samstag, 4. Dezember 2010, 00:16:12 schrieb Dmitry Torokhov:
>>
>>>> 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.
>>>>
>>> Why didn't you take the lock and check hiddev->exist at the beginning of
>>> ioctl handler instead of pushing it down into individual command
>>> handlers? I guess it would slow down HIDIOCGVERSION but I think we could
>>> pay this price for code that is more clear ;)
>>>
>> Strictly speaking you'd change the semantics. Right now you can execute
>> the ioctl even if you know you are holding an fd to a disconnected device
>> open.
>>
>
> No, I do not think I would. I do not believe that the availability for
> HIDIOCGVERSION on disconnected device is spelled out in API/ABI spec. We
> only know that ioctl will either succeed or appropriate error code is
> returned. The fact that right now HIDIOCGVERSION is available on
> disconnected devices is just an implementation detail subject to change.
>
>
It's not just HIDIOCGVERSION. A couple of other commands
(HIDIOCGFLAG/HIDIOCSFLAG) didn't check device existence in the first
place either.
Current implementation depends on when the device is actually removed.
If it has been removed before the hiddev_ioctl(), hiddev_ioctl() returns
-EIO.
If the device is removed while hiddev_ioctl() is in progress, we either
do not notice that and handle HIDIOCGVERSION and HIDIOCGFLAG/HIDIOCSFLAG
just fine, or return -ENODEV.
I'll submit a patch in a bit that applies on top of the "[PATCH] USB:
USBHID: Fix race between disconnect and hiddev_ioctl" and makes the
hiddev_ioctl() check
device existence before processing the command and always return -ENODEV
in case the device has been removed.
Thanks,
Val.
Started new thread: [PATCH 0/2] USB: HID: Fix race between disconnect
and hiddev_ioctl
Thanks,
Val.