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

Jose Villaseñor Montfort <[email protected]>
Newsgroups org.kernel.vger.linux-input,org.kernel.vger.linux-kernel
Message-ID <[email protected]>
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 <[email protected]>
Link: https://lore.kernel.org/linux-input/[email protected]/
Link: https://lore.kernel.org/linux-input/[email protected]/
Cc: [email protected]
Signed-off-by: Jose Villaseñor Montfort <[email protected]>
---
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/[email protected]/
- 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/[email protected]/

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/[email protected]/

 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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.