drivers/hid/hid-multitouch.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)
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.5On 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
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
>
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
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
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
>
© 2016 - 2025 Red Hat, Inc.