drivers/hid/hid-core.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
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@rowland.harvard.edu>
Reported-by: syzbot+b63d677d63bcac06cf90@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/linux-usb/68753a08.050a0220.33d347.0008.GAE@google.com/
Tested-by: syzbot+b63d677d63bcac06cf90@syzkaller.appspotmail.com
Fixes: dde5845a529f ("[PATCH] Generic HID layer - code split")
Cc: stable@vger.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;
@@ -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;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@rowland.harvard.edu>
> Reported-by: syzbot+b63d677d63bcac06cf90@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/linux-usb/68753a08.050a0220.33d347.0008.GAE@google.com/
> Tested-by: syzbot+b63d677d63bcac06cf90@syzkaller.appspotmail.com
> Fixes: dde5845a529f ("[PATCH] Generic HID layer - code split")
> Cc: stable@vger.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;
On Sat, Jul 19, 2025 at 10:36:01AM +0200, 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@rowland.harvard.edu>
> > Reported-by: syzbot+b63d677d63bcac06cf90@syzkaller.appspotmail.com
> > Closes: https://lore.kernel.org/linux-usb/68753a08.050a0220.33d347.0008.GAE@google.com/
> > Tested-by: syzbot+b63d677d63bcac06cf90@syzkaller.appspotmail.com
> > Fixes: dde5845a529f ("[PATCH] Generic HID layer - code split")
> > Cc: stable@vger.kernel.org
> 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.
So then would it be better to accept these report fields (perhaps with a
warning) and instead, harden the core HID code so that it doesn't choke
when it runs across one of them?
Alan Stern
On Jul 19 2025, Alan Stern wrote:
> On Sat, Jul 19, 2025 at 10:36:01AM +0200, 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@rowland.harvard.edu>
> > > Reported-by: syzbot+b63d677d63bcac06cf90@syzkaller.appspotmail.com
> > > Closes: https://lore.kernel.org/linux-usb/68753a08.050a0220.33d347.0008.GAE@google.com/
> > > Tested-by: syzbot+b63d677d63bcac06cf90@syzkaller.appspotmail.com
> > > Fixes: dde5845a529f ("[PATCH] Generic HID layer - code split")
> > > Cc: stable@vger.kernel.org
>
> > 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.
>
> So then would it be better to accept these report fields (perhaps with a
> warning) and instead, harden the core HID code so that it doesn't choke
> when it runs across one of them?
>
Yeah, that seems like the best plan forward.
[sorry on reduced setup for the next 3 weeks, so I can't really debug
the entire thing now.]
Though, we should probably not annoy users unless we are trying to do
something that won't be needed. I doubt that Saitek gamepad will ever
call the faulty functions, so why putting an error in the logs when it's
working fine?
Cheers,
Benjamin
On Mon, Jul 21, 2025 at 03:05:58PM +0200, Benjamin Tissoires wrote: > > So then would it be better to accept these report fields (perhaps with a > > warning) and instead, harden the core HID code so that it doesn't choke > > when it runs across one of them? > > > > Yeah, that seems like the best plan forward. > > [sorry on reduced setup for the next 3 weeks, so I can't really debug > the entire thing now.] > > Though, we should probably not annoy users unless we are trying to do > something that won't be needed. I doubt that Saitek gamepad will ever > call the faulty functions, so why putting an error in the logs when it's > working fine? All right. Probably the best way to do this is simply to revert the commit that's already applied and then merge a new patch to harden the core. Would you like me to post the reversion patch or do you prefer to do it yourself? Alan Stern
On Jul 21 2025, Alan Stern wrote: > On Mon, Jul 21, 2025 at 03:05:58PM +0200, Benjamin Tissoires wrote: > > > So then would it be better to accept these report fields (perhaps with a > > > warning) and instead, harden the core HID code so that it doesn't choke > > > when it runs across one of them? > > > > > > > Yeah, that seems like the best plan forward. > > > > [sorry on reduced setup for the next 3 weeks, so I can't really debug > > the entire thing now.] > > > > Though, we should probably not annoy users unless we are trying to do > > something that won't be needed. I doubt that Saitek gamepad will ever > > call the faulty functions, so why putting an error in the logs when it's > > working fine? > > All right. > > Probably the best way to do this is simply to revert the commit that's > already applied and then merge a new patch to harden the core. Would > you like me to post the reversion patch or do you prefer to do it > yourself? Given that the faulty commit is on top of for-6.17/core, I can simply force push to the parent, and also force push the for-next branch. That should do the trick. Can you post your s32ton fix on top of that then? Cheers, Benjamin
On Wed, Jul 23, 2025 at 11:32:14AM +0200, Benjamin Tissoires wrote: > On Jul 21 2025, Alan Stern wrote: > > On Mon, Jul 21, 2025 at 03:05:58PM +0200, Benjamin Tissoires wrote: > > > > So then would it be better to accept these report fields (perhaps with a > > > > warning) and instead, harden the core HID code so that it doesn't choke > > > > when it runs across one of them? > > > > > > > > > > Yeah, that seems like the best plan forward. > > > > > > [sorry on reduced setup for the next 3 weeks, so I can't really debug > > > the entire thing now.] > > > > > > Though, we should probably not annoy users unless we are trying to do > > > something that won't be needed. I doubt that Saitek gamepad will ever > > > call the faulty functions, so why putting an error in the logs when it's > > > working fine? > > > > All right. > > > > Probably the best way to do this is simply to revert the commit that's > > already applied and then merge a new patch to harden the core. Would > > you like me to post the reversion patch or do you prefer to do it > > yourself? > > Given that the faulty commit is on top of for-6.17/core, I can simply > force push to the parent, and also force push the for-next branch. That > should do the trick. > > Can you post your s32ton fix on top of that then? Sure. Patch coming up shortly... Alan Stern
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. Ideally this should never occur, but
there are buggy devices and some might have a report field with size
set to zero; we shouldn't reject the report or the device just because
of that.
Instead, harden the s32ton() routine so that it returns a reasonable
result instead of crashing when it is called with the number of bits
set to 0 -- the same as what snto32() does.
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Reported-by: syzbot+b63d677d63bcac06cf90@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/linux-usb/68753a08.050a0220.33d347.0008.GAE@google.com/
Tested-by: syzbot+b63d677d63bcac06cf90@syzkaller.appspotmail.com
Fixes: dde5845a529f ("[PATCH] Generic HID layer - code split")
Cc: stable@vger.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 | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
Index: usb-devel/drivers/hid/hid-core.c
===================================================================
--- usb-devel.orig/drivers/hid/hid-core.c
+++ usb-devel/drivers/hid/hid-core.c
@@ -66,8 +66,12 @@ static s32 snto32(__u32 value, unsigned
static u32 s32ton(__s32 value, unsigned int n)
{
- s32 a = value >> (n - 1);
+ s32 a;
+ if (!value || !n)
+ return 0;
+
+ a = value >> (n - 1);
if (a && a != -1)
return value < 0 ? 1 << (n - 1) : (1 << (n - 1)) - 1;
return value & ((1 << n) - 1);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@rowland.harvard.edu>
> > Reported-by: syzbot+b63d677d63bcac06cf90@syzkaller.appspotmail.com
> > Closes: https://lore.kernel.org/linux-usb/68753a08.050a0220.33d347.0008.GAE@google.com/
> > Tested-by: syzbot+b63d677d63bcac06cf90@syzkaller.appspotmail.com
> > Fixes: dde5845a529f ("[PATCH] Generic HID layer - code split")
> > Cc: stable@vger.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
© 2016 - 2025 Red Hat, Inc.