Re: [PATCH AUTOSEL 7.0-5.10] HID: magicmouse: Prevent out-of-bounds (OOB) read during DOUBLE_REPORT_ID

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] The newly added warning for mismatched report lengths is not rate-limited, allowing an attacker to continuously spam the kernel log.
- [Low] Missing trailing newline in the newly added `hid_warn` format string.

Pre-existing issues:
- [Critical] A NULL pointer dereference can occur if a raw event is received before the device is fully initialized or if initialization fails to create an input device.
- [High] A malicious USB device can cause a kernel stack overflow by sending a crafted HID report with deeply nested DOUBLE_REPORT_ID reports.
--

commit 722425cdf7389e2fa0f87560f3507f1c80daa8b7
Author: Lee Jones <lee@kernel.org>

HID: magicmouse: Prevent out-of-bounds (OOB) read during DOUBLE_REPORT_ID

This commit adds checks to prevent out-of-bounds reads in the magicmouse
driver when processing DOUBLE_REPORT_ID reports. It verifies the report
size against the specified length and adds protection against zero-sized
recursive calls.

> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index 9eadf3252d0dc..f53839484c861 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -390,6 +390,10 @@ static int magicmouse_raw_event(struct hid_device *hdev,
>  	struct input_dev *input = msc->input;

[Severity: Critical]
This is a pre-existing issue, but can this lead to a NULL pointer
dereference?

If a device provides a crafted report descriptor without any input fields,
hidinput_connect() will not call .input_mapping, leaving msc->input as NULL.
During magicmouse_probe(), for some devices the probe returns 0 early without
verifying msc->input. If raw IN reports are then streamed, this function will
blindly dereference the NULL input pointer later when evaluating
input->id.product.

>  	int x = 0, y = 0, ii, clicks = 0, npoints;
>  
> +	/* Protect against zero sized recursive calls from DOUBLE_REPORT_ID */
> +	if (size < 1)
> +		return 0;
> +

[ ... ]

> @@ -490,6 +494,18 @@ static int magicmouse_raw_event(struct hid_device *hdev,
>  		/* Sometimes the trackpad sends two touch reports in one
>  		 * packet.
>  		 */
> +
> +		/* Ensure that we have at least 2 elements (report type and size) */
> +		if (size < 2)
> +			return 0;
> +
> +		if (size < data[1] + 2) {
> +			hid_warn(hdev,
> +				 "received report length (%d) was smaller than specified (%d)",

[Severity: Low]
Does this log message need a trailing newline? It looks like the missing
newline will prevent the buffer from flushing immediately, causing subsequent
log messages to append to this line.

[Severity: Medium]
Could this warning be abused to spam the kernel log? A malicious or
misconfigured device could rapidly stream malformed reports, flooding dmesg.
Would it be safer to use hid_warn_ratelimited() here?

> +				 size, data[1] + 2);
> +			return 0;
> +		}
> +
>  		magicmouse_raw_event(hdev, report, data + 2, data[1]);
>  		magicmouse_raw_event(hdev, report, data + 2 + data[1],
>  			size - 2 - data[1]);

[Severity: High]
This is a pre-existing issue, but does this recursion safely handle a payload
entirely filled with DOUBLE_REPORT_ID markers and data[1] set to 0?

Because size is only decremented by 2 in each recursive step:

    magicmouse_raw_event(..., size - 2 - data[1]);

A large report of 4096 bytes could trigger up to 2048 nested calls. Since
tail-call optimization is not guaranteed in the kernel, couldn't this exhaust
the kernel stack and trigger a panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520111944.3424570-9-sashal@kernel.org?part=1