Hi Alan [quick FYI, I'm lagging a lot upstream. I had a rough time in November and then got some internal work which lead me to be less present upstream. And now the holidays are coming. sigh] On Dec 17 2024, Alan Stern wrote: > Jiri and Benjamin: > > The syzbot monthly USB report led to this old email message, which was > never answered. The full bug report and email thread are here: > > https://lore.kernel.org/all/000000000000109c040597dc5843@xxxxxxxxxx/T/ > > The bug still has not been fixed, according to syzbot. Please review > this material and let me know whether the patch should be changed or > submitted. Sorry this fell through the cracks. > > Thanks, > > Alan Stern > > On Mon, Apr 08, 2024 at 12:55:13PM -0400, Alan Stern wrote: > > Jiri and Benjamin: > > > > Tracking down an old syzbot report from over four years ago (but still > > not closed out!) led me to this email thread. It turned out there were > > two separate bugs involved, one of which has since been fixed. I don't > > remember the issues very well, so here's a copy of what I wrote back > > then: > > > > On Mon, 09 Dec 2019, Alan Stern wrote: > > > > > The big problem is that the parser assumes all usages will > > > belong to a collection. > > > > > > There's also a second, smaller bug: hid_apply_multipler() assumes every > > > Resolution Multiplier control is associated with a Logical Collection > > > (i.e., there's no way the routine can ever set multiplier_collection to > > > NULL) even though there's a big quotation from the HID Usage Table > > > manual at the start of the function saying that they don't have to be. > > > This bug can be fixed easily, though. > > > > > > The first bug is more troublesome. hid_add_usage() explicitly sets the > > > parser->local.collection_index[] entry to 0 if the current collection > > > stack is empty. But there's no way to distinguish this 0 from a > > > genuine index value that happens to point to the first collection! > > > > > > So what should happen when a usage appears outside of all collections? > > > Is it a bug in the report descriptor (the current code suggests that it > > > is not)? > > > > > > Or should we use a different sentinel value for the collection_index[] > > > entry, one that cannot be confused with a genuine value, such as > > > UINT_MAX? > > > > Syzbot tested a proposed patch: > > > > On Tue, 26 Nov 2019, syzbot wrote: > > > > > Hello, > > > > > > syzbot has tested the proposed patch and the reproducer did not trigger > > > crash: > > > > > > Reported-and-tested-by: > > > syzbot+ec5f884c4a135aa0dbb9@xxxxxxxxxxxxxxxxxxxxxxxxx > > > > Here is the patch that syzbot tested: > > > > drivers/hid/hid-core.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > Index: usb-devel/drivers/hid/hid-core.c > > =================================================================== > > --- usb-devel.orig/drivers/hid/hid-core.c > > +++ usb-devel/drivers/hid/hid-core.c > > @@ -1057,6 +1057,8 @@ static void hid_apply_multiplier(struct > > while (multiplier_collection->parent_idx != -1 && > > multiplier_collection->type != HID_COLLECTION_LOGICAL) > > multiplier_collection = &hid->collection[multiplier_collection->parent_idx]; > > + if (multiplier_collection->type != HID_COLLECTION_LOGICAL) > > + multiplier_collection = NULL; As far as I can tell, this might be good. I had a hard time finding out if this is correct, but we are in undefined behavior, so we should probably just fix the bug. The selftests are all passing[0], so I guess we just need to respin the patch dropping the second hunk, no? Cheers, Benjamin [0] https://gitlab.freedesktop.org/bentiss/hid/-/pipelines/1333833 > > > > effective_multiplier = hid_calculate_multiplier(hid, multiplier); > > > > @@ -1191,6 +1193,9 @@ int hid_open_report(struct hid_device *d > > } > > device->collection_size = HID_DEFAULT_NUM_COLLECTIONS; > > > > + /* Needed for usages before the first collection */ > > + device->collection[0].parent_idx = -1; > > + > > ret = -EINVAL; > > while ((start = fetch_item(start, end, &item)) != NULL) { > > > > > > The second hunk, addressing the first bug described above, was > > implemented in commit ea427a222d8b ("HID: core: Fix deadloop in > > hid_apply_multiplier.") in 2023. But the first hunk, addressing the > > second bug, is still outstanding. > > > > You guys undoubtedly understand this code much better than I do. Is the > > first hunk in this patch still required? Is it a correct fix for > > handling Resolution Multiplier controls not associated with any Logical > > Collection? > > > > Alan Stern
