Re: [PATCH 7/7] hid-asus: add prod-id, quirk for Zenbook Duo keyboard

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Out-of-bounds read and heap buffer overflow in report descriptor fixup logic.
--

commit ad6b37250b171e419ae8bd990515ef1b0181a049
Author: Luke Jones <luke@ljones.dev>

hid-asus: add prod-id, quirk for Zenbook Duo keyboard

This commit adds product IDs and Bluetooth IDs for the 2024/2025/2026 ASUS
Zenbook Duo keyboards. It enables keyboard control, backlight, and Fn-lock
by applying the appropriate quirks.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 37e3aec612b6d..ba093f11f802f 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -1583,6 +1583,24 @@ static const struct hid_device_id asus_devices[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>  	    USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
>  	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> +	    USB_DEVICE_ID_ASUSTEK_ZENBOOK_DUO_UX8406MA_KEYBOARD),
> +	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_HID_FN_LOCK | QUIRK_ZENBOOK_DUO_KEYBOARD },

This isn't a bug introduced in this patch, but adding these new device IDs with
QUIRK_ZENBOOK_DUO_KEYBOARD exposes them to an existing out-of-bounds read and
heap buffer overflow in asus_report_fixup().

When a device with this quirk connects, the driver attempts to strip trailing
zeros from its report descriptor:

drivers/hid/hid-asus.c:asus_report_fixup() {
    ...
    if (drvdata->quirks & (QUIRK_T100CHI | QUIRK_T90CHI | QUIRK_ZENBOOK_DUO_KEYBOARD)) {
        ...
        if (drvdata->quirks & QUIRK_ZENBOOK_DUO_KEYBOARD) {
            if (hid_is_usb(hdev)) {
                rsize_orig = 90;
                offs = 66;
            ...
        }
        
        if (*rsize == rsize_orig &&
            rdesc[offs] == 0x09 && rdesc[offs + 1] == 0x76) {
            __u8 *new_rdesc;
            unsigned int new_rsize = rsize_orig;

            /* Clear bogus trailing zero bytes. */
            while (rdesc[new_rsize + 1] == 0)
                --new_rsize;
    ...
}

Does this evaluate rdesc[new_rsize + 1] reading 2 bytes past the end of the
dynamically allocated descriptor buffer, since new_rsize is initialized to the
exact size of the descriptor?

Furthermore, if the out-of-bounds bytes are non-zero, new_rsize remains its
original value. The code then copies rsize_orig + 2 bytes from the
rsize_orig-sized buffer:

drivers/hid/hid-asus.c:asus_report_fixup() {
    ...
            new_rsize += 2;
            new_rdesc = devm_kzalloc(&hdev->dev, new_rsize, GFP_KERNEL);
            ...
            memcpy(new_rdesc, rdesc, new_rsize);
    ...
}

Can this result in an unconditional slab-out-of-bounds read?

Finally, if new_rsize shrank below offs + 14 due to trailing zeros, could the
subsequent memmove and assignments write past the end of the newly allocated
buffer, causing a heap buffer overflow?

drivers/hid/hid-asus.c:asus_report_fixup() {
    ...
            memmove(rdesc + offs + 4, rdesc + offs + 2, 12);
            ...
            rdesc[offs + 14] = 0x00;
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513163248.16483-1-p.pisati@gmail.com?part=7