On Tue, Jul 22, 2025 at 11:16:15AM +0200, Jiri Slaby wrote:
On 22. 07. 25, 10:00, Qasim Ijaz wrote:
A malicious HID device can trigger a slab out-of-bounds during mt_report_fixup() by passing in report descriptor smaller than 607 bytes. mt_report_fixup() attempts to patch byte offset 607 of the descriptor with 0x25 by first checking if byte offset 607 is 0x15 however it lacks bounds checks to verify if the descriptor is big enough before conducting this check. Fix this vulnerability by ensuring the descriptor size is greater than or equal to 608 before accessing it.
Below is the KASAN splat after the out of bounds access happens:
[ 13.671954] ================================================================== [ 13.672667] BUG: KASAN: slab-out-of-bounds in mt_report_fixup+0x103/0x110 [ 13.673297] Read of size 1 at addr ffff888103df39df by task kworker/0:1/10 [ 13.673297] [ 13.673297] CPU: 0 UID: 0 PID: 10 Comm: kworker/0:1 Not tainted 6.15.0-00005-gec5d573d83f4-dirty #3 [ 13.673297] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/04 [ 13.673297] Call Trace: [ 13.673297] <TASK> [ 13.673297] dump_stack_lvl+0x5f/0x80 [ 13.673297] print_report+0xd1/0x660 [ 13.673297] kasan_report+0xe5/0x120 [ 13.673297] __asan_report_load1_noabort+0x18/0x20 [ 13.673297] mt_report_fixup+0x103/0x110 [ 13.673297] hid_open_report+0x1ef/0x810 [ 13.673297] mt_probe+0x422/0x960 [ 13.673297] hid_device_probe+0x2e2/0x6f0 [ 13.673297] really_probe+0x1c6/0x6b0 [ 13.673297] __driver_probe_device+0x24f/0x310 [ 13.673297] driver_probe_device+0x4e/0x220 [ 13.673297] __device_attach_driver+0x169/0x320 [ 13.673297] bus_for_each_drv+0x11d/0x1b0 [ 13.673297] __device_attach+0x1b8/0x3e0 [ 13.673297] device_initial_probe+0x12/0x20 [ 13.673297] bus_probe_device+0x13d/0x180 [ 13.673297] device_add+0xe3a/0x1670 [ 13.673297] hid_add_device+0x31d/0xa40 [...]
Fixes: c8000deb6836 ("HID: multitouch: Add support for GT7868Q") Cc: stable@vger.kernel.org Signed-off-by: Qasim Ijaz qasdev00@gmail.com Reviewed-by: Dmitry Savin envelsavinds@gmail.com
drivers/hid/hid-multitouch.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 7ac8e16e6158..af4abe3ba410 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -1461,18 +1461,25 @@ static const __u8 *mt_report_fixup(struct hid_device *hdev, __u8 *rdesc, if (hdev->vendor == I2C_VENDOR_ID_GOODIX && (hdev->product == I2C_DEVICE_ID_GOODIX_01E8 || hdev->product == I2C_DEVICE_ID_GOODIX_01E9)) {
if (rdesc[607] == 0x15) {
rdesc[607] = 0x25;
dev_info(
&hdev->dev,
"GT7868Q report descriptor fixup is applied.\n");
if (*size >= 608) {
if (rdesc[607] == 0x15) {
rdesc[607] = 0x25;
dev_info(
&hdev->dev,
"GT7868Q report descriptor fixup is applied.\n");
} else {
dev_info(
&hdev->dev,
"The byte is not expected for fixing the report descriptor. \
It's possible that the touchpad firmware is not suitable for applying the fix. \
got: %x\n",
This is wrong. You have all the spaces/tabs in the string now. Drop all the backslashes, and open and close the string on every line.
rdesc[607]);
}
As this is superlong and superindented, perhaps introduce a new function for these devices?
} else { dev_info( &hdev->dev,
"The byte is not expected for fixing the report descriptor. \
-It's possible that the touchpad firmware is not suitable for applying the fix. \ -got: %x\n",
This was horrid too, yeah.
rdesc[607]);
"GT7868Q fixup: report descriptor only %u bytes, skipping\n",
A predicate missing. Eg. "has only", or "is only".
Thanks for the feedback Jiri, I took the advice on board, is something like this better?
static const __u8 *mt_report_fixup(struct hid_device *hdev, __u8 *rdesc, unsigned int *size) { if (hdev->vendor == I2C_VENDOR_ID_GOODIX && (hdev->product == I2C_DEVICE_ID_GOODIX_01E8 || hdev->product == I2C_DEVICE_ID_GOODIX_01E9)) { if (*size < 608) { dev_info( &hdev->dev, "GT7868Q fixup: report descriptor is only %u bytes, skipping\n", *size); return rdesc; }
if (rdesc[607] == 0x15) { rdesc[607] = 0x25; dev_info( &hdev->dev, "GT7868Q fixup: report descriptor fixup is applied.\n"); } else { dev_info(&hdev->dev, "GT7868Q fixup: offset 607 is %x (expected 0x15), " "descriptor may be malformed, skipping\n", rdesc[607]); } }
return rdesc; }
the key changes I made are:
- Move size check to the top, this way the indentation level is decent - get rid of message backslashes - shorten the fixup failure message when rdesc[607] is not 0x15 and make it a bit clearer since this message was the longest - just a minor cleanup - added "is only %u bytes" as you suggested
if this is all good I can send v2.
Thanks qasim
} }*size);
thanks,
js suse labs