linux-kernel - Re: [RFC/RFT] HID: primax: Fix wireless keyboards descriptor

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAO-hwJ+9ft070W-shhEwxfTq9B5XyVB2v6Qx0b4nC4LdTiTEDg@mail.gmail.com>
Date:   Fri, 1 Mar 2019 10:48:44 +0100
From:   Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:     Nicolas Saenz Julienne <nsaenzjulienne@...e.de>
Cc:     "Junge, Terry" <Terry.Junge@...ntronics.com>,
        Jiri Kosina <jikos@...nel.org>,
        "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: [RFC/RFT] HID: primax: Fix wireless keyboards descriptor

On Thu, Feb 28, 2019 at 7:01 PM Nicolas Saenz Julienne
<nsaenzjulienne@...e.de> wrote:
>
> On Thu, 2019-02-28 at 17:02 +0000, Junge, Terry wrote:
> > This could also be a parser error. In the HID specification section 6.2.2.8 it
> > states that the last declared Usage Page is applied to Usages when the Main
> > item is encountered.
> >
> > "If the bSize field = 1 or 2 then the Usage is interpreted as an unsigned
> > value
> > that selects a Usage ID on the currently defined Usage Page. When the parser
> > encounters a main item it concatenates the last declared Usage Page with a
> > Usage to form a complete usage value. Extended usages can be used to
> > override the currently defined Usage Page for individual usages."
> >
>
> Hi Terry, thanks for the comment.
> Just for the record the paragraph I cited on my patch is the following:
>
>         6.2.2.7 Global Items
>
>         [...]
>
>         Usage Page: Unsigned integer specifying the current Usage Page. Since a
>         usage are 32 bit values, Usage Page items can be used to conserve space
>         in a report descriptor by setting the high order 16 bits of a
>         subsequent usages. Any usage that follows which is defines* 16 bits or
>         less is interpreted as a Usage ID and concatenated with the Usage Page
>         to form a 32 bit Usage.
>
>         * This is a spec errata, I belive it should say "defined"
>
> As you can see they use the word "follows" which in my opinion contradicts the
> paragraph you pointed out. That said I may be wrong, I'm not too good at
> reading specs :).

I think you are both right (that is some decision making skills :-P).

I never saw a case where the Usage Page was after the Usage. And I
would lean towards Nicolas interpretation.
However, Terry's point is valid too, and by re-reading the two
paragraphs, one could argue that the "follows" of 6.2.2.7 could be
applied when the Main item is processed as in 6.2.2.8.

So I am going to base my decision on the "reference" driver. How well
behaves Windows with this particular keyboard?

If it behaves properly, then we better use the 6.2.2.8 version where
the Usage Page is only appended when there is a Main item. This means
we need to remember what size was the last Usage (and Min/Max), to
know if we should concatenate the 2.

Note that for every change that involves the generic parser, I'd like
a few tests added to `tests/test_keyboard.py in
https://gitlab.freedesktop.org/libevdev/hid-tools
Also note that the HID parser implementation in hid-tools follows the
kernel, so we might need to adjust it there too if we want to add high
level events. If you directly call `.call_input_event()` with raw
events, it should be fine.

>
> I checked the HID parser and it's indeed written assuming local items are
> preceded by a Usage Page. I'd be glad to fix it there, but it would be nice to
> have the mantainer's opinion on the matter first.
>

You've now got the opinion of one of the 2 maintainers, hope this helps :)

Cheers,
Benjamin