| Message ID | CAGXu5jKVCme=a8EVg1BOrB+5enpjT5aABy3QB=div-t230+NSA@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);