From: Alan Stern <stern@rowland.harvard.edu>
To: Jiri Kosina <jikos@kernel.org>,
Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: linux-input@vger.kernel.org,
USB mailing list <linux-usb@vger.kernel.org>
Subject: Re: INFO: rcu detected stall in hub_event
Date: Mon, 8 Apr 2024 12:55:13 -0400 [thread overview]
Message-ID: <ade3bb13-e612-49a6-ace2-bf6eeca93f8e@rowland.harvard.edu> (raw)
In-Reply-To: <Pine.LNX.4.44L0.1912091318210.1462-100000@iolanthe.rowland.org>
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@syzkaller.appspotmail.com
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;
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
next prev parent reply other threads:[~2024-04-08 16:55 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-21 14:45 INFO: rcu detected stall in hub_event syzbot
2019-11-22 16:51 ` Alan Stern
2019-11-22 21:31 ` Andrey Konovalov
2019-11-25 17:30 ` Alan Stern
2019-12-09 18:26 ` Alan Stern
2019-12-10 21:26 ` [PATCH] HID: Fix slab-out-of-bounds read in hid_field_extract Alan Stern
2019-12-11 14:18 ` Jiri Kosina
2019-12-11 15:10 ` Alan Stern
2019-12-13 8:44 ` Jiri Kosina
2024-04-08 16:55 ` Alan Stern [this message]
2019-11-23 20:20 ` INFO: rcu detected stall in hub_event syzbot
2019-11-24 16:17 ` Alan Stern
2019-11-25 9:38 ` syzbot
2019-11-25 21:24 ` Alan Stern
2019-11-26 7:48 ` Jiri Kosina
2019-11-26 15:18 ` Alan Stern
2019-11-26 20:21 ` syzbot
2022-07-30 10:27 ` [syzbot] " syzbot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ade3bb13-e612-49a6-ace2-bf6eeca93f8e@rowland.harvard.edu \
--to=stern@rowland.harvard.edu \
--cc=benjamin.tissoires@redhat.com \
--cc=jikos@kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).