Linux Input/HID development
 help / color / mirror / Atom feed
* [PATCH] HID: magicmouse: do not keep a stale msc->input if no input is claimed
@ 2026-07-29  4:15 Jose Villaseñor Montfort
  2026-07-29  4:31 ` sashiko-bot
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Jose Villaseñor Montfort @ 2026-07-29  4:15 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires
  Cc: Alec Hall, linux-input, linux-kernel,
	Jose Villaseñor Montfort

magicmouse_input_mapping() caches the first hid_input's input_dev in
msc->input while the report descriptor is parsed, and the rest of the
driver treats a non-NULL msc->input as proof that an input device was
registered.

That does not hold on the hid-input error path. If hidinput_connect()
fails -- for instance because input_register_device() returns an error --
it unwinds through hidinput_disconnect(), which frees every input_dev it
created, including the one cached in msc->input.

The failure does not abort the probe. hid_connect() only skips the claim:

	if ((connect_mask & HID_CONNECT_HIDINPUT) && !hidinput_connect(hdev,
				connect_mask & HID_CONNECT_HIDINPUT_FORCE))
		hdev->claimed |= HID_CLAIMED_INPUT;

and the "device has no listeners" bailout below it does not fire for this
driver, which sets ->raw_event; on the USB Magic Mouse 2 / Magic Trackpad
2 paths hidraw and hiddev are claimed as well. hid_hw_start() therefore
returns 0 and magicmouse_probe() continues with msc->input pointing at
freed memory. Being non-NULL, it passes the "input not registered" check
in probe and the NULL checks in ->raw_event and ->event, so the next
input report dereferences freed memory.

Clear msc->input when the HID core did not claim an input device, so the
existing NULL checks cover this case as well.

Fixes: f1a9a149abc8 ("HID: magicmouse: fix race between input_register() and probe()")
Link: https://lore.kernel.org/linux-input/20260728185542.65F091F000E9@smtp.kernel.org/
Cc: stable@vger.kernel.org
Signed-off-by: Jose Villaseñor Montfort <pepemontfort@gmail.com>
---
Found by the Sashiko AI review of "[PATCH v2] HID: magicmouse: avoid NULL
pointer deref when there is no input device" (Link: above); I verified
the path before writing this. That patch [1] is a sibling fix that adds
the NULL checks in ->raw_event and ->event which this one makes
effective on the error path; it applies independently of this one.

I picked f1a9a149abc8 for the Fixes: tag because that is where probe()
started treating a non-NULL msc->input as proof that an input device was
registered. The dangling pointer itself is older than that; happy to
change the tag if reviewers prefer a different one, or to drop the
stable Cc, given this needs an input_register_device() failure to
trigger.

Also a sibling to "HID: magicmouse: prevent unbounded recursion in
magicmouse_raw_event()" [2], which touches the same driver but a
different function.

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

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

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 802a3479e..2f14094a6 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -900,6 +900,16 @@ static int magicmouse_probe(struct hid_device *hdev,
 		return ret;
 	}
 
+	/*
+	 * When hidinput_connect() fails it frees every input device it
+	 * created, but that does not fail hid_hw_start(): the core simply
+	 * does not claim an input. msc->input, cached in ->input_mapping
+	 * while the report descriptor was parsed, would then be a dangling
+	 * pointer that passes every NULL check. Trust the core's claim.
+	 */
+	if (!(hdev->claimed & HID_CLAIMED_INPUT))
+		msc->input = NULL;
+
 	if (is_usb_magicmouse2(id->vendor, id->product) ||
 	    is_usb_magictrackpad2(id->vendor, id->product)) {
 		timer_setup(&msc->battery_timer, magicmouse_battery_timer_tick, 0);

base-commit: b7556c8e713c88596046a906c7c4385218d44736
-- 
2.55.0


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

* Re: [PATCH] HID: magicmouse: do not keep a stale msc->input if no input is claimed
  2026-07-29  4:15 [PATCH] HID: magicmouse: do not keep a stale msc->input if no input is claimed Jose Villaseñor Montfort
@ 2026-07-29  4:31 ` sashiko-bot
  2026-08-03  4:25 ` Alec Hall
  2026-08-03 17:55 ` Jiri Kosina
  2 siblings, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-07-29  4:31 UTC (permalink / raw)
  To: Jose Villaseñor Montfort; +Cc: dmitry.torokhov, linux-input

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Deterministic NULL pointer dereference in event handlers after a failed input registration.
--

--- Patch [1]: [PATCH] HID: magicmouse: do not keep a stale msc->input if no input is claimed ---
commit 6fea999082f28bca408072a6788f3616b0ebbd9f
Author: Jose Villaseñor Montfort <pepemontfort@gmail.com>

HID: magicmouse: do not keep a stale msc->input if no input is claimed

This patch clears msc->input when the HID core does not claim an input
device. This is intended to ensure existing NULL checks safely handle the
failure path where hidinput_connect() frees the input devices.

Link: https://lore.kernel.org/linux-input/20260728185542.65F091F000E9@smtp.kernel.org/

> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index 802a3479e24b9..2f14094a63b1e 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -900,6 +900,16 @@ static int magicmouse_probe(struct hid_device *hdev,
>  		return ret;
>  	}
>  
> +	/*
> +	 * When hidinput_connect() fails it frees every input device it
> +	 * created, but that does not fail hid_hw_start(): the core simply
> +	 * does not claim an input. msc->input, cached in ->input_mapping
> +	 * while the report descriptor was parsed, would then be a dangling
> +	 * pointer that passes every NULL check. Trust the core's claim.
> +	 */
> +	if (!(hdev->claimed & HID_CLAIMED_INPUT))
> +		msc->input = NULL;
> +

[Severity: High]
If msc->input is set to NULL here due to a failure, could this cause a NULL
pointer dereference for certain USB devices?

Looking further down in magicmouse_probe(), there is an early return on
success for specific devices that bypasses the failure check:

drivers/hid/hid-magicmouse.c:magicmouse_probe() {
    ...
    if (!(hdev->claimed & HID_CLAIMED_INPUT))
        msc->input = NULL;
    ...
    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");
        ...
}

If the probe succeeds by returning 0, the driver remains bound.

When an input report is later received, will magicmouse_raw_event() and
magicmouse_event() unconditionally dereference the NULL msc->input pointer?

For example, looking at magicmouse_raw_event():

drivers/hid/hid-magicmouse.c:magicmouse_raw_event() {
    ...
    struct input_dev *input = msc->input;
    ...
    if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE ||
    ...
}

And similarly in magicmouse_event():

drivers/hid/hid-magicmouse.c:magicmouse_event() {
    ...
    struct magicmouse_sc *msc = hid_get_drvdata(hdev);
    if ((msc->input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE2 ||
    ...
}

>  	if (is_usb_magicmouse2(id->vendor, id->product) ||
>  	    is_usb_magictrackpad2(id->vendor, id->product)) {
>  		timer_setup(&msc->battery_timer, magicmouse_battery_timer_tick, 0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729041557.1185819-1-pepemontfort@gmail.com?part=1

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

* Re: [PATCH] HID: magicmouse: do not keep a stale msc->input if no input is claimed
  2026-07-29  4:15 [PATCH] HID: magicmouse: do not keep a stale msc->input if no input is claimed Jose Villaseñor Montfort
  2026-07-29  4:31 ` sashiko-bot
@ 2026-08-03  4:25 ` Alec Hall
  2026-08-03 17:55 ` Jiri Kosina
  2 siblings, 0 replies; 4+ messages in thread
From: Alec Hall @ 2026-08-03  4:25 UTC (permalink / raw)
  To: pepemontfort; +Cc: jikos, bentiss, linux-input, linux-kernel, Alec Hall

On Tue, Jul 28, 2026, Jose Villaseñor Montfort wrote:
> Clear msc->input when the HID core did not claim an input device, so the
> existing NULL checks cover this case as well.

Reviewed the path and agree with the analysis. Trusting hdev->claimed
rather than the cached pointer is the right signal: it is set by the core
from the outcome of hidinput_connect(), whereas msc->input is set from
->input_mapping while the descriptor is parsed, long before anyone knows
whether the registration will succeed.

One data point in support of the "hid_hw_start() still returns 0" step,
from instrumenting the earlier v1 report on a USB Magic Trackpad 2: the
input-less vendor interfaces reach magicmouse_probe() with
hdev->claimed == 0x6 (HIDRAW|HIDDEV, no HID_CLAIMED_INPUT) and probe
carries on normally. That is the same "core claimed no input, probe
continues" state your patch keys off, just arrived at without an error.

Note the driver already clears msc->input in magicmouse_input_configured()
when its own setup fails, so this closes the remaining door: hid-input
unwinding for a reason the driver never sees.

Tested on a Magic Trackpad 2 [Lightning] (05ac:0265) on 7.1.5, with your sibling
guards patch [1] also applied. Since input_register_device() does not fail
on real hardware, I reproduced the state with a test-only fault injection:
a module parameter making magicmouse_input_configured() return -EINVAL
*without* clearing msc->input. hid-input then unwinds and frees the
input_dev that ->input_mapping had already cached, and hid_hw_start()
returns 0 -- the same dangling msc->input your patch describes, reached
from a failure the driver did not cause itself.

Without your patch (fix disabled at runtime, injection armed), plugging in
over USB:

  magicmouse 0003:05AC:0265.0022: hiddev109,hidraw16: USB HID v1.10 Mouse
      [Apple Inc. Magic Trackpad 2] on usb-0000:00:14.0-8/input1

The boot-protocol interface binds. No "magicmouse input not registered",
because the freed-but-non-NULL pointer passes that check exactly as you
said it would, and the driver goes on to arm the device.

With your patch, the same plug:

  magicmouse 0003:05AC:0265.0014: magicmouse input not registered
  magicmouse 0003:05AC:0265.0014: probe with driver magicmouse failed with error -12

The pointer is cleared, the existing check fires, and that interface
refuses to bind instead of running with freed memory. The interfaces that
take the early return bind inert with hiddev/hidraw only, and over
Bluetooth the device fails probe the same way. No oops or corruption in
either configuration, and disarming the injection restores normal
operation on both transports (touch, gestures, battery 76% Discharging
over BT, 74% Charging over USB).

Caveat on the above: this reproduces the dangling-pointer state, not an
actual input_register_device() failure, and the kernel here has no KASAN,
so I deliberately did not drive input reports through the unpatched case
-- the evidence is that the check passes when it should not, at probe
time.

On your open questions: f1a9a149abc8 reads right to me for the Fixes: tag,
since that is where probe() began treating a non-NULL msc->input as proof
of registration, and the commit message is clear that the dangling pointer
predates it. I would keep the stable Cc -- it is a use-after-free, and
"hard to trigger" is about allocation failure rather than anything a
device can or cannot do -- but that is a maintainer call.

Reviewed-by: Alec Hall <signshop.alec@gmail.com>
Tested-by: Alec Hall <signshop.alec@gmail.com>

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

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

* Re: [PATCH] HID: magicmouse: do not keep a stale msc->input if no input is claimed
  2026-07-29  4:15 [PATCH] HID: magicmouse: do not keep a stale msc->input if no input is claimed Jose Villaseñor Montfort
  2026-07-29  4:31 ` sashiko-bot
  2026-08-03  4:25 ` Alec Hall
@ 2026-08-03 17:55 ` Jiri Kosina
  2 siblings, 0 replies; 4+ messages in thread
From: Jiri Kosina @ 2026-08-03 17:55 UTC (permalink / raw)
  To: Jose Villaseñor Montfort
  Cc: Benjamin Tissoires, Alec Hall, linux-input, linux-kernel

On Tue, 28 Jul 2026, Jose Villaseñor Montfort wrote:

> magicmouse_input_mapping() caches the first hid_input's input_dev in
> msc->input while the report descriptor is parsed, and the rest of the
> driver treats a non-NULL msc->input as proof that an input device was
> registered.
> 
> That does not hold on the hid-input error path. If hidinput_connect()
> fails -- for instance because input_register_device() returns an error --
> it unwinds through hidinput_disconnect(), which frees every input_dev it
> created, including the one cached in msc->input.
> 
> The failure does not abort the probe. hid_connect() only skips the claim:
> 
> 	if ((connect_mask & HID_CONNECT_HIDINPUT) && !hidinput_connect(hdev,
> 				connect_mask & HID_CONNECT_HIDINPUT_FORCE))
> 		hdev->claimed |= HID_CLAIMED_INPUT;
> 
> and the "device has no listeners" bailout below it does not fire for this
> driver, which sets ->raw_event; on the USB Magic Mouse 2 / Magic Trackpad
> 2 paths hidraw and hiddev are claimed as well. hid_hw_start() therefore
> returns 0 and magicmouse_probe() continues with msc->input pointing at
> freed memory. Being non-NULL, it passes the "input not registered" check
> in probe and the NULL checks in ->raw_event and ->event, so the next
> input report dereferences freed memory.
> 
> Clear msc->input when the HID core did not claim an input device, so the
> existing NULL checks cover this case as well.
> 
> Fixes: f1a9a149abc8 ("HID: magicmouse: fix race between input_register() and probe()")
> Link: https://lore.kernel.org/linux-input/20260728185542.65F091F000E9@smtp.kernel.org/
> Cc: stable@vger.kernel.org
> Signed-off-by: Jose Villaseñor Montfort <pepemontfort@gmail.com>

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs


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

end of thread, other threads:[~2026-08-03 17:55 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-29  4:15 [PATCH] HID: magicmouse: do not keep a stale msc->input if no input is claimed Jose Villaseñor Montfort
2026-07-29  4:31 ` sashiko-bot
2026-08-03  4:25 ` Alec Hall
2026-08-03 17:55 ` Jiri Kosina

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