Message-ID: <qjas3gu47h65hq4ppoagogte6gxi246x7j4sbnd6bi2axo4abc@scun755iynel>
Date: Sun, 13 Jul 2025 09:44:20 +0200
From: Benjamin Tissoires <bentiss@...nel.org>
To: Jiri Kosina <jikos@...nel.org>, Alan Stern <stern@...land.harvard.edu>, 
	Shuah Khan <shuah@...nel.org>
Cc: linux-input@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v2 4/4] selftests/hid: add a test case for the recent
 syzbot underflow

On Jul 11 2025, Benjamin Tissoires wrote:
> On Jul 10 2025, Benjamin Tissoires wrote:
> > Syzbot found a buffer underflow in __hid_request(). Add a related test
> > case for it.
> > 
> > It's not perfect, but it allows to catch a corner case when a report
> > descriptor is crafted so that it has a size of 0.
> > 
> > Signed-off-by: Benjamin Tissoires <bentiss@...nel.org>
> > ---
> >  tools/testing/selftests/hid/tests/test_mouse.py | 70 +++++++++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/hid/tests/test_mouse.py b/tools/testing/selftests/hid/tests/test_mouse.py
> > index 66daf7e5975ca50f0b065080669d7f6123fb177f..eb4e15a0e53bd5f3c8e0ea02365ff9da7eead93d 100644
> > --- a/tools/testing/selftests/hid/tests/test_mouse.py
> > +++ b/tools/testing/selftests/hid/tests/test_mouse.py
> > @@ -439,6 +439,68 @@ class BadResolutionMultiplierMouse(ResolutionMultiplierMouse):
> >          return 32  # EPIPE
> >  
> >  
> > +class BadReportDescriptorMouse(BaseMouse):
> > +    """
> > +    This "device" was one autogenerated by syzbot. There are a lot of issues in
> > +    it, and the most problematic is that it declares features that have no
> > +    size.
> > +
> > +    This leads to report->size being set to 0 and can mess up with usbhid
> > +    internals.  Fortunately, uhid merely passes the incoming buffer, without
> > +    touching it so a buffer of size 0 will be translated to [] without
> > +    triggering a kernel oops.
> > +
> > +    Because the report descriptor is wrong, no input are created, and we need
> > +    to tweak a little bit the parameters to make it look correct.
> > +    """
> > +
> > +    # fmt: off
> > +    report_descriptor = [
> > +        0x96, 0x01, 0x00,              # Report Count (1)                    0
> > +        0x06, 0x01, 0x00,              # Usage Page (Generic Desktop)        3
> > +        # 0x03, 0x00, 0x00, 0x00, 0x00,  # Ignored by the kernel somehow
> > +        0x2a, 0x90, 0xa0,              # Usage Maximum (41104)               6
> > +        0x27, 0x00, 0x00, 0x00, 0x00,  # Logical Maximum (0)                 9
> > +        0xb3, 0x81, 0x3e, 0x25, 0x03,  # Feature (Cnst,Arr,Abs,Vol)          14
> > +        0x1b, 0xdd, 0xe8, 0x40, 0x50,  # Usage Minimum (1346431197)          19
> > +        0x3b, 0x5d, 0x8c, 0x3d, 0xda,  # Designator Index                    24
> > +    ]
> > +    # fmt: on
> > +
> > +    def __init__(
> > +        self, rdesc=report_descriptor, name=None, input_info=(3, 0x045E, 0x07DA)
> > +    ):
> > +        super().__init__(rdesc, name, input_info)
> > +        self.high_resolution_report_called = False
> > +
> > +    def get_evdev(self, application=None):
> > +        assert self._input_nodes is None
> > +        return (
> > +            "Ok"  # should be a list or None, but both would fail, so abusing the system
> > +        )
> > +
> > +    def next_sync_events(self, application=None):
> > +        # there are no evdev nodes, so no events
> > +        return []
> > +
> > +    def is_ready(self):
> > +        # we wait for the SET_REPORT command to come
> > +        return self.high_resolution_report_called
> > +
> > +    def set_report(self, req, rnum, rtype, data):
> > +        if rtype != self.UHID_FEATURE_REPORT:
> > +            raise InvalidHIDCommunication(f"Unexpected report type: {rtype}")
> > +        if rnum != 0x0:
> > +            raise InvalidHIDCommunication(f"Unexpected report number: {rnum}")
> > +
> > +        if len(data) != 1:
> > +            raise InvalidHIDCommunication(f"Unexpected data: {data}, expected '[0]'")
> 
> For the record, while thinking more about this, I realized that I
> changed the API for uhid with the previous patches.
> 
> *But* after second thoughts, every request to a HID device made through
> hid_hw_request() would see that change, but every request made through
> hid_hw_raw_request() already has the new behaviour. So that means that
> the users are already facing situations where they might have or not the
> first byte being the null report ID when it is 0, so, maybe we are
> making things more straightforward in the end.
> 

Looking into this more:
- uhid is mainly used for BLE devices
- uhid is also used for testing, but I don't see that change a big issue
- for BLE devices, we can check which kernel module is calling
	hid_hw_request()
- and in those modules, we can check which are using a Bluetooth device
- and then we can check if the command is used with a report ID or not.

Doing all the checks above gives me confidence that the only time the
report ID 0 is used is when using the resolution multiplier. I don't
expect a lot of BLE device without report ID to expose a feature with a
high resolution wheel.

But for a more extensive checking:
https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/profiles/input/hog-lib.c#n879
in function set_report(), bluez does the same shift if the report ID is
0 and the given buffer has a size > 0.

So I think this patch will also fix a hypothetical BLE device without
report ID with high resolution wheel.

Therefore, I'm going to merge this series as it is (and include those
blobs in the commit description).

Cheers,
Benjamin

> > +
> > +        self.high_resolution_report_called = True
> > +
> > +        return 0
> > +
> > +
> >  class ResolutionMultiplierHWheelMouse(TwoWheelMouse):
> >      # fmt: off
> >      report_descriptor = [
> > @@ -975,3 +1037,11 @@ class TestMiMouse(TestWheelMouse):
> >              # assert below print out the real error
> >              pass
> >          assert remaining == []
> > +
> > +
> > +class TestBadReportDescriptorMouse(base.BaseTestCase.TestUhid):
> > +    def create_device(self):
> > +        return BadReportDescriptorMouse()
> > +
> > +    def assertName(self, uhdev):
> > +        pass
> > 
> > -- 
> > 2.49.0
> > 

Powered by blists - more mailing lists