Re: [PATCH v2] HID: magicmouse: avoid NULL pointer deref when there is no input device

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

Pre-existing issues:
- [Critical] Unbounded recursion in `magicmouse_raw_event()` when processing `DOUBLE_REPORT_ID` leads to kernel stack overflow.
- [High] Dangling `msc->input` pointer leading to Use-After-Free if `input_register_device()` fails.
--

--- Patch [1]: [PATCH v2] HID: magicmouse: avoid NULL pointer deref when there is no input device ---
commit 43afb4a20d87a9959f50f2b092097b19eab03cdd
Author: Jose Villaseñor Montfort <[email protected]>

HID: magicmouse: avoid NULL pointer deref when there is no input device

This patch guards against NULL pointer dereferences in the raw_event and event
callbacks for the magicmouse driver. It bails out if the input device is NULL,
which is a legitimate state for some hardware interfaces that do not register
an input device but still bind to the driver.

Link: https://lore.kernel.org/linux-input/[email protected]/
Link: https://lore.kernel.org/linux-input/[email protected]/

> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index 802a3479e24b9..7164ced590764 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -390,6 +390,14 @@ static int magicmouse_raw_event(struct hid_device *hdev,
>  	struct input_dev *input = msc->input;
>  	int x = 0, y = 0, ii, clicks = 0, npoints;
>  
> +	/*
> +	 * This driver also binds interfaces that hid-input does not create an
> +	 * input device for, so msc->input can legitimately be NULL here. Leave
> +	 * their reports to the generic HID paths instead of dereferencing it.
> +	 */
> +	if (!input)
> +		return 0;
> +

[Severity: High]
This isn't a bug introduced by this patch, but does the !input check miss
dangling pointers if input_register_device() fails?

During device initialization, hid_hw_start() calls hidinput_connect(). This
function allocates an input_dev and invokes the driver's input_mapping
callback, which caches the pointer:

drivers/hid/hid-magicmouse.c:magicmouse_input_mapping() {
    ...
    if (!msc->input)
        msc->input = hi->input;
    ...
}

If input_register_device() subsequently fails, the HID core frees the
input_dev but does not notify the driver, leaving msc->input dangling.

magicmouse_probe() can then succeed without realizing the input device was
freed, as it either returns early or evaluates the dangling pointer as true:

drivers/hid/hid-magicmouse.c:magicmouse_probe() {
    ...
    if (is_usb_magicmouse2(id->vendor, id->product) ||
        (is_usb_magictrackpad2(id->vendor, id->product) &&
         hdev->type != HID_TYPE_USBMOUSE))
        return 0;

    if (!msc->input) {
        hid_err(hdev, "magicmouse input not registered\n");
    ...
}

When a raw HID report arrives, would this freed pointer bypass the !input
guard and be dereferenced?

>  	/* Protect against zero sized recursive calls from DOUBLE_REPORT_ID */
>  	if (size < 1)
>  		return 0;

[Severity: Critical]
This is a pre-existing issue, but does this function have an unbounded
recursion bug when processing a DOUBLE_REPORT_ID?

If data[1] is 0, the first recursive call returns immediately:

drivers/hid/hid-magicmouse.c:magicmouse_raw_event() {
    ...
    case DOUBLE_REPORT_ID:
        ...
        magicmouse_raw_event(hdev, report, data + 2, data[1]);
        magicmouse_raw_event(hdev, report, data + 2 + data[1],
            size - 2 - data[1]);
        return 0;
    ...
}

However, the second call will process the remainder of the buffer, reducing
the size by only 2 bytes per iteration. With a maximum-sized HID report, could
this cause up to ~8192 recursive calls, exhausting the standard 16KB kernel
stack and leading to a kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1