From 04515e08bca9b5a1f9729b8d99d8b322e56d7454 Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Thu, 10 Jul 2025 12:41:20 +0300 Subject: [PATCH 1/5] HID: debug: Remove duplicate entry (BTN_WHEEL) BTN_WHEEL is duplicated (by value) and compiler is not happy about that: drivers/hid/hid-debug.c:3302:16: error: initializer overrides prior initialization of this subobject [-Werror,-Winitializer-overrides] 3302 | [BTN_WHEEL] = "BtnWheel", [KEY_OK] = "Ok", | ^~~~~~~~~~ drivers/hid/hid-debug.c:3301:20: note: previous initialization is here 3301 | [BTN_GEAR_DOWN] = "BtnGearDown", [BTN_GEAR_UP] = "BtnGearUp", | ^~~~~~~~~~~~~ Remove it again, as the commit 7b2daa648eb7 ("HID: debug: Remove duplicates from 'keys'") already did this once in the past. Fixes: 194808a1ea39 ("HID: Fix debug name for BTN_GEAR_DOWN, BTN_GEAR_UP, BTN_WHEEL") Signed-off-by: Andy Shevchenko Link: https://patch.msgid.link/20250710094120.753358-1-andriy.shevchenko@linux.intel.com Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-debug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c index c6b6b1029540..4424c0512bae 100644 --- a/drivers/hid/hid-debug.c +++ b/drivers/hid/hid-debug.c @@ -3299,7 +3299,7 @@ static const char *keys[KEY_MAX + 1] = { [BTN_STYLUS2] = "Stylus2", [BTN_TOOL_DOUBLETAP] = "ToolDoubleTap", [BTN_TOOL_TRIPLETAP] = "ToolTripleTap", [BTN_TOOL_QUADTAP] = "ToolQuadrupleTap", [BTN_GEAR_DOWN] = "BtnGearDown", [BTN_GEAR_UP] = "BtnGearUp", - [BTN_WHEEL] = "BtnWheel", [KEY_OK] = "Ok", + [KEY_OK] = "Ok", [KEY_SELECT] = "Select", [KEY_GOTO] = "Goto", [KEY_CLEAR] = "Clear", [KEY_POWER2] = "Power2", [KEY_OPTION] = "Option", [KEY_INFO] = "Info", From 4f15ee98304b96e164ff2340e1dfd6181c3f42aa Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Thu, 10 Jul 2025 16:01:33 +0200 Subject: [PATCH 2/5] HID: core: ensure the allocated report buffer can contain the reserved report ID When the report ID is not used, the low level transport drivers expect the first byte to be 0. However, currently the allocated buffer not account for that extra byte, meaning that instead of having 8 guaranteed bytes for implement to be working, we only have 7. Reported-by: Alan Stern Closes: https://lore.kernel.org/linux-input/c75433e0-9b47-4072-bbe8-b1d14ea97b13@rowland.harvard.edu/ Cc: stable@vger.kernel.org Suggested-by: Alan Stern Link: https://patch.msgid.link/20250710-report-size-null-v2-1-ccf922b7c4e5@kernel.org Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-core.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index b348d0464314..1a231dd9e4bc 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -1883,9 +1883,12 @@ u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags) /* * 7 extra bytes are necessary to achieve proper functionality * of implement() working on 8 byte chunks + * 1 extra byte for the report ID if it is null (not used) so + * we can reserve that extra byte in the first position of the buffer + * when sending it to .raw_request() */ - u32 len = hid_report_len(report) + 7; + u32 len = hid_report_len(report) + 7 + (report->id == 0); return kzalloc(len, flags); } From 0d0777ccaa2d46609d05b66ba0096802a2746193 Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Thu, 10 Jul 2025 16:01:34 +0200 Subject: [PATCH 3/5] HID: core: ensure __hid_request reserves the report ID as the first byte The low level transport driver expects the first byte to be the report ID, even when the report ID is not use (in which case they just shift the buffer). However, __hid_request() whas not offsetting the buffer it used by one in this case, meaning that the raw_request() callback emitted by the transport driver would be stripped of the first byte. Note: this changes the API for uhid devices when a request is made through hid_hw_request. However, several considerations makes me think this is fine: - 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. We are making things more straightforward in the end. - 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. - surprise: none of the kernel module are using a report ID 0 - and finally, bluez, in its function set_report()[0], does the same shift if the report ID is 0 and the given buffer has a size > 0. [0] https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/profiles/input/hog-lib.c#n879 Reported-by: Alan Stern Closes: https://lore.kernel.org/linux-input/c75433e0-9b47-4072-bbe8-b1d14ea97b13@rowland.harvard.edu/ Reported-by: syzbot+8258d5439c49d4c35f43@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=8258d5439c49d4c35f43 Tested-by: syzbot+8258d5439c49d4c35f43@syzkaller.appspotmail.com Fixes: 4fa5a7f76cc7 ("HID: core: implement generic .request()") Cc: stable@vger.kernel.org Link: https://patch.msgid.link/20250710-report-size-null-v2-2-ccf922b7c4e5@kernel.org Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-core.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 1a231dd9e4bc..320887c365f7 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -1976,7 +1976,7 @@ static struct hid_report *hid_get_report(struct hid_report_enum *report_enum, int __hid_request(struct hid_device *hid, struct hid_report *report, enum hid_class_request reqtype) { - char *buf; + char *buf, *data_buf; int ret; u32 len; @@ -1984,10 +1984,17 @@ int __hid_request(struct hid_device *hid, struct hid_report *report, if (!buf) return -ENOMEM; + data_buf = buf; len = hid_report_len(report); + if (report->id == 0) { + /* reserve the first byte for the report ID */ + data_buf++; + len++; + } + if (reqtype == HID_REQ_SET_REPORT) - hid_output_report(report, buf); + hid_output_report(report, data_buf); ret = hid->ll_driver->raw_request(hid, report->id, buf, len, report->type, reqtype); From c2ca42f190b6714d6c481dfd3d9b62ea091c946b Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Thu, 10 Jul 2025 16:01:35 +0200 Subject: [PATCH 4/5] HID: core: do not bypass hid_hw_raw_request hid_hw_raw_request() is actually useful to ensure the provided buffer and length are valid. Directly calling in the low level transport driver function bypassed those checks and allowed invalid paramto be used. Reported-by: Alan Stern Closes: https://lore.kernel.org/linux-input/c75433e0-9b47-4072-bbe8-b1d14ea97b13@rowland.harvard.edu/ Cc: stable@vger.kernel.org Link: https://patch.msgid.link/20250710-report-size-null-v2-3-ccf922b7c4e5@kernel.org Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-core.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 320887c365f7..b31b8a2fd540 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -1996,8 +1996,7 @@ int __hid_request(struct hid_device *hid, struct hid_report *report, if (reqtype == HID_REQ_SET_REPORT) hid_output_report(report, data_buf); - ret = hid->ll_driver->raw_request(hid, report->id, buf, len, - report->type, reqtype); + ret = hid_hw_raw_request(hid, report->id, buf, len, report->type, reqtype); if (ret < 0) { dbg_hid("unable to complete request: %d\n", ret); goto out; From 3a1d22bd85381c4e358fc3340e776c3a3223a1d0 Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Thu, 10 Jul 2025 16:01:36 +0200 Subject: [PATCH 5/5] selftests/hid: add a test case for the recent syzbot underflow 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. Link: https://patch.msgid.link/20250710-report-size-null-v2-4-ccf922b7c4e5@kernel.org Signed-off-by: Benjamin Tissoires --- .../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 66daf7e5975c..eb4e15a0e53b 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]'") + + 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