Linux Input/HID development
 help / color / mirror / Atom feed
* [PATCH] HID: magicmouse: reject devices that bind without an input device
@ 2026-07-15 19:58 Jose Villaseñor Montfort
  2026-07-28  7:25 ` Alec Hall
  2026-07-28 18:40 ` [PATCH v2] HID: magicmouse: avoid NULL pointer deref when there is no " Jose Villaseñor Montfort
  0 siblings, 2 replies; 5+ messages in thread
From: Jose Villaseñor Montfort @ 2026-07-15 19:58 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires
  Cc: linux-input, linux-kernel, Jose Villaseñor Montfort

magicmouse_raw_event() and magicmouse_event() dereference msc->input
(e.g. input->id.product, and via magicmouse_emit_touch() and
magicmouse_emit_buttons()) without checking it for NULL. hid-input only
sets msc->input when the device exposes a usable input device.

magicmouse_probe() guards against this with an "input not registered"
check that fails the probe when msc->input is NULL -- but the USB Magic
Mouse 2 / Magic Trackpad 2 path returns 0 before reaching that check.
A device that binds this driver on that path (for example a malicious
one spoofing an Apple VID/PID) with a report descriptor that does not
produce an input device therefore ends up bound with msc->input == NULL.
A subsequent input report then dereferences the NULL pointer in the
->raw_event / ->event callbacks and panics the kernel.

Move the msc->input check ahead of the early return so it covers every
bind path. Legitimate devices register an input during hid_hw_start()
and are unaffected.

Fixes: 0b91b4e4dae6 ("HID: magicmouse: Report battery level over USB")
Link: https://lore.kernel.org/linux-input/20260714102540.3EB2E1F000E9@smtp.kernel.org/
Cc: stable@vger.kernel.org
Signed-off-by: Jose Villaseñor Montfort <pepemontfort@gmail.com>
---
Surfaced by an automated review of Alec Hall's parallel battery series
(the Link: above), independent of that work. This is a sibling hardening
fix to "HID: magicmouse: prevent unbounded recursion in
magicmouse_raw_event()" [1], which touches the same driver.

I went with fixing the probe path (rejecting a bind without an input)
rather than adding per-callback "if (!msc->input) return 0;" guards,
since a single check at probe covers both ->raw_event and ->event and
addresses the root asymmetry. Happy to switch to per-callback guards if
reviewers prefer that.

[1] https://lore.kernel.org/linux-input/20260715053526.574725-1-pepemontfort@gmail.com/

 drivers/hid/hid-magicmouse.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 97562765a..bd6a12e40 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -923,17 +923,25 @@ static int magicmouse_probe(struct hid_device *hdev,
 		magicmouse_fetch_battery(hdev);
 	}
 
-	if (is_usb_magicmouse2(id->vendor, id->product) ||
-	    (is_usb_magictrackpad2(id->vendor, id->product) &&
-	     hdev->type != HID_TYPE_USBMOUSE))
-		return 0;
-
+	/*
+	 * The ->raw_event and ->event callbacks dereference msc->input, which
+	 * hid-input only populates when the device exposes a usable input.
+	 * Reject a device that bound without one -- including on the USB Magic
+	 * Mouse 2 / Trackpad 2 path that returns early below -- so a device
+	 * (e.g. one spoofing an Apple VID/PID) cannot drive those callbacks
+	 * into a NULL pointer dereference.
+	 */
 	if (!msc->input) {
 		hid_err(hdev, "magicmouse input not registered\n");
 		ret = -ENOMEM;
 		goto err_stop_hw;
 	}
 
+	if (is_usb_magicmouse2(id->vendor, id->product) ||
+	    (is_usb_magictrackpad2(id->vendor, id->product) &&
+	     hdev->type != HID_TYPE_USBMOUSE))
+		return 0;
+
 	switch (id->product) {
 	case USB_DEVICE_ID_APPLE_MAGICMOUSE:
 		report = hid_register_report(hdev, HID_INPUT_REPORT, MOUSE_REPORT_ID, 0);
-- 
2.55.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] HID: magicmouse: reject devices that bind without an input device
  2026-07-15 19:58 [PATCH] HID: magicmouse: reject devices that bind without an input device Jose Villaseñor Montfort
@ 2026-07-28  7:25 ` Alec Hall
  2026-07-28 18:37   ` Jose Villaseñor Montfort
  2026-07-28 18:40 ` [PATCH v2] HID: magicmouse: avoid NULL pointer deref when there is no " Jose Villaseñor Montfort
  1 sibling, 1 reply; 5+ messages in thread
From: Alec Hall @ 2026-07-28  7:25 UTC (permalink / raw)
  To: pepemontfort; +Cc: jikos, bentiss, linux-input, linux-kernel

On Wed, Jul 15, 2026, Jose Villaseñor Montfort wrote:
> Move the msc->input check ahead of the early return so it covers every
> bind path. Legitimate devices register an input during hid_hw_start()
> and are unaffected.

The second half of that turns out not to hold on real hardware. A USB
Magic Trackpad 2 exposes four HID interfaces, and only the first two
register an input; interfaces 2 and 3 are vendor-defined, hiddev/hidraw
only. With this patch applied (stacked on the pending battery series,
kernel 7.1.5), plugging in a Magic Trackpad 2 over USB rejects both of
them:

  magicmouse 0003:05AC:0265.001F: magicmouse input not registered
  magicmouse 0003:05AC:0265.0020: magicmouse input not registered

I confirmed with a temporary printk in probe that interfaces 2/3 reach
the moved check with msc->input == NULL and hdev->claimed == 0x6
(hiddev|hidraw, no input), while interfaces 0/1 probe with an input and
claimed == 0x7 and keep working, battery included.

The rejected interfaces then end up bound to nothing -- hid-generic
declines them because a specific driver matches the ID -- so their
hidraw nodes disappear, and every cable plug logs two -ENOMEM probe
failures for a healthy device. Nothing user-facing breaks (touch and
battery live on interfaces 0/1), but the driver has claimed those
interfaces ever since 0b91b4e4dae6, and silently unbinding them with an
error doesn't seem right for a stable-tagged fix.

Bluetooth is unaffected: that path already required an input before the
early return, and a Magic Trackpad 2 + Magic Keyboard over BT show no
change with this patch.

> I went with fixing the probe path (rejecting a bind without an input)
> rather than adding per-callback "if (!msc->input) return 0;" guards,
> since a single check at probe covers both ->raw_event and ->event and
> addresses the root asymmetry. Happy to switch to per-callback guards if
> reviewers prefer that.

Given the above I think the per-callback guard is the better shape after
all: it keeps the legitimate input-less USB interfaces bound exactly as
today, while still closing the NULL deref in ->raw_event/->event -- and
those interfaces demonstrate that a bind with msc->input == NULL is a
state real devices reach, not only spoofed ones. Happy to re-test a v2
on the same hardware over both USB and Bluetooth.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] HID: magicmouse: reject devices that bind without an input device
  2026-07-28  7:25 ` Alec Hall
@ 2026-07-28 18:37   ` Jose Villaseñor Montfort
  0 siblings, 0 replies; 5+ messages in thread
From: Jose Villaseñor Montfort @ 2026-07-28 18:37 UTC (permalink / raw)
  To: Alec Hall
  Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
	Jose Villaseñor Montfort

On Tue, Jul 28, 2026, Alec Hall wrote:
> The second half of that turns out not to hold on real hardware. A USB
> Magic Trackpad 2 exposes four HID interfaces, and only the first two
> register an input; interfaces 2 and 3 are vendor-defined, hiddev/hidraw
> only.

Thanks for putting it on hardware. You're right, and the stable argument
settles it -- unbinding two interfaces of a healthy device is not
something to ship as a fix. v1 is dropped.

I can confirm the same shape on the other model. A USB-C Magic Trackpad
(05ac:0324) on an unpatched 7.1.5 here exposes three HID interfaces
rather than four, all three bound to magicmouse:

  0003:05AC:0324.0006  input=input12  hidraw=hidraw5
  0003:05AC:0324.0007  input=input13  hidraw=hidraw6
  0003:05AC:0324.0008  input=<none>   hidraw=hidraw7

Interface 2 is the vendor-defined one (bInterfaceSubClass 0,
bInterfaceProtocol 0, two endpoints) and has no input device. So the
interface count varies by model, but an input-less interface bound to
this driver is plain normal.

One thing worth adding to the picture: those interfaces do not merely
bind with msc->input == NULL, they can also reach the dereference.
hid_process_event() calls ->event without checking HID_CLAIMED_INPUT:

	if (hdrv && hdrv->event && hid_match_usage(hid, usage)) {
		ret = hdrv->event(hid, field, usage, value);
		...
	}

	if (hid->claimed & HID_CLAIMED_INPUT)
		hidinput_hid_event(hid, field, usage, value);

and hid_match_usage() returns 1 for everything here, since magicmouse
declares no usage_table. So an input report on interface 2 or 3 walks
into magicmouse_event(), which dereferences msc->input->id.product right
at the top; ->raw_event is called for every report regardless. The NULL
deref is therefore reachable on genuine hardware, not only on a device
spoofing an Apple VID/PID -- a better justification than the one I sent,
so thanks for that as well.

v2 with the per-callback guards follows shortly. Both callbacks return 0,
so those reports keep flowing through the generic HID paths exactly as
they do today, and magicmouse_probe() is left alone. I've credited the
change of approach to you with Suggested-by.

The offer to re-test over USB and Bluetooth is very welcome, thank you.

Jose

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2] HID: magicmouse: avoid NULL pointer deref when there is no input device
  2026-07-15 19:58 [PATCH] HID: magicmouse: reject devices that bind without an input device Jose Villaseñor Montfort
  2026-07-28  7:25 ` Alec Hall
@ 2026-07-28 18:40 ` Jose Villaseñor Montfort
  2026-07-28 18:55   ` sashiko-bot
  1 sibling, 1 reply; 5+ messages in thread
From: Jose Villaseñor Montfort @ 2026-07-28 18:40 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires
  Cc: Alec Hall, linux-input, linux-kernel,
	Jose Villaseñor Montfort

magicmouse_raw_event() and magicmouse_event() dereference msc->input
(e.g. input->id.product, and via magicmouse_emit_touch() and
magicmouse_emit_buttons()) without checking it for NULL. hid-input only
sets msc->input when the device exposes a usable input device.

magicmouse_probe() has an "input not registered" check that fails the
probe when msc->input is NULL, but the USB Magic Mouse 2 / Magic
Trackpad 2 path returns 0 before reaching it. Real hardware ends up in
that state: a USB Magic Trackpad 2 exposes four HID interfaces and only
the first two register an input, and a USB-C Magic Trackpad (05ac:0324)
exposes three with the same split. The remaining interfaces are
vendor-defined, are claimed as hiddev/hidraw only, and stay bound to
this driver with msc->input == NULL.

Both callbacks are reachable in that state. ->raw_event is called for
every incoming report, and hid_process_event() calls ->event without
requiring HID_CLAIMED_INPUT -- hid_match_usage() matches everything here
because this driver has no usage_table. An input report on one of those
interfaces therefore dereferences a NULL pointer and panics the kernel.
A device that binds this driver by spoofing an Apple VID/PID, with a
report descriptor that does not produce an input device, reaches the
same state.

Bail out of both callbacks when msc->input is NULL and leave the report
to the generic HID paths, which is what those interfaces get today.
Rejecting the bind in magicmouse_probe() instead would unbind interfaces
that a healthy device legitimately exposes and drop their hidraw nodes.

Fixes: 0b91b4e4dae6 ("HID: magicmouse: Report battery level over USB")
Suggested-by: Alec Hall <signshop.alec@gmail.com>
Link: https://lore.kernel.org/linux-input/20260714102540.3EB2E1F000E9@smtp.kernel.org/
Link: https://lore.kernel.org/linux-input/20260728072554.47069-1-signshop.alec@gmail.com/
Cc: stable@vger.kernel.org
Signed-off-by: Jose Villaseñor Montfort <pepemontfort@gmail.com>
---
Changes in v2:
- Switch from rejecting the bind in magicmouse_probe() to guarding both
  callbacks. Alec Hall tested v1 on a USB Magic Trackpad 2 and found that
  it unbinds interfaces 2 and 3: they are vendor-defined, have no input
  device, and have been claimed by this driver since 0b91b4e4dae6, so
  every cable plug logged two -ENOMEM probe failures and their hidraw
  nodes went away. Nothing user-facing broke (touch and battery live on
  interfaces 0/1), but silently unbinding a healthy device's interfaces
  is not a change to make in a stable-tagged fix. Report:
  https://lore.kernel.org/linux-input/20260728072554.47069-1-signshop.alec@gmail.com/
- I confirmed the same shape on a USB-C Magic Trackpad (05ac:0324) here,
  on an unpatched 7.1.5: three HID interfaces, all three bound to
  magicmouse, and the last one -- vendor-defined, bInterfaceSubClass 0,
  two endpoints -- has a hidraw node but no input device. So the exact
  interface count varies by model, but at least one input-less interface
  bound to this driver is normal on genuine hardware.
- Reworded the commit message accordingly. Those interfaces show that a
  bind with msc->input == NULL is a state real hardware reaches, not only
  a spoofed device; and hid_process_event() calls ->event regardless of
  HID_CLAIMED_INPUT (hid_match_usage() matches everything, this driver
  has no usage_table), so ->event is reachable on them too.
- v1: https://lore.kernel.org/linux-input/20260715195853.1302765-1-pepemontfort@gmail.com/

This is a sibling hardening fix to "HID: magicmouse: prevent unbounded
recursion in magicmouse_raw_event()" [1], which touches the same driver.
If that one is applied first, this needs a trivial rebase: the guard
moves into __magicmouse_raw_event().

[1] https://lore.kernel.org/linux-input/20260715053526.574725-1-pepemontfort@gmail.com/

 drivers/hid/hid-magicmouse.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 802a3479e..7164ced59 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;
+
 	/* Protect against zero sized recursive calls from DOUBLE_REPORT_ID */
 	if (size < 1)
 		return 0;
@@ -538,6 +546,11 @@ static int magicmouse_event(struct hid_device *hdev, struct hid_field *field,
 		struct hid_usage *usage, __s32 value)
 {
 	struct magicmouse_sc *msc = hid_get_drvdata(hdev);
+
+	/* See the comment in magicmouse_raw_event(). */
+	if (!msc->input)
+		return 0;
+
 	if ((msc->input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE2 ||
 	     msc->input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE2_USBC) &&
 	    field->report->id == MOUSE2_REPORT_ID) {

base-commit: b7556c8e713c88596046a906c7c4385218d44736
-- 
2.55.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] HID: magicmouse: avoid NULL pointer deref when there is no input device
  2026-07-28 18:40 ` [PATCH v2] HID: magicmouse: avoid NULL pointer deref when there is no " Jose Villaseñor Montfort
@ 2026-07-28 18:55   ` sashiko-bot
  0 siblings, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-07-28 18:55 UTC (permalink / raw)
  To: Jose Villaseñor Montfort; +Cc: linux-input, dmitry.torokhov

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 <pepemontfort@gmail.com>

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/20260714102540.3EB2E1F000E9@smtp.kernel.org/
Link: https://lore.kernel.org/linux-input/20260728072554.47069-1-signshop.alec@gmail.com/

> 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/20260728184059.688513-1-pepemontfort@gmail.com?part=1

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-07-28 18:55 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-15 19:58 [PATCH] HID: magicmouse: reject devices that bind without an input device Jose Villaseñor Montfort
2026-07-28  7:25 ` Alec Hall
2026-07-28 18:37   ` Jose Villaseñor Montfort
2026-07-28 18:40 ` [PATCH v2] HID: magicmouse: avoid NULL pointer deref when there is no " Jose Villaseñor Montfort
2026-07-28 18:55   ` sashiko-bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox