Re: [PATCH 4/7] hid-asus: Remove more bogus zero bytes from some report descriptors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The trailing zero removal loop reads past the end of the report descriptor buffer and lacks a lower bounds check.
- [High] The `memcpy` operation copies more bytes than are available in the source `rdesc` buffer.
- [High] Trimming trailing bytes can shrink the new buffer size below what is required for subsequent fixed-offset memory operations, causing a heap buffer overflow.
--

commit 1a97afa374b1aa0ec76a035ced7d0e6414476f71
Author: Joshua Leivenzon <hacker1024@users.sourceforge.net>

hid-asus: Remove more bogus zero bytes from some report descriptors

This commit attempts to remove bogus trailing zero bytes from specific
HID report descriptors. It adjusts the new report descriptor size by
stripping zeroes from the end and then allocates a new buffer.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
>  		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;

Since the rdesc buffer contains exactly rsize_orig bytes, does evaluating
rdesc[new_rsize + 1] read two bytes past the end of the allocation?

Additionally, if the adjacent memory happens to contain zeroes, could
new_rsize decrement indefinitely and underflow since there is no lower
bound check?

> +            /* Make room for the added bytes. */
> +			new_rsize += 2;
> +
> +            new_rdesc = devm_kzalloc(&hdev->dev, new_rsize,
>  						 GFP_KERNEL);
>  			if (!new_rdesc)
>  				return rdesc;
[ ... ]
> -			memcpy(new_rdesc, rdesc, rsize_orig);
> -			*rsize = rsize_orig + 1;
> +			memcpy(new_rdesc, rdesc, new_rsize);

If no trailing zeroes were stripped, new_rsize is now rsize_orig + 2. Does
this memcpy copy more bytes than are available in the original rdesc
buffer, leading to an out-of-bounds read?

> +			*rsize = new_rsize;
>  			rdesc = new_rdesc;
>  
>  			memmove(rdesc + offs + 4, rdesc + offs + 2, 12);

If trailing zeroes were successfully removed, could new_rsize drop below the
offsets used in this memmove and subsequent assignments?

For example, if new_rsize shrinks below offs + 15, will the fixed-offset
memmove and subsequent assignments write past the end of the newly allocated
new_rdesc buffer?

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