On Fri, 9 Jul 2004 15:50:24 -0400
Adam Kropelin <[EMAIL PROTECTED]> wrote:
> Ewwww. Somebody botched that up but good.
>
> Here is a patch to fix up hiddev_ioctl(), slightly improved over the way
> it was done for 2.6. This patch avoids allocating a uref_multi when a
> standard uref will do. It also fixes copy_{to,from}_user() return value
> handling as was done in 2.6.
I don't know if allocating uref is an improvement. I'd like to follow
2.6 rather than inventing stuff.
The patch is also buggy in the following place:
> +++ linux-2.4.26/drivers/usb/hiddev.c Fri Jul 9 15:06:01 2004
.........
> + goto goodreturn;
> case HIDIOCSUSAGES:
> - for (i = 0; i < uref_multi.num_values; i++)
> + for (i = 0; i < uref_multi->num_values; i++)
> field->value[uref->usage_index + i] =
> - uref_multi.values[i];
> - return 0;
> + uref_multi->values[i];
> + goto goodreturn;
> }
> break;
>
> +goodreturn:
> + kfree(uref_multi);
> + return 0;
If the command is unknown and the switch falls through, the uref_multi
is not freed.
I fixed it up, and I need someone to go over the patch AGAIN and
review the thing, then mail me a positive result. Obviously, the maze of
gotos is not conductive to a bug-free code.
The 2.6 version is in AKPM's tree right now. I'll get back to this
when Linus accepts it and a revision of a three-numbered 2.6.x turns
around with users.
-- Pete
diff -urp -X dontdiff linux-2.4.28-pre3/drivers/usb/hiddev.c
linux-2.4.28-pre3-usb/drivers/usb/hiddev.c
--- linux-2.4.28-pre3/drivers/usb/hiddev.c 2004-08-24 12:38:51.000000000 -0700
+++ linux-2.4.28-pre3-usb/drivers/usb/hiddev.c 2004-09-12 22:31:48.000000000 -0700
@@ -396,8 +396,8 @@ static int hiddev_ioctl(struct inode *in
struct hiddev_collection_info cinfo;
struct hiddev_report_info rinfo;
struct hiddev_field_info finfo;
- struct hiddev_usage_ref_multi uref_multi;
- struct hiddev_usage_ref *uref = &uref_multi.uref;
+ struct hiddev_usage_ref_multi *uref_multi;
+ struct hiddev_usage_ref *uref;
struct hiddev_devinfo dinfo;
struct hid_report *report;
struct hid_field *field;
@@ -561,25 +561,31 @@ static int hiddev_ioctl(struct inode *in
return 0;
case HIDIOCGUCODE:
+ uref_multi = kmalloc(sizeof(*uref_multi), GFP_KERNEL);
+ if (!uref_multi)
+ return -ENOMEM;
+ uref = &uref_multi->uref;
if (copy_from_user(uref, (void *) arg, sizeof(*uref)))
- return -EFAULT;
+ goto fault;
rinfo.report_type = uref->report_type;
rinfo.report_id = uref->report_id;
if ((report = hiddev_lookup_report(hid, &rinfo)) == NULL)
- return -EINVAL;
+ goto inval;
if (uref->field_index >= report->maxfield)
- return -EINVAL;
+ goto inval;
field = report->field[uref->field_index];
if (uref->usage_index >= field->maxusage)
- return -EINVAL;
+ goto inval;
uref->usage_code = field->usage[uref->usage_index].hid;
if (copy_to_user((void *) arg, uref, sizeof(*uref)))
- return -EFAULT;
+ goto fault;
+
+ kfree(uref_multi);
return 0;
case HIDIOCGUSAGE:
@@ -587,71 +593,86 @@ static int hiddev_ioctl(struct inode *in
case HIDIOCGUSAGES:
case HIDIOCSUSAGES:
case HIDIOCGCOLLECTIONINDEX:
+ uref_multi = kmalloc(sizeof(*uref_multi), GFP_KERNEL);
+ if (!uref_multi)
+ return -ENOMEM;
+ uref = &uref_multi->uref;
if (cmd == HIDIOCGUSAGES || cmd == HIDIOCSUSAGES) {
- if (copy_from_user(&uref_multi, (void *) arg,
- sizeof(uref_multi)))
- return -EFAULT;
+ if (copy_from_user(uref_multi, (void *) arg,
+ sizeof(*uref_multi)))
+ goto fault;
} else {
if (copy_from_user(uref, (void *) arg, sizeof(*uref)))
- return -EFAULT;
+ goto fault;
}
if (cmd != HIDIOCGUSAGE &&
cmd != HIDIOCGUSAGES &&
uref->report_type == HID_REPORT_TYPE_INPUT)
- return -EINVAL;
+ goto inval;
if (uref->report_id == HID_REPORT_ID_UNKNOWN) {
field = hiddev_lookup_usage(hid, uref);
if (field == NULL)
- return -EINVAL;
+ goto inval;
} else {
rinfo.report_type = uref->report_type;
rinfo.report_id = uref->report_id;
if ((report = hiddev_lookup_report(hid, &rinfo)) == NULL)
- return -EINVAL;
+ goto inval;
if (uref->field_index >= report->maxfield)
- return -EINVAL;
+ goto inval;
field = report->field[uref->field_index];
if (uref->usage_index >= field->maxusage)
- return -EINVAL;
+ goto inval;
if (cmd == HIDIOCGUSAGES || cmd == HIDIOCSUSAGES) {
- if (uref_multi.num_values >= HID_MAX_USAGES ||
+ if (uref_multi->num_values >= HID_MAX_USAGES ||
uref->usage_index >= field->maxusage ||
- (uref->usage_index + uref_multi.num_values) >=
field->maxusage)
- return -EINVAL;
+ (uref->usage_index + uref_multi->num_values) >=
field->maxusage)
+ goto inval;
}
}
switch (cmd) {
case HIDIOCGUSAGE:
uref->value = field->value[uref->usage_index];
- return copy_to_user((void *) arg, uref, sizeof(*uref));
-
+ if (copy_to_user((void *) arg, uref, sizeof(*uref)))
+ goto fault;
+ break;
case HIDIOCSUSAGE:
field->value[uref->usage_index] = uref->value;
- return 0;
-
+ break;
case HIDIOCGCOLLECTIONINDEX:
+ kfree(uref_multi);
return field->usage[uref->usage_index].collection_index;
case HIDIOCGUSAGES:
- for (i = 0; i < uref_multi.num_values; i++)
- uref_multi.values[i] =
+ for (i = 0; i < uref_multi->num_values; i++)
+ uref_multi->values[i] =
field->value[uref->usage_index + i];
- if (copy_to_user((void *) arg, &uref_multi,
- sizeof(uref_multi)))
- return -EFAULT;
- return 0;
+ if (copy_to_user((void *) arg, uref_multi,
+ sizeof(*uref_multi)))
+ goto fault;
+ break;
case HIDIOCSUSAGES:
- for (i = 0; i < uref_multi.num_values; i++)
+ for (i = 0; i < uref_multi->num_values; i++)
field->value[uref->usage_index + i] =
- uref_multi.values[i];
- return 0;
+ uref_multi->values[i];
+ break;
+ default:
+ kfree(uref_multi);
+ return -EINVAL;
}
- break;
+ kfree(uref_multi);
+ return 0;
+fault:
+ kfree(uref_multi);
+ return -EFAULT;
+inval:
+ kfree(uref_multi);
+ return -EINVAL;
case HIDIOCGCOLLECTIONINFO:
if (copy_from_user(&cinfo, (void *) arg, sizeof(cinfo)))
diff -urp -X dontdiff linux-2.4.28-pre3/include/linux/hiddev.h
linux-2.4.28-pre3-usb/include/linux/hiddev.h
--- linux-2.4.28-pre3/include/linux/hiddev.h 2004-09-12 17:07:47.000000000 -0700
+++ linux-2.4.28-pre3-usb/include/linux/hiddev.h 2004-09-12 20:53:43.000000000
-0700
@@ -128,10 +128,11 @@ struct hiddev_usage_ref {
/* hiddev_usage_ref_multi is used for sending multiple bytes to a control.
* It really manifests itself as setting the value of consecutive usages */
+#define HID_MAX_MULTI_USAGES 1024
struct hiddev_usage_ref_multi {
struct hiddev_usage_ref uref;
__u32 num_values;
- __s32 values[HID_MAX_USAGES];
+ __s32 values[HID_MAX_MULTI_USAGES];
};
/* FIELD_INDEX_NONE is returned in read() data from the kernel when flags
@@ -213,6 +214,11 @@ struct hiddev_usage_ref_multi {
*/
#ifdef CONFIG_USB_HIDDEV
+struct hid_device;
+struct hid_usage;
+struct hid_field;
+struct hid_report;
+
int hiddev_connect(struct hid_device *);
void hiddev_disconnect(struct hid_device *);
void hiddev_hid_event(struct hid_device *hid, struct hid_field *field,
-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM.
Deadline: Sept. 13. Go here: http://sf.net/ppc_contest.php
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
