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: <lrhydvc7huuqck2kvqzobqt7hhwt36zwsa2i3fpegbpykt5q43@2md6gfitjlb3>
Date: Sat, 19 Jul 2025 10:36:01 +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 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.

Cheers,
Benjamin

> @@ -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;