linux-kernel - Re: [PATCH] HID: core: Reject report fields with a size or count of 0

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: <mcigin2behhkoiq3dzgpjqjyqynskn4kgvuympi55wnlr3nud6@yuljmwr2zlfp>
Date: Sat, 19 Jul 2025 10:40:51 +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: [PATCH] HID: core: Reject report fields with a size or count of 0

On Jul 19 2025, Benjamin Tissoires wrote:
> On Jul 17 2025, Alan Stern wrote:
> > Testing by the syzbot fuzzer showed that the HID core gets a
> > shift-out-of-bounds exception when it tries to convert a 32-bit
> > quantity to a 0-bit quantity.  This is hardly an unexpected result,
> > but it means that we should not accept report fields that have a size
> > of zero bits.  Similarly, there's no reason to accept report fields
> > with a count of zero; either type of item is completely meaningless
> > since no information can be conveyed in zero bits.
> > 
> > Reject fields with a size or count of zero, and reject reports
> > containing such fields.
> > 
> > Signed-off-by: Alan Stern <stern@...land.harvard.edu>
> > Reported-by: syzbot+b63d677d63bcac06cf90@...kaller.appspotmail.com
> > Closes: https://lore.kernel.org/linux-usb/68753a08.050a0220.33d347.0008.GAE@google.com/
> > Tested-by: syzbot+b63d677d63bcac06cf90@...kaller.appspotmail.com
> > Fixes: dde5845a529f ("[PATCH] Generic HID layer - code split")
> > Cc: stable@...r.kernel.org
> > 
> > ---
> > 
> > The commit listed in the Fixes tag is not really the right one.  But
> > code motion made tracking it back any further more difficult than I
> > wanted to deal with, so I stopped there.  That commit is from 2006,
> > which is already far enough in the past.
> > 
> >  drivers/hid/hid-core.c |   15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > 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;
> 
> Sigh... I applied this one too quickly before going on holidays.
> 
> This breaks the hid testsuite:
> https://gitlab.freedesktop.org/bentiss/hid/-/jobs/80805946
> 
> (yes, I should have run it before applying).
> 
> So basically, there are devices out there with a "broken" report size of
> 0, and this patch now entirely disables them.
> 
> That Saitek gamepad has the following (see tools/testing/selftests/hid/tests/test_gamepad.py):
>         0x95, 0x01,                    # ..Report Count (1)                  60
>         0x75, 0x00,                    # ..Report Size (0)                   62
>         0x81, 0x03,                    # ..Input (Cnst,Var,Abs)              64
> 
> So we'd need to disable the field, but not invalidate the entire report.
> 
> I'm glad I scheduled this one for the next cycle.
> 
> I'll try to get something next week.
> 

In case you are interested in fixing this, a quick reproducer can be
done through virtme-ng:

$> vng --verbose --kconfig --skip-module --config tools/testing/selftests/hid/config
$> make -j8
$> vng -v --user root --exec "mount bpffs -t bpf /sys/fs/bpf ; pytest ./tools/testing/selftests/hid/tests/test_gamepad.py -v -x --color=yes -k Saitek"

Cheers,
Benjamin