[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <zakga5qqql6zyat6wbnntm6tvcmhlhmjt5ecz6nm5hpc7z2iw7@mcpmrg7r4qlt>
Date: Thu, 17 Jul 2025 13:48:41 +0200
From: Benjamin Tissoires <bentiss@...nel.org>
To: Alan Stern <stern@...land.harvard.edu>
Cc: syzbot <syzbot+b63d677d63bcac06cf90@...kaller.appspotmail.com>,
jikos@...nel.org, linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-usb@...r.kernel.org, syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] [input?] [usb?] UBSAN: shift-out-of-bounds in s32ton (2)
Hi Alan,
On Jul 16 2025, Alan Stern wrote:
> Benjamin:
>
> On Wed, Jul 16, 2025 at 10:29:38AM -0400, Alan Stern wrote:
> > On Tue, Jul 15, 2025 at 12:50:04PM -0700, syzbot wrote:
> > > Hello,
> > >
> > > syzbot has tested the proposed patch but the reproducer is still triggering an issue:
> > > UBSAN: shift-out-of-bounds in s32ton
> > >
> > > microsoft 0003:045E:07DA.0001: ignoring exceeding usage max
> > > microsoft 0003:045E:07DA.0001: unsupported Resolution Multiplier 0
> > > hid: s32ton: n 0 val 0 size 0x0
> > > ------------[ cut here ]------------
> > > UBSAN: shift-out-of-bounds in drivers/hid/hid-core.c:69:16
> > > shift exponent 4294967295 is too large for 32-bit type '__s32' (aka 'int')
> >
> > Benjamin:
> >
> > Clearly there's going to be trouble when you try to convert a signed
> > 32-bit value to a 0-bit number!
> >
> > My impression is that hid_parser_global() should reject Report Size or
> > Report Count items with a value of 0. Such fields would be meaningless
> > in any case. The routine checks for values that are too large, but not
> > for values that are too small.
> >
> > Does this look like the right approach?
> >
> > Alan Stern
> >
> > #syz test: git://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git c2ca42f1
> >
> > drivers/hid/hid-core.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > Index: usb-devel/drivers/hid/hid-core.c
> > ===================================================================
> > --- usb-devel.orig/drivers/hid/hid-core.c
> > +++ usb-devel/drivers/hid/hid-core.c
> > @@ -464,7 +464,8 @@ static int hid_parser_global(struct hid_
> >
> > case HID_GLOBAL_ITEM_TAG_REPORT_SIZE:
> > parser->global.report_size = item_udata(item);
> > - if (parser->global.report_size > 256) {
> > + if (parser->global.report_size > 256 ||
> > + parser->global.report_size == 0) {
> > hid_err(parser->device, "invalid report_size %d\n",
> > parser->global.report_size);
> > return -1;
> > @@ -473,7 +474,8 @@ static int hid_parser_global(struct hid_
> >
> > case HID_GLOBAL_ITEM_TAG_REPORT_COUNT:
> > parser->global.report_count = item_udata(item);
> > - if (parser->global.report_count > HID_MAX_USAGES) {
> > + if (parser->global.report_count > HID_MAX_USAGES ||
> > + parser->global.report_count == 0) {
> > hid_err(parser->device, "invalid report_count %d\n",
> > parser->global.report_count);
> > return -1;
>
> This patch didn't work; the error message never showed up in the kernel
> log. Nevertheless, hidinput_change_resolution_multipliers() tried to
> create an output report with a field having size 0.
>
> How can this to happen without hid_scan_report() or hid_open_report()
> running? It shouldn't be possible to use a report before it has been
> checked for validity.
It's just that the provided report descriptor was never setting a report
size or a report count. This way, we are stuck with the default value
from kzalloc: 0.
Basically, if your report descriptor is as simple as:
Usage Page (Generic Desktop)
Usage (X)
Usage (Y)
Report Count (2)
Input (Data,Var,Rel)
Then we would trigger this bug: "report Size" is never set and is thus 0.
Your patch is good though, as it is probably a good thing to prevent a
report size/count to be 0. But it's not addressing the issue here
because the only time we can check for those values is when we receive
an Input/Feature/Output value (or ranges), so in hid_add_field().
Cheers,
Benjamin
>
> Alan Stern
Powered by blists - more mailing lists