Re: [PATCH 3/3] HID: magicmouse: report charge status over Bluetooth

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver unconditionally dereferences `msc->input` in `magicmouse_raw_event` and `magicmouse_emit_touch`, leading to a kernel panic if `msc->input` is NULL.
- [High] A malicious device can send a deeply nested sequence of `DOUBLE_REPORT_ID` (0xf7) reports in a single packet, causing `magicmouse_raw_event` to recurse until it overflows the kernel stack.
--

--- Patch [3]: [PATCH 3/3] HID: magicmouse: report charge status over Bluetooth ---
commit 7588bfd842ef79dc73b3904f9e721e709d0dc4fe
Author: Alec Hall <signshop.alec@gmail.com>

HID: magicmouse: report charge status over Bluetooth

This commit adds parsing for the status byte in raw battery input reports
for the Magic Mouse/Trackpad. It extracts external power and charge complete
bits and reports them to user space via power_supply_changed().

> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index 6d0e76314b105..8bb791efef3a8 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c

[ ... ]

> @@ -394,6 +428,9 @@ static int magicmouse_raw_event(struct hid_device *hdev,
>  	if (size < 1)
>  		return 0;
>  
> +	if (data[0] == MAGICMOUSE_BATTERY_REPORT_ID && size >= 3)
> +		magicmouse_report_charge_status(hdev, data[1]);
> +
>  	switch (data[0]) {
>  	case TRACKPAD_REPORT_ID:
>  	case TRACKPAD2_BT_REPORT_ID:

[Severity: High]
This is a pre-existing issue, but can this function trigger a kernel panic
due to a NULL pointer dereference on msc->input?

If a malicious USB device spoofs the Magic Mouse or Trackpad 2 product IDs,
magicmouse_probe() can return early without validating !msc->input:

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) {
        ...
    }
}

When this device later sends a raw HID report, magicmouse_raw_event() will
unconditionally dereference input->id.product at the end of the function:

    if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE || ...

This might also happen if hid_hw_start() enables interrupts and processes
incoming raw events before hid_connect() finishes initializing msc->input.

Could a check for msc->input be added to prevent this?

[Severity: High]
This is also a pre-existing issue, but does this function have a potential
stack overflow vulnerability via unbounded recursion?

If a malicious device sends a nested sequence of DOUBLE_REPORT_ID (0xf7)
reports in a single packet with a size payload of 0, the recursive calls
will consume only 2 bytes of the payload per call:

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;
    ...
}

With a maximum USB/HID packet size of 16KB, could this result in up to
8,192 recursive calls and exhaust the kernel stack?

Is there a way to limit the maximum recursion depth here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714101235.99447-1-signshop.alec@gmail.com?part=3