* [PATCH RESEND] HID: multitouch: fix slab out-of-bounds access in mt_report_fixup()
@ 2025-07-22 8:00 Qasim Ijaz
2025-07-22 8:09 ` Dmitry Savin
2025-07-22 9:16 ` Jiri Slaby
0 siblings, 2 replies; 6+ messages in thread
From: Qasim Ijaz @ 2025-07-22 8:00 UTC (permalink / raw)
To: jikos, bentiss; +Cc: linux-input, linux-kernel, stable, Dmitry Savin
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",
+ rdesc[607]);
+ }
} 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",
- rdesc[607]);
+ "GT7868Q fixup: report descriptor only %u bytes, skipping\n",
+ *size);
}
}
--
2.39.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH RESEND] HID: multitouch: fix slab out-of-bounds access in mt_report_fixup() 2025-07-22 8:00 [PATCH RESEND] HID: multitouch: fix slab out-of-bounds access in mt_report_fixup() Qasim Ijaz @ 2025-07-22 8:09 ` Dmitry Savin 2025-07-22 9:16 ` Jiri Slaby 1 sibling, 0 replies; 6+ messages in thread From: Dmitry Savin @ 2025-07-22 8:09 UTC (permalink / raw) To: Qasim Ijaz; +Cc: jikos, bentiss, linux-input, linux-kernel, stable Reviewed-by: Dmitry Savin <envelsavinds@gmail.com> On Tue, 22 Jul 2025 at 09:00, Qasim Ijaz <qasdev00@gmail.com> 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", > + rdesc[607]); > + } > } 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", > - rdesc[607]); > + "GT7868Q fixup: report descriptor only %u bytes, skipping\n", > + *size); > } > } > > -- > 2.39.5 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND] HID: multitouch: fix slab out-of-bounds access in mt_report_fixup() 2025-07-22 8:00 [PATCH RESEND] HID: multitouch: fix slab out-of-bounds access in mt_report_fixup() Qasim Ijaz 2025-07-22 8:09 ` Dmitry Savin @ 2025-07-22 9:16 ` Jiri Slaby 2025-07-22 11:19 ` Qasim Ijaz 1 sibling, 1 reply; 6+ messages in thread From: Jiri Slaby @ 2025-07-22 9:16 UTC (permalink / raw) To: Qasim Ijaz, jikos, bentiss Cc: linux-input, linux-kernel, stable, Dmitry Savin 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". > + *size); > } > } > thanks, -- js suse labs ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND] HID: multitouch: fix slab out-of-bounds access in mt_report_fixup() 2025-07-22 9:16 ` Jiri Slaby @ 2025-07-22 11:19 ` Qasim Ijaz 2025-07-23 4:40 ` Jiri Slaby 0 siblings, 1 reply; 6+ messages in thread From: Qasim Ijaz @ 2025-07-22 11:19 UTC (permalink / raw) To: Jiri Slaby Cc: jikos, bentiss, linux-input, linux-kernel, stable, Dmitry Savin 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND] HID: multitouch: fix slab out-of-bounds access in mt_report_fixup() 2025-07-22 11:19 ` Qasim Ijaz @ 2025-07-23 4:40 ` Jiri Slaby 2025-07-23 10:24 ` Qasim Ijaz 0 siblings, 1 reply; 6+ messages in thread From: Jiri Slaby @ 2025-07-23 4:40 UTC (permalink / raw) To: Qasim Ijaz Cc: jikos, bentiss, linux-input, linux-kernel, stable, Dmitry Savin On 22. 07. 25, 13:19, Qasim Ijaz wrote: > 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? Definitely. > 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. I would invert the conditions. So the code would look like: static bool goodix_needs_fixup(hdev) { if (hdev->vendor != I2C_VENDOR_ID_GOODIX) return false; return hdev->product == I2C_DEVICE_ID_GOODIX_01E8 || hdev->product == I2C_DEVICE_ID_GOODIX_01E9; } static const __u8 *mt_report_fixup(struct hid_device *hdev, __u8 *rdesc, unsigned int *size) { if (!goodix_needs_fixup(hdev)) return rdesc; if (*size < 608) { dev_info(&hdev->dev, "GT7868Q fixup: report descriptor is only %u bytes, skipping\n", *size); return rdesc; } if (rdesc[607] != 0x15) { dev_info(&hdev->dev, "GT7868Q fixup: offset 607 is %x (expected 0x15), descriptor may be malformed, skipping\n", rdesc[607]); return rdesc; } rdesc[607] = 0x25; dev_info(&hdev->dev, "GT7868Q fixup: report descriptor fixup is applied.\n"); return rdesc; } thanks, -- js suse labs ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND] HID: multitouch: fix slab out-of-bounds access in mt_report_fixup() 2025-07-23 4:40 ` Jiri Slaby @ 2025-07-23 10:24 ` Qasim Ijaz 0 siblings, 0 replies; 6+ messages in thread From: Qasim Ijaz @ 2025-07-23 10:24 UTC (permalink / raw) To: Jiri Slaby Cc: jikos, bentiss, linux-input, linux-kernel, stable, Dmitry Savin On Wed, Jul 23, 2025 at 06:40:52AM +0200, Jiri Slaby wrote: > On 22. 07. 25, 13:19, Qasim Ijaz wrote: > > 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? > > Definitely. > > > 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. > > I would invert the conditions. So the code would look like: > > static bool goodix_needs_fixup(hdev) > { > if (hdev->vendor != I2C_VENDOR_ID_GOODIX) > return false; > > return hdev->product == I2C_DEVICE_ID_GOODIX_01E8 || > hdev->product == I2C_DEVICE_ID_GOODIX_01E9; > } > > static const __u8 *mt_report_fixup(struct hid_device *hdev, __u8 *rdesc, > unsigned int *size) > { > if (!goodix_needs_fixup(hdev)) > return rdesc; > > if (*size < 608) { > dev_info(&hdev->dev, > "GT7868Q fixup: report descriptor is only %u bytes, > skipping\n", > *size); > return rdesc; > } > > if (rdesc[607] != 0x15) { > dev_info(&hdev->dev, > "GT7868Q fixup: offset 607 is %x (expected 0x15), descriptor may be > malformed, skipping\n", > rdesc[607]); > return rdesc; > } > > rdesc[607] = 0x25; > dev_info(&hdev->dev, > "GT7868Q fixup: report descriptor fixup is applied.\n"); > > return rdesc; > } Thanks Jiri, this looks good. I think Ill split this into 2 patches one for fixing the OOB with a size check and the second for a general function cleanup. Hope that sounds good. Thanks qasim > > thanks, > -- > js > suse labs ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-07-23 10:24 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-07-22 8:00 [PATCH RESEND] HID: multitouch: fix slab out-of-bounds access in mt_report_fixup() Qasim Ijaz 2025-07-22 8:09 ` Dmitry Savin 2025-07-22 9:16 ` Jiri Slaby 2025-07-22 11:19 ` Qasim Ijaz 2025-07-23 4:40 ` Jiri Slaby 2025-07-23 10:24 ` Qasim Ijaz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).