HID: hid-logitech - missing HID_OUTPUT_REPORT 0

HID: hid-logitech - missing HID_OUTPUT_REPORT 0

[simon]

[-- Attachment #1: Type: text/plain, Size: 1413 bytes --]

Hi Kees and all,
I've got a report from an end user that their Logitech F710 wireless
gamepad is not functioning correctly. This device has a switch to select
between 'X' mode (X-Pad, works OK) and 'D' mode (HID using hid-logitech,
doesn't work).

After some discussion off list, Elias and I think that this is related to
checking the report descriptor (attached).

Kernel log shows
--
[15961.607787] usb 5-5: USB disconnect, device number 12
[15962.235373] usb 5-5: new full-speed USB device number 13 using ohci-pci
[15962.412284] usb 5-5: New USB device found, idVendor=046d, idProduct=c219
[15962.412287] usb 5-5: New USB device strings: Mfr=1, Product=2,
SerialNumber=0
[15962.412289] usb 5-5: Product: Logitech Cordless RumblePad 2
[15962.412291] usb 5-5: Manufacturer: Logitech
[15962.424343] input: Logitech Logitech Cordless RumblePad 2 as
/devices/pci0000:00/0000:00:13.0/usb5/5-5/5-5:1.0/input/input26
[15962.424477] logitech 0003:046D:C219.0012: input,hidraw5: USB HID v1.11
Gamepad
[Logitech Logitech Cordless RumblePad 2] on usb-0000:00:13.0-5/input0
[15962.424480] logitech 0003:046D:C219.0012: missing HID_OUTPUT_REPORT 0
--

Last message comes from here (I believe):
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-lgff.c?id=refs/tags/v3.15-rc1#n137

Can anyone see what might be wrong with the report, to cause this code to
spit out an error?

Cheers,
Simon.

[-- Attachment #2: report_descriptor.txt --]
[-- Type: text/plain, Size: 3751 bytes --]

0x05, 0x01,         /*  Usage Page (Desktop),                   */
0x09, 0x05,         /*  Usage (Gamepad),                        */
0xA1, 0x01,         /*  Collection (Application),               */
0xA1, 0x02,         /*      Collection (Logical),               */
0x85, 0x01,         /*          Report ID (1),                  */
0x75, 0x08,         /*          Report Size (8),                */
0x95, 0x04,         /*          Report Count (4),               */
0x15, 0x00,         /*          Logical Minimum (0),            */
0x26, 0xFF, 0x00,   /*          Logical Maximum (255),          */
0x35, 0x00,         /*          Physical Minimum (0),           */
0x46, 0xFF, 0x00,   /*          Physical Maximum (255),         */
0x09, 0x30,         /*          Usage (X),                      */
0x09, 0x31,         /*          Usage (Y),                      */
0x09, 0x32,         /*          Usage (Z),                      */
0x09, 0x35,         /*          Usage (Rz),                     */
0x81, 0x02,         /*          Input (Variable),               */
0x75, 0x04,         /*          Report Size (4),                */
0x95, 0x01,         /*          Report Count (1),               */
0x25, 0x07,         /*          Logical Maximum (7),            */
0x46, 0x3B, 0x01,   /*          Physical Maximum (315),         */
0x66, 0x14, 0x00,   /*          Unit (Degrees),                 */
0x09, 0x39,         /*          Usage (Hat Switch),             */
0x81, 0x42,         /*          Input (Variable, Null State),   */
0x66, 0x00, 0x00,   /*          Unit,                           */
0x75, 0x01,         /*          Report Size (1),                */
0x95, 0x0C,         /*          Report Count (12),              */
0x25, 0x01,         /*          Logical Maximum (1),            */
0x45, 0x01,         /*          Physical Maximum (1),           */
0x05, 0x09,         /*          Usage Page (Button),            */
0x19, 0x01,         /*          Usage Minimum (01h),            */
0x29, 0x0C,         /*          Usage Maximum (0Ch),            */
0x81, 0x02,         /*          Input (Variable),               */
0x95, 0x01,         /*          Report Count (1),               */
0x75, 0x08,         /*          Report Size (8),                */
0x06, 0x00, 0xFF,   /*          Usage Page (FF00h),             */
0x26, 0xFF, 0x00,   /*          Logical Maximum (255),          */
0x46, 0xFF, 0x00,   /*          Physical Maximum (255),         */
0x09, 0x00,         /*          Usage (00h),                    */
0x81, 0x02,         /*          Input (Variable),               */
0xC0,               /*      End Collection,                     */
0xA1, 0x02,         /*      Collection (Logical),               */
0x85, 0x02,         /*          Report ID (2),                  */
0x95, 0x07,         /*          Report Count (7),               */
0x75, 0x08,         /*          Report Size (8),                */
0x26, 0xFF, 0x00,   /*          Logical Maximum (255),          */
0x46, 0xFF, 0x00,   /*          Physical Maximum (255),         */
0x06, 0x00, 0xFF,   /*          Usage Page (FF00h),             */
0x09, 0x03,         /*          Usage (03h),                    */
0x81, 0x02,         /*          Input (Variable),               */
0xC0,               /*      End Collection,                     */
0xA1, 0x02,         /*      Collection (Logical),               */
0x85, 0x03,         /*          Report ID (3),                  */
0x09, 0x04,         /*          Usage (04h),                    */
0x91, 0x02,         /*          Output (Variable),              */
0xC0,               /*      End Collection,                     */
0xC0                /*  End Collection                          */

Re: HID: hid-logitech - missing HID_OUTPUT_REPORT 0

[Kees Cook]

On Wed, Apr 16, 2014 at 3:35 PM,  <simon@mungewell.org> wrote:
> Hi Kees and all,
> I've got a report from an end user that their Logitech F710 wireless
> gamepad is not functioning correctly. This device has a switch to select
> between 'X' mode (X-Pad, works OK) and 'D' mode (HID using hid-logitech,
> doesn't work).
>
> After some discussion off list, Elias and I think that this is related to
> checking the report descriptor (attached).
>
> Kernel log shows
> --
> [15961.607787] usb 5-5: USB disconnect, device number 12
> [15962.235373] usb 5-5: new full-speed USB device number 13 using ohci-pci
> [15962.412284] usb 5-5: New USB device found, idVendor=046d, idProduct=c219
> [15962.412287] usb 5-5: New USB device strings: Mfr=1, Product=2,
> SerialNumber=0
> [15962.412289] usb 5-5: Product: Logitech Cordless RumblePad 2
> [15962.412291] usb 5-5: Manufacturer: Logitech
> [15962.424343] input: Logitech Logitech Cordless RumblePad 2 as
> /devices/pci0000:00/0000:00:13.0/usb5/5-5/5-5:1.0/input/input26
> [15962.424477] logitech 0003:046D:C219.0012: input,hidraw5: USB HID v1.11
> Gamepad
> [Logitech Logitech Cordless RumblePad 2] on usb-0000:00:13.0-5/input0
> [15962.424480] logitech 0003:046D:C219.0012: missing HID_OUTPUT_REPORT 0
> --
>
> Last message comes from here (I believe):
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-lgff.c?id=refs/tags/v3.15-rc1#n137
>
> Can anyone see what might be wrong with the report, to cause this code to
> spit out an error?

I don't know the lg driver very well, but it looks like it's expecting
a single report ID (0), but the device is showing two report IDs: 1
and 2. So, from the perspective of the driver, this is correct: it
wouldn't know how to deal with things since it is only expecting
Report ID 0. It seems like the driver needs to be updated for this
different device.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: HID: hid-logitech - missing HID_OUTPUT_REPORT 0

[simon]

> I don't know the lg driver very well, but it looks like it's expecting
> a single report ID (0), but the device is showing two report IDs: 1
> and 2. So, from the perspective of the driver, this is correct: it
> wouldn't know how to deal with things since it is only expecting
> Report ID 0. It seems like the driver needs to be updated for this
> different device.

Hi,
The 'hid-lgff.c' driver supports lots of devices (see end of 'hid-lg.c'),
and presumably these devices offer up a wide/varied range of HID
descriptors.

Does the recently introduced(/changed) check need to have prior knowledge
of which 'Report ID's are actually used? If so, it possible that the
change has broken a number of devices...

I am trying to get the end user to test with an older kernel to see
whether his device was always 'broken'.

Thanks.
Simon

Re: HID: hid-logitech - missing HID_OUTPUT_REPORT 0

[Kees Cook]

On Thu, Apr 17, 2014 at 9:35 AM,  <simon@mungewell.org> wrote:
>
>> I don't know the lg driver very well, but it looks like it's expecting
>> a single report ID (0), but the device is showing two report IDs: 1
>> and 2. So, from the perspective of the driver, this is correct: it
>> wouldn't know how to deal with things since it is only expecting
>> Report ID 0. It seems like the driver needs to be updated for this
>> different device.
>
> Hi,
> The 'hid-lgff.c' driver supports lots of devices (see end of 'hid-lg.c'),
> and presumably these devices offer up a wide/varied range of HID
> descriptors.
>
> Does the recently introduced(/changed) check need to have prior knowledge
> of which 'Report ID's are actually used? If so, it possible that the
> change has broken a number of devices...
>
> I am trying to get the end user to test with an older kernel to see
> whether his device was always 'broken'.

Ah-ha, actually, when taking a closer look at this, I see that lgff
isn't using ID "0", it's actually using "the first report in the
list", without using an ID at all. This appears to be true for all the
lg*ff devices, actually. Instead of validating ID 0, it needs to
validate "the first report".

Documentation for hid_validate_values says:
 * @id: which report ID to examine (0 for first)

Benjamin, does that mean "first report in the list", or is that a hint
that IDs are 0-indexed? What do you think is the best way to handle
this? Seems like passing something for "id" that means "whatever is
first in list" would be safest? I don't think overloading the meaning
of "0" is good, in case a driver really is using a 0 index but no
report with that ID exists in the list. Or if we do change the
meaning, make sure drivers always use the report returned by
hid_validate_values instead of re-finding it later.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: HID: hid-logitech - missing HID_OUTPUT_REPORT 0

[Benjamin Tissoires]

On Apr 17 2014 or thereabouts, Kees Cook wrote:
> On Thu, Apr 17, 2014 at 9:35 AM,  <simon@mungewell.org> wrote:
> >
> >> I don't know the lg driver very well, but it looks like it's expecting
> >> a single report ID (0), but the device is showing two report IDs: 1
> >> and 2. So, from the perspective of the driver, this is correct: it
> >> wouldn't know how to deal with things since it is only expecting
> >> Report ID 0. It seems like the driver needs to be updated for this
> >> different device.
> >
> > Hi,
> > The 'hid-lgff.c' driver supports lots of devices (see end of 'hid-lg.c'),
> > and presumably these devices offer up a wide/varied range of HID
> > descriptors.
> >
> > Does the recently introduced(/changed) check need to have prior knowledge
> > of which 'Report ID's are actually used? If so, it possible that the
> > change has broken a number of devices...
> >
> > I am trying to get the end user to test with an older kernel to see
> > whether his device was always 'broken'.
> 
> Ah-ha, actually, when taking a closer look at this, I see that lgff
> isn't using ID "0", it's actually using "the first report in the
> list", without using an ID at all. This appears to be true for all the
> lg*ff devices, actually. Instead of validating ID 0, it needs to
> validate "the first report".
> 
> Documentation for hid_validate_values says:
>  * @id: which report ID to examine (0 for first)
> 
> Benjamin, does that mean "first report in the list", or is that a hint

yep

> that IDs are 0-indexed? 

nope :)

page 46 of the HID 1.11 spec (http://www.usb.org/developers/devclass_docs/HID1_11.pdf)
says: "Report ID zero is reserved and should not be used."

> What do you think is the best way to handle
> this? Seems like passing something for "id" that means "whatever is
> first in list" would be safest? I don't think overloading the meaning
> of "0" is good, in case a driver really is using a 0 index but no
> report with that ID exists in the list.

"Overloading" 0 here is fine because reportID==0 can not exist according
to the spec. Actually, a HID device is not forced to use report IDs at
all if it sends only one type of reports.
So in the various transport layers, 0 is handled as a special case
anyway, and that means that there is no report ID. And when there is no
report ID, there should be only one which is the first in the list :)

Still, hid-lg should not use this trick to find the first report, but
this driver has quite a lot of history, so I will not try to fix it.

Anyway, it looks like hid_validate_values has to be fixed to handle HID
devices without a report ID (which would fix hid-lg too).

> Or if we do change the
> meaning, make sure drivers always use the report returned by
> hid_validate_values instead of re-finding it later.

As explained above, it shouldn't matter. And it's more likely a bug in
hid_validate_values that I should have spot when reviewing it :/

Cheers,
Benjamin

Re: HID: hid-logitech - missing HID_OUTPUT_REPORT 0

[Kees Cook]

On Thu, Apr 17, 2014 at 10:37 AM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> On Apr 17 2014 or thereabouts, Kees Cook wrote:
>> On Thu, Apr 17, 2014 at 9:35 AM,  <simon@mungewell.org> wrote:
>> >
>> >> I don't know the lg driver very well, but it looks like it's expecting
>> >> a single report ID (0), but the device is showing two report IDs: 1
>> >> and 2. So, from the perspective of the driver, this is correct: it
>> >> wouldn't know how to deal with things since it is only expecting
>> >> Report ID 0. It seems like the driver needs to be updated for this
>> >> different device.
>> >
>> > Hi,
>> > The 'hid-lgff.c' driver supports lots of devices (see end of 'hid-lg.c'),
>> > and presumably these devices offer up a wide/varied range of HID
>> > descriptors.
>> >
>> > Does the recently introduced(/changed) check need to have prior knowledge
>> > of which 'Report ID's are actually used? If so, it possible that the
>> > change has broken a number of devices...
>> >
>> > I am trying to get the end user to test with an older kernel to see
>> > whether his device was always 'broken'.
>>
>> Ah-ha, actually, when taking a closer look at this, I see that lgff
>> isn't using ID "0", it's actually using "the first report in the
>> list", without using an ID at all. This appears to be true for all the
>> lg*ff devices, actually. Instead of validating ID 0, it needs to
>> validate "the first report".
>>
>> Documentation for hid_validate_values says:
>>  * @id: which report ID to examine (0 for first)
>>
>> Benjamin, does that mean "first report in the list", or is that a hint
>
> yep
>
>> that IDs are 0-indexed?
>
> nope :)
>
> page 46 of the HID 1.11 spec (http://www.usb.org/developers/devclass_docs/HID1_11.pdf)
> says: "Report ID zero is reserved and should not be used."

Ah! Perfect, yes. And I see we're doing that validation:

        case HID_GLOBAL_ITEM_TAG_REPORT_ID:
                parser->global.report_id = item_udata(item);
                if (parser->global.report_id == 0 ||
                    parser->global.report_id >= HID_MAX_IDS) {
                        hid_err(parser->device, "report_id %u is invalid\n",
                                parser->global.report_id);
                        return -1;
                }
                return 0;


>> What do you think is the best way to handle
>> this? Seems like passing something for "id" that means "whatever is
>> first in list" would be safest? I don't think overloading the meaning
>> of "0" is good, in case a driver really is using a 0 index but no
>> report with that ID exists in the list.
>
> "Overloading" 0 here is fine because reportID==0 can not exist according
> to the spec. Actually, a HID device is not forced to use report IDs at
> all if it sends only one type of reports.
> So in the various transport layers, 0 is handled as a special case
> anyway, and that means that there is no report ID. And when there is no
> report ID, there should be only one which is the first in the list :)
>
> Still, hid-lg should not use this trick to find the first report, but
> this driver has quite a lot of history, so I will not try to fix it.
>
> Anyway, it looks like hid_validate_values has to be fixed to handle HID
> devices without a report ID (which would fix hid-lg too).
>
>> Or if we do change the
>> meaning, make sure drivers always use the report returned by
>> hid_validate_values instead of re-finding it later.
>
> As explained above, it shouldn't matter. And it's more likely a bug in
> hid_validate_values that I should have spot when reviewing it :/

How does this look? (Likely whitespace damaged -- I can resend
correctly if it's what you had in mind.)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 9e8064205bc7..5205ebb76cd2 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -840,6 +840,15 @@ struct hid_report *hid_validate_values(struct hid_device *h
         * drivers go to access report values.
         */
        report = hid->report_enum[type].report_id_hash[id];
+       if (!report && id == 0) {
+               /*
+                * Requesting id 0 means we should fall back to the first
+                * report in the list.
+                */
+               report = list_entry(
+                               hid->report_enum[type].report_list.next,
+                               struct hid_report, list);
+       }
        if (!report) {
                hid_err(hid, "missing %s %u\n", hid_report_names[type], id);
                return NULL;

Alternatively, should hid_register_report add a default to the hash
instead, so no change to hid_validate_values is needed?

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 9e8064205bc7..5d07124457ba 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -80,6 +80,8 @@ struct hid_report *hid_register_report(struct hid_device *devi
        report->size = 0;
        report->device = device;
        report_enum->report_id_hash[id] = report;
+       if (!report_enum->report_id_hash[0])
+               report_enum->report_id_hash[0] = report;

        list_add_tail(&report->list, &report_enum->report_list);



-Kees

-- 
Kees Cook
Chrome OS Security

Re: HID: hid-logitech - missing HID_OUTPUT_REPORT 0

[Benjamin Tissoires]

On Apr 17 2014 or thereabouts, Kees Cook wrote:
> On Thu, Apr 17, 2014 at 10:37 AM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> > On Apr 17 2014 or thereabouts, Kees Cook wrote:
> >> On Thu, Apr 17, 2014 at 9:35 AM,  <simon@mungewell.org> wrote:
> >> >
> >> >> I don't know the lg driver very well, but it looks like it's expecting
> >> >> a single report ID (0), but the device is showing two report IDs: 1
> >> >> and 2. So, from the perspective of the driver, this is correct: it
> >> >> wouldn't know how to deal with things since it is only expecting
> >> >> Report ID 0. It seems like the driver needs to be updated for this
> >> >> different device.
> >> >
> >> > Hi,
> >> > The 'hid-lgff.c' driver supports lots of devices (see end of 'hid-lg.c'),
> >> > and presumably these devices offer up a wide/varied range of HID
> >> > descriptors.
> >> >
> >> > Does the recently introduced(/changed) check need to have prior knowledge
> >> > of which 'Report ID's are actually used? If so, it possible that the
> >> > change has broken a number of devices...
> >> >
> >> > I am trying to get the end user to test with an older kernel to see
> >> > whether his device was always 'broken'.
> >>
> >> Ah-ha, actually, when taking a closer look at this, I see that lgff
> >> isn't using ID "0", it's actually using "the first report in the
> >> list", without using an ID at all. This appears to be true for all the
> >> lg*ff devices, actually. Instead of validating ID 0, it needs to
> >> validate "the first report".
> >>
> >> Documentation for hid_validate_values says:
> >>  * @id: which report ID to examine (0 for first)
> >>
> >> Benjamin, does that mean "first report in the list", or is that a hint
> >
> > yep
> >
> >> that IDs are 0-indexed?
> >
> > nope :)
> >
> > page 46 of the HID 1.11 spec (http://www.usb.org/developers/devclass_docs/HID1_11.pdf)
> > says: "Report ID zero is reserved and should not be used."
> 
> Ah! Perfect, yes. And I see we're doing that validation:
> 
>         case HID_GLOBAL_ITEM_TAG_REPORT_ID:
>                 parser->global.report_id = item_udata(item);
>                 if (parser->global.report_id == 0 ||
>                     parser->global.report_id >= HID_MAX_IDS) {
>                         hid_err(parser->device, "report_id %u is invalid\n",
>                                 parser->global.report_id);
>                         return -1;
>                 }
>                 return 0;
> 
> 
> >> What do you think is the best way to handle
> >> this? Seems like passing something for "id" that means "whatever is
> >> first in list" would be safest? I don't think overloading the meaning
> >> of "0" is good, in case a driver really is using a 0 index but no
> >> report with that ID exists in the list.
> >
> > "Overloading" 0 here is fine because reportID==0 can not exist according
> > to the spec. Actually, a HID device is not forced to use report IDs at
> > all if it sends only one type of reports.
> > So in the various transport layers, 0 is handled as a special case
> > anyway, and that means that there is no report ID. And when there is no
> > report ID, there should be only one which is the first in the list :)
> >
> > Still, hid-lg should not use this trick to find the first report, but
> > this driver has quite a lot of history, so I will not try to fix it.
> >
> > Anyway, it looks like hid_validate_values has to be fixed to handle HID
> > devices without a report ID (which would fix hid-lg too).
> >
> >> Or if we do change the
> >> meaning, make sure drivers always use the report returned by
> >> hid_validate_values instead of re-finding it later.
> >
> > As explained above, it shouldn't matter. And it's more likely a bug in
> > hid_validate_values that I should have spot when reviewing it :/
> 
> How does this look? (Likely whitespace damaged -- I can resend
> correctly if it's what you had in mind.)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 9e8064205bc7..5205ebb76cd2 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -840,6 +840,15 @@ struct hid_report *hid_validate_values(struct hid_device *h
>          * drivers go to access report values.
>          */
>         report = hid->report_enum[type].report_id_hash[id];
> +       if (!report && id == 0) {

I would place the test above the previous statement and just test
against id:

if (!id) {
	/* [comments] */
 report = list_entry(hid->report_enum[type].report_list.next,
                               struct hid_report, list);
 id = report->id;	
}

Or sth like that...

> +               /*
> +                * Requesting id 0 means we should fall back to the first
> +                * report in the list.
> +                */
> +               report = list_entry(
> +                               hid->report_enum[type].report_list.next,
> +                               struct hid_report, list);
> +       }
>         if (!report) {
>                 hid_err(hid, "missing %s %u\n", hid_report_names[type], id);
>                 return NULL;
> 
> Alternatively, should hid_register_report add a default to the hash
> instead, so no change to hid_validate_values is needed?
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 9e8064205bc7..5d07124457ba 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -80,6 +80,8 @@ struct hid_report *hid_register_report(struct hid_device *devi
>         report->size = 0;
>         report->device = device;
>         report_enum->report_id_hash[id] = report;
> +       if (!report_enum->report_id_hash[0])
> +               report_enum->report_id_hash[0] = report;

I don't like this that much, because id==0 should be a special case, and
I do not want to see drivers starting fetching report_enum->report_id_hash[0]
without knowing what they are doing.

Anyway, it will be Jiri's call, but I am more in favour of changing
hid_validate_report.

Cheers,
Benjamin

> 
>         list_add_tail(&report->list, &report_enum->report_list);
> 
> 
> 
> -Kees
> 
> -- 
> Kees Cook
> Chrome OS Security

Re: HID: hid-logitech - missing HID_OUTPUT_REPORT 0

[simon]

>>> Ah-ha, actually, when taking a closer look at this, I see that lgff
>>> isn't using ID "0", it's actually using "the first report in the
>>> list", without using an ID at all. This appears to be true for all the
>>> lg*ff devices, actually. Instead of validating ID 0, it needs to
>>> validate "the first report".


> +       if (!report && id == 0) {
> +               /*
> +                * Requesting id 0 means we should fall back to the first
> +                * report in the list.
> +                */
> +               report = list_entry(
> +                               hid->report_enum[type].report_list.next,
> +                               struct hid_report, list);
> +       }

Is the task of this check to locate/check the 'output' report? Because for
this particular device it is defined in Report ID 3, the third one in
descriptor. So would presumably still fail to be found.

I don't have any devices which use hid-lgff, but have some which use
hid-lg4ff which was also changed to perform the same test. I can check
their operation/HID reports over the weekend.

Simon.

Re: HID: hid-logitech - missing HID_OUTPUT_REPORT 0

[Kees Cook]

On Thu, Apr 17, 2014 at 11:27 AM,  <simon@mungewell.org> wrote:
>
>>>> Ah-ha, actually, when taking a closer look at this, I see that lgff
>>>> isn't using ID "0", it's actually using "the first report in the
>>>> list", without using an ID at all. This appears to be true for all the
>>>> lg*ff devices, actually. Instead of validating ID 0, it needs to
>>>> validate "the first report".
>
>
>> +       if (!report && id == 0) {
>> +               /*
>> +                * Requesting id 0 means we should fall back to the first
>> +                * report in the list.
>> +                */
>> +               report = list_entry(
>> +                               hid->report_enum[type].report_list.next,
>> +                               struct hid_report, list);
>> +       }
>
> Is the task of this check to locate/check the 'output' report? Because for
> this particular device it is defined in Report ID 3, the third one in
> descriptor. So would presumably still fail to be found.

The driver currently uses "the first entry in the report list",
regardless of ID. The bug that got introduced here was that the driver
was effectively forced to look up reports by ID, and the code didn't
acknowledge the concept of ID 0 effectively being a wildcard ID.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: HID: hid-logitech - missing HID_OUTPUT_REPORT 0

[simon]

> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 9e8064205bc7..5205ebb76cd2 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -840,6 +840,15 @@ struct hid_report *hid_validate_values(struct
> hid_device *h
>          * drivers go to access report values.
>          */
>         report = hid->report_enum[type].report_id_hash[id];
> +       if (!report && id == 0) {
> +               /*
> +                * Requesting id 0 means we should fall back to the first
> +                * report in the list.
> +                */
> +               report = list_entry(
> +                               hid->report_enum[type].report_list.next,
> +                               struct hid_report, list);
> +       }
>         if (!report) {
>                 hid_err(hid, "missing %s %u\n", hid_report_names[type],
> id);
>                 return NULL;
>
> Alternatively, should hid_register_report add a default to the hash
> instead, so no change to hid_validate_values is needed?
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 9e8064205bc7..5d07124457ba 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -80,6 +80,8 @@ struct hid_report *hid_register_report(struct hid_device
> *devi
>         report->size = 0;
>         report->device = device;
>         report_enum->report_id_hash[id] = report;
> +       if (!report_enum->report_id_hash[0])
> +               report_enum->report_id_hash[0] = report;
>
>         list_add_tail(&report->list, &report_enum->report_list);
>
>
>
> -Kees

I tried this patch last night and saw Kernel Panics on controller
insertion and/or removal....

And just noticed that you have another more recent one, so I'll try that
tonight. My bad :-(

Simon.