| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 13 Aug 2013 20:37:19 +0200
From: Alexander Holler <holler@...oftware.de>
To: Benjamin Tissoires <benjamin.tissoires@...hat.com>
CC: Benjamin Tissoires <benjamin.tissoires@...il.com>,
Henrik Rydberg <rydberg@...omail.se>,
Jiri Kosina <jkosina@...e.cz>,
Stephane Chatty <chatty@...c.fr>,
Mika Westerberg <mika.westerberg@...ux.intel.com>,
linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
Subject: Re: [PATCH 1/3] HID: Use existing parser for pre-scanning the report
descriptors
Am 13.08.2013 16:58, schrieb Benjamin Tissoires:
> hid_scan_report() implements its own HID report descriptor parsing. It was
> fine until we added the SENSOR_HUB detection. It is going to be even worse
> with the detection of Win 8 certified touchscreen, as this detection
> relies on a special feature and on the report_size and report_count fields.
Sorry, but I can't agree with your wording here.
If you look at the if expression I've added to support HID sensor hubs,
you will notice that the first expression was a test for the usage page
of sensor hubs. That wasn't the first test by accident, instead I've
choosen the order of those 4 tests very carefully to make the impact on
the existing parsing of other HID devices very small. So it might not
have be pleasing to your eyes, but it was for sure an appropriate solution.
Anyway, I've tested your patch 1/3 on top of 3.11-rc5 here and the
detection of sensor hubs doesn't work anymore with your patch applied.
Up to now I only had a quick look at it, but it looks like the test in
hid_scan_open_collection() isn't hit for my device.
Another problem is that I don't have any commercial sensor hub and I'm
therefor not a very relvant as tester (I've implemented the firmware for
my HID (sensor hub) device too). Therefor I've added Srinivas Pandruvada
to cc, because he's the only one I know who has HID sensor hubs.
And, as said, I've implemented the other side here too, therefor I've
added the descriptor I'm using below.
Regards,
Alexander Holler
--my-descriptor--
0x05, 0x20, // HID_USAGE_PAGE_SENSOR
0xa1, 0x01, // COLLECTION (Application)
0x09, 0xa0, // HID_USAGE_SENSOR_CATEGORY_TIME
0xa1, 0x00, // COLLECTION (Physical)
#ifndef USE_FULL_YEAR
0x75, 0x08, // REPORT_SIZE (8 bits)
#endif
0x95, 0x01, // REPORT_COUNT (1)
0x85, HID_REPORT_ID_TIME, // REPORT_ID
0x0a, 0x21, 0x05, // USAGE (Year)
#ifdef USE_FULL_YEAR
0x75, 0x10, // REPORT_SIZE (16 bits)
#else
0x15, 0x00, // LOGICAL_MINIMUM (0) (Range is currently not specified
in HUTRR39)
0x25, 0x63, // LOGICAL_MAXIMUM (99)
#endif
0x65, 0x00, // UNIT (None)
0x81, 0x02, // INPUT (Data,Var,Abs)
0x0a, 0x22, 0x05, // USAGE (Month)
#ifdef USE_FULL_YEAR
0x75, 0x08, // REPORT_SIZE (8 bits)
#endif
0x15, 0x01, // LOGICAL_MINIMUM (1)
0x25, 0x0c, // LOGICAL_MAXIMUM (12)
0x65, 0x00, // UNIT (None)
0x81, 0x02, // INPUT (Data,Var,Abs)
0x0a, 0x23, 0x05, // USAGE (Day)
0x15, 0x01, // LOGICAL_MINIMUM (1)
0x25, 0x1f, // LOGICAL_MAXIMUM (31)
0x65, 0x00, // UNIT (None)
0x81, 0x02, // INPUT (Data,Var,Abs)
0x0a, 0x24, 0x05, // USAGE (Day of Week)
0x15, 0x00, // LOGICAL_MINIMUM (0)
0x25, 0x06, // LOGICAL_MAXIMUM (6)
0xa1, 0x02, // COLLECTION (Logical)
0x0a, 0xc0, 0x08, // Day of Week: Sunday
0x0a, 0xc1, 0x08, // Day of Week: Monday
0x0a, 0xc2, 0x08, // Day of Week: Tuesday
0x0a, 0xc3, 0x08, // Day of Week: Wednesday
0x0a, 0xc4, 0x08, // Day of Week: Thursday
0x0a, 0xc5, 0x08, // Day of Week: Friday
0x0a, 0xc6, 0x08, // Day of Week: Saturday
0x81, 0x02, // INPUT (Const,Arr,Abs)
0xc0, // END_COLLECTION
0x0a, 0x25, 0x05, // USAGE (Hour)
0x15, 0x00, // LOGICAL_MINIMUM (0)
0x25, 0x17, // LOGICAL_MAXIMUM (23)
0x65, 0x00, // UNIT (None)
0x81, 0x02, // INPUT (Data,Var,Abs)
0x0a, 0x26, 0x05, // USAGE (Minute)
0x15, 0x00, // LOGICAL_MINIMUM (0)
0x25, 0x3b, // LOGICAL_MAXIMUM (59)
0x65, 0x00, // UNIT (None)
0x81, 0x02, // INPUT (Data,Var,Abs)
0x0a, 0x27, 0x05, // USAGE (Second)
0x15, 0x00, // LOGICAL_MINIMUM (0)
0x25, 0x3b, // LOGICAL_MAXIMUM (59)
0x65, 0x00, // UNIT (None)
//0x66, 0x10, 0x01, // UNIT (second)
0x81, 0x02, // INPUT (Data,Var,Abs)
0x0a, 0x28, 0x05, // USAGE (Millisecond)
0x75, 0x10, // REPORT_SIZE (16 bits)
0x65, 0x00, // UNIT (None)
0x81, 0x02, // INPUT (Data,Var,Abs)
0xc0, // END_COLLECTION
0xc0, // END_COLLECTION
--my-descriptor--
>
> We can use the existing HID parser in hid-core for hid_scan_report()
> by re-using the code from hid_open_report(). hid_parser_global,
> hid_parser_local and hid_parser_reserved does not have any side effects.
> We just need to reimplement the MAIN_ITEM callback to have a proper
> parsing without side effects.
>
> Instead of directly overwriting the ->group field, this patch introduce
> a ->flags field and then decide which group the device belongs to,
> depending on the whole parsing (not just the local item). This will be
> useful for Win 8 multitouch devices, which are multitouch devices and
> Win 8 certified (so 2 flags to check).
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@...hat.com>
> ---
> drivers/hid/hid-core.c | 131 +++++++++++++++++++++++++++++++++++--------------
> include/linux/hid.h | 4 ++
> 2 files changed, 97 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 3efe19f..d8cdb0a 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -677,10 +677,49 @@ static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
> return NULL;
> }
>
> -static void hid_scan_usage(struct hid_device *hid, u32 usage)
> +static void hid_scan_input_usage(struct hid_parser *parser, u32 usage)
> {
> if (usage == HID_DG_CONTACTID)
> - hid->group = HID_GROUP_MULTITOUCH;
> + parser->flags |= HID_FLAG_MULTITOUCH;
> +}
> +
> +static void hid_scan_open_collection(struct hid_parser *parser, unsigned type)
> +{
> + if (parser->global.usage_page == HID_UP_SENSOR &&
> + type == HID_COLLECTION_PHYSICAL)
> + parser->flags |= HID_FLAG_SENSOR_HUB;
> +}
> +
> +static int hid_scan_main(struct hid_parser *parser, struct hid_item *item)
> +{
> + __u32 data;
> + int i;
> +
> + data = item_udata(item);
> +
> + switch (item->tag) {
> + case HID_MAIN_ITEM_TAG_BEGIN_COLLECTION:
> + hid_scan_open_collection(parser, data & 0xff);
> + break;
> + case HID_MAIN_ITEM_TAG_END_COLLECTION:
> + break;
> + case HID_MAIN_ITEM_TAG_INPUT:
> + for (i = 0; i < parser->local.usage_index; i++)
> + hid_scan_input_usage(parser, parser->local.usage[i]);
> + break;
> + case HID_MAIN_ITEM_TAG_OUTPUT:
> + break;
> + case HID_MAIN_ITEM_TAG_FEATURE:
> + break;
> + default:
> + hid_err(parser->device, "unknown main item tag 0x%x\n",
> + item->tag);
> + }
> +
> + /* Reset the local parser environment */
> + memset(&parser->local, 0, sizeof(parser->local));
> +
> + return 0;
> }
>
> /*
> @@ -690,49 +729,65 @@ static void hid_scan_usage(struct hid_device *hid, u32 usage)
> */
> static int hid_scan_report(struct hid_device *hid)
> {
> - unsigned int page = 0, delim = 0;
> + struct hid_parser *parser;
> + struct hid_item item;
> __u8 *start = hid->dev_rdesc;
> __u8 *end = start + hid->dev_rsize;
> - unsigned int u, u_min = 0, u_max = 0;
> - struct hid_item item;
> + int ret;
> + static int (*dispatch_type[])(struct hid_parser *parser,
> + struct hid_item *item) = {
> + hid_scan_main,
> + hid_parser_global,
> + hid_parser_local,
> + hid_parser_reserved
> + };
>
> - hid->group = HID_GROUP_GENERIC;
> + parser = vzalloc(sizeof(struct hid_parser));
> + if (!parser)
> + return -ENOMEM;
> +
> + parser->device = hid;
> +
> + ret = -EINVAL;
> while ((start = fetch_item(start, end, &item)) != NULL) {
> - if (item.format != HID_ITEM_FORMAT_SHORT)
> - return -EINVAL;
> - if (item.type == HID_ITEM_TYPE_GLOBAL) {
> - if (item.tag == HID_GLOBAL_ITEM_TAG_USAGE_PAGE)
> - page = item_udata(&item) << 16;
> - } else if (item.type == HID_ITEM_TYPE_LOCAL) {
> - if (delim > 1)
> - break;
> - u = item_udata(&item);
> - if (item.size <= 2)
> - u += page;
> - switch (item.tag) {
> - case HID_LOCAL_ITEM_TAG_DELIMITER:
> - delim += !!u;
> - break;
> - case HID_LOCAL_ITEM_TAG_USAGE:
> - hid_scan_usage(hid, u);
> - break;
> - case HID_LOCAL_ITEM_TAG_USAGE_MINIMUM:
> - u_min = u;
> - break;
> - case HID_LOCAL_ITEM_TAG_USAGE_MAXIMUM:
> - u_max = u;
> - for (u = u_min; u <= u_max; u++)
> - hid_scan_usage(hid, u);
> - break;
> +
> + if (item.format != HID_ITEM_FORMAT_SHORT) {
> + hid_err(hid, "unexpected long global item\n");
> + goto out;
> + }
> +
> + if (dispatch_type[item.type](parser, &item)) {
> + hid_err(hid, "item %u %u %u %u parsing failed\n",
> + item.format, (unsigned)item.size,
> + (unsigned)item.type, (unsigned)item.tag);
> + goto out;
> + }
> +
> + if (start == end) {
> + if (parser->local.delimiter_depth) {
> + hid_err(hid, "unbalanced delimiter at end of report description\n");
> + goto out;
> }
> - } else if (page == HID_UP_SENSOR &&
> - item.type == HID_ITEM_TYPE_MAIN &&
> - item.tag == HID_MAIN_ITEM_TAG_BEGIN_COLLECTION &&
> - (item_udata(&item) & 0xff) == HID_COLLECTION_PHYSICAL)
> - hid->group = HID_GROUP_SENSOR_HUB;
> + ret = 0;
> + goto out;
> + }
> }
>
> - return 0;
> + hid_err(hid, "item fetching failed at offset %d\n", (int)(end - start));
> +out:
> + switch (parser->flags) {
> + case HID_FLAG_MULTITOUCH:
> + hid->group = HID_GROUP_MULTITOUCH;
> + break;
> + case HID_FLAG_SENSOR_HUB:
> + hid->group = HID_GROUP_SENSOR_HUB;
> + break;
> + default:
> + hid->group = HID_GROUP_GENERIC;
> + }
> +
> + vfree(parser);
> + return ret;
> }
>
> /**
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 5a4e789..7d823db 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -533,6 +533,9 @@ static inline void hid_set_drvdata(struct hid_device *hdev, void *data)
> #define HID_GLOBAL_STACK_SIZE 4
> #define HID_COLLECTION_STACK_SIZE 4
>
> +#define HID_FLAG_MULTITOUCH 0x0001
> +#define HID_FLAG_SENSOR_HUB 0x0002
> +
> struct hid_parser {
> struct hid_global global;
> struct hid_global global_stack[HID_GLOBAL_STACK_SIZE];
> @@ -541,6 +544,7 @@ struct hid_parser {
> unsigned collection_stack[HID_COLLECTION_STACK_SIZE];
> unsigned collection_stack_ptr;
> struct hid_device *device;
> + unsigned flags;
> };
>
> struct hid_class_descriptor {
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists