linux-kernel - Re: [syzbot] [input?] [usb?] UBSAN: shift-out-of-bounds in s32ton (2)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <15723551-960b-4257-bfbd-073e136deaa2@rowland.harvard.edu>
Date: Thu, 17 Jul 2025 11:10:01 -0400
From: Alan Stern <stern@...land.harvard.edu>
To: Benjamin Tissoires <bentiss@...nel.org>
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)

On Thu, Jul 17, 2025 at 01:48:41PM +0200, Benjamin Tissoires wrote:
> Hi Alan,
> 
> On Jul 16 2025, Alan Stern wrote:
> > > 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?

...

> > 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().

Of course!  Thanks for the pointer.  Let's try it out.

Alan Stern

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git c2ca42f190b6

Index: usb-devel/drivers/hid/hid-core.c
===================================================================
--- usb-devel.orig/drivers/hid/hid-core.c
+++ usb-devel/drivers/hid/hid-core.c
@@ -313,7 +313,14 @@ static int hid_add_field(struct hid_pars
 	}
 
 	offset = report->size;
-	report->size += parser->global.report_size * parser->global.report_count;
+	i = parser->global.report_size * parser->global.report_count;
+	if (i == 0) {
+		dbg_hid("invalid field size/count 0x%x 0x%x\n",
+			parser->global.report_size,
+			parser->global.report_count);
+		return -1;
+	}
+	report->size += i;
 
 	if (parser->device->ll_driver->max_buffer_size)
 		max_buffer_size = parser->device->ll_driver->max_buffer_size;
@@ -464,7 +471,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 +481,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;