On Apr 17 2014 or thereabouts, Kees Cook wrote: > On Thu, Apr 17, 2014 at 10:37 AM, Benjamin Tissoires > <benjamin.tissoires@xxxxxxxxxx> wrote: > > On Apr 17 2014 or thereabouts, Kees Cook wrote: > >> On Thu, Apr 17, 2014 at 9:35 AM, <simon@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
