| 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
| ||
|
Message-ID: <0226eb8f5b20acd07e1bdd49c855e692ecad6306.camel@suse.de>
Date: Wed, 27 Mar 2019 11:15:16 +0100
From: Nicolas Saenz Julienne <nsaenzjulienne@...e.de>
To: "Junge, Terry" <terry.junge@...y.com>,
Jiri Kosina <jikos@...nel.org>,
Benjamin Tissoires <benjamin.tissoires@...hat.com>
Cc: "oneukum@...e.de" <oneukum@...e.de>,
"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] HID: core: move Usage Page concatenation to Main item
Hi Terry, thanks for the review!
On Tue, 2019-03-26 at 22:43 +0000, Junge, Terry wrote:
> Hi Nicolas,
>
> This patch looks good except for one comment/question below.
>
> Thanks,
> Terry
>
> On Tuesday, March 26, 2019 1:04 PM Nicolas Saenz Julienne <
> nsaenzjulienne@...e.de> wrote:
> > As seen on some USB wireless keyboards manufactured by Primax, the HID
> > parser was using some assumptions that are not always true. In this case
> > it's s
> > the fact that, inside the scope of a main item, an Usage Page will always
> > precede an Usage.
> >
> > The spec is not pretty clear as 6.2.2.7 states "Any usage that follows is
> > interpreted as a Usage ID and concatenated with the Usage Page".
> > While 6.2.2.8 states "When the parser encounters a main item it concatenates
> > the last declared Usage Page with a Usage to form a complete usage value."
> > Being somewhat contradictory it was decided to match Window's
> > implementation, which follows 6.2.2.8.
> >
> > In summary, the patch moves the Usage Page concatenation from the local
> > item parsing function to the main item parsing function.
> >
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@...e.de>
> > ---
> >
> > v2->v3: - Update patch title
> >
> > v1->v2: - Add usage concatenation to hid_scan_main()
> > - Rework tests in hid-tools, making sure no-one is failing
> >
> > drivers/hid/hid-core.c | 40 ++++++++++++++++++++++++++++------------
> > include/linux/hid.h | 1 +
> > 2 files changed, 29 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index
> > 9993b692598f..40c836ce3248 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -218,13 +218,14 @@ static unsigned hid_lookup_collection(struct
> > hid_parser *parser, unsigned type)
> > * Add a usage to the temporary parser table.
> > */
> >
> > -static int hid_add_usage(struct hid_parser *parser, unsigned usage)
> > +static int hid_add_usage(struct hid_parser *parser, unsigned usage,
> > +__u8 size)
> > {
> > if (parser->local.usage_index >= HID_MAX_USAGES) {
> > hid_err(parser->device, "usage index exceeded\n");
> > return -1;
> > }
> > parser->local.usage[parser->local.usage_index] = usage;
> > + parser->local.usage_size[parser->local.usage_index] = size;
> > parser->local.collection_index[parser->local.usage_index] =
> > parser->collection_stack_ptr ?
> > parser->collection_stack[parser->collection_stack_ptr - 1] : 0;
> > @@ -486,10 +487,7 @@ static int hid_parser_local(struct hid_parser *parser,
> > struct hid_item *item)
> > return 0;
> > }
> >
> > - if (item->size <= 2)
> > - data = (parser->global.usage_page << 16) + data;
> > -
> > - return hid_add_usage(parser, data);
> > + return hid_add_usage(parser, data, item->size);
> >
> > case HID_LOCAL_ITEM_TAG_USAGE_MINIMUM:
> >
> > @@ -498,9 +496,6 @@ static int hid_parser_local(struct hid_parser *parser,
> > struct hid_item *item)
> > return 0;
> > }
> >
> > - if (item->size <= 2)
> > - data = (parser->global.usage_page << 16) + data;
> > -
> > parser->local.usage_minimum = data;
> > return 0;
> >
> > @@ -511,9 +506,6 @@ static int hid_parser_local(struct hid_parser *parser,
> > struct hid_item *item)
> > return 0;
> > }
> >
> > - if (item->size <= 2)
> > - data = (parser->global.usage_page << 16) + data;
> > -
> > count = data - parser->local.usage_minimum;
> > if (count + parser->local.usage_index >= HID_MAX_USAGES) {
> > /*
> > @@ -533,7 +525,7 @@ static int hid_parser_local(struct hid_parser *parser,
> > struct hid_item *item)
> > }
> >
> > for (n = parser->local.usage_minimum; n <= data; n++)
> > - if (hid_add_usage(parser, n)) {
> > + if (hid_add_usage(parser, n, item->size)) {
> > dbg_hid("hid_add_usage failed\n");
> > return -1;
> > }
> > @@ -547,6 +539,26 @@ static int hid_parser_local(struct hid_parser *parser,
> > struct hid_item *item)
> > return 0;
> > }
> >
> > +/*
> > + * Concatenate Usage Pages into Usages where relevant:
> > + * As per specification, 6.2.2.8: "When the parser encounters a main
> > +item it
> > + * concatenates the last declared Usage Page with a Usage to form a
> > +complete
> > + * usage value."
> > + */
> > +
> > +static void hid_concatenate_usage_page(struct hid_parser *parser) {
> > + unsigned usages;
> > + int i;
> > +
> > + usages = max_t(unsigned, parser->local.usage_index,
> > + parser->global.report_count);
>
> I don't think we need to worry about global.report_count here,
> just concatenate for the usages currently in the local queue so could
> this be simplified by removing usages and just using local.usage_index?
>
> for (i = 0; i < local.usage_index; i++)
Sounds about right.
>
> > +
> > + for (i = 0; i < usages; i++)
> > + if (parser->local.usage_size[i] <= 2)
> > + parser->local.usage[i] += parser->global.usage_page
> > << 16; }
> > +
> > /*
> > * Process a main item.
> > */
> > @@ -556,6 +568,8 @@ static int hid_parser_main(struct hid_parser *parser,
> > struct hid_item *item)
> > __u32 data;
> > int ret;
> >
> > + hid_concatenate_usage_page(parser);
> > +
> > data = item_udata(item);
> >
> > switch (item->tag) {
> > @@ -765,6 +779,8 @@ static int hid_scan_main(struct hid_parser *parser,
> > struct hid_item *item)
> > __u32 data;
> > int i;
> >
> > + hid_concatenate_usage_page(parser);
> > +
> > data = item_udata(item);
> >
> > switch (item->tag) {
> > diff --git a/include/linux/hid.h b/include/linux/hid.h index
> > f9707d1dcb58..d1fb4b678873 100644
> > --- a/include/linux/hid.h
> > +++ b/include/linux/hid.h
> > @@ -417,6 +417,7 @@ struct hid_global {
> >
> > struct hid_local {
> > unsigned usage[HID_MAX_USAGES]; /* usage array */
> > + __u8 usage_size[HID_MAX_USAGES]; /* usage size array */
> > unsigned collection_index[HID_MAX_USAGES]; /* collection index
> > array */
> > unsigned usage_index;
> > unsigned usage_minimum;
> > --
> > 2.21.0
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists