Re: [PATCH v14 1/4] HID: nintendo: Add preliminary Switch 2 controller driver

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] input: memory leak and UAF due to devm actions on unmanaged input_dev
- [High] usb: TOCTOU race condition causing UAF in message_in_work
- [High] usb: infinite interrupt storm on -EPIPE URB completion
--

commit 276f7b7da744b926faf29e3e4027c0cc47d9bc19
Author: Vicki Pfau [off-list ref]
Subject: HID: nintendo: Add preliminary Switch 2 controller driver

This commit introduces preliminary support for the Nintendo Switch 2
controllers. It uses a split-driver design with a main HID interface driver
and a secondary configuration interface driver to handle input and non-standard
communications over USB and Bluetooth.

[ ... ]

+static void switch2_bulk_in(struct urb *urb)
+{
+	struct switch2_usb *ns2_usb = urb->context;
+	int i;
+	bool schedule = false;
+	unsigned long flags;
+
+	switch (urb->status) {
+	case 0:
+		schedule = true;
+		break;
+	case -ECONNRESET:
+	case -ENOENT:
+	case -ESHUTDOWN:
+		dev_dbg(&ns2_usb->udev->dev, "shutting down input urb: %d\n", urb->status);
+		return;
+	case -EPIPE:
+		break;

[Severity: High]
Does this error path handle halted endpoints correctly? The EPIPE status
indicates a stalled endpoint. By breaking out here without setting the
schedule flag, the code avoids scheduling work, but it proceeds to the
loop below where it immediately re-anchors and resubmits the URB via
usb_submit_urb() without clearing the halt condition. This can cause the
hardware to instantly complete the new URB with EPIPE again, leading to an
infinite interrupt storm in softirq context and a CPU lockup.

+	default:
+		dev_dbg(&ns2_usb->udev->dev, "unknown input urb status: %d\n", urb->status);
+		break;
+	}
+
+	spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
+	if (ns2_usb->shutdown)
+		schedule = false;
+
+	for (i = 0; i < NS2_IN_URBS; i++) {
+		int err;
+		struct switch2_urb *ns2_urb;
+
+		if (ns2_usb->bulk_in[i].urb == urb) {
+			if (schedule) {
+				ns2_usb->bulk_in[i].state = NS2_URB_IN;
+				continue;
+			} else {
+				ns2_usb->bulk_in[i].state = NS2_URB_FREE;
+			}
+		}
+
+		if (ns2_usb->bulk_in[i].state != NS2_URB_FREE)
+			continue;
+
+		/*
+		 * We want exactly one bulk in URB scheduled at a time, so only
+		 * reschedule this immediately if nothing else is scheduled
+		 * currently.
+		 */
+		if (!usb_anchor_empty(&ns2_usb->bulk_in_anchor) || ns2_usb->shutdown)
+			continue;
+
+		ns2_urb = &ns2_usb->bulk_in[i];
+		if (!ns2_urb)
+			continue;
+
+		usb_anchor_urb(ns2_urb->urb, &ns2_usb->bulk_in_anchor);
+		err = usb_submit_urb(ns2_urb->urb, GFP_ATOMIC);
+		if (err) {
+			usb_unanchor_urb(ns2_urb->urb);
+			dev_dbg(&ns2_usb->udev->dev, "failed to queue input urb: %d\n", err);
+		} else {
+			ns2_urb->state = NS2_URB_OUT;
+		}
+	}
+	spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
+
+	if (schedule)
+		schedule_work(&ns2_usb->message_in_work);

[Severity: High]
Can calling schedule_work() outside the spinlock create a TOCTOU race
against switch2_usb_disconnect()?

If switch2_bulk_in() checks ns2_usb->shutdown, releases the lock, and is
then preempted before calling schedule_work(), switch2_usb_disconnect()
could run. It would acquire the lock, set shutdown to true, release the lock,
and call cancel_work_sync(&ns2_usb->message_in_work). Since the work is not
yet queued, cancel_work_sync() returns immediately, and devres frees
ns2_usb. When switch2_bulk_in() resumes, it will call schedule_work() on
freed memory, causing a use-after-free.

+}

[ ... ]

[ ... ]

+static int switch2_player_led_brightness_set(struct led_classdev *led,
+					    enum led_brightness brightness)
+{
+	struct device *dev = led->dev->parent;
+	struct input_dev *input = to_input_dev(dev);
+	struct switch2_controller *ns2 = input_get_drvdata(input);
+
+	if (!ns2)
+		return -ENODEV;
+
+	guard(mutex)(&ns2->lock);
+	return switch2_set_leds(ns2);
+}

[ ... ]

+static int switch2_init_input(struct switch2_controller *ns2)
+{
+	struct input_dev *input;

[ ... ]

+	input = input_allocate_device();
+	if (!input)
+		return -ENOMEM;

[ ... ]

+	ns2->leds = devm_kcalloc(&input->dev, JC_NUM_LEDS, sizeof(*ns2->leds), GFP_KERNEL);

[Severity: High]
Does allocating devm resources tied to an unmanaged input device create a
memory leak and use-after-free risk?

The input device here is allocated manually with input_allocate_device().
Using devm_kcalloc() and later devm_led_classdev_register() with
&input->dev as the parent means the LED devices take a reference to their
parent input device. When the device is removed, input_unregister_device()
is called, but the child LED devices hold the reference so the input device
is never freed.

This leaves the devres actions unexecuted, leaking the input device and
leaving the LED devices active in sysfs. Meanwhile, the driver frees the
ns2 structure. If userspace then writes to the LED brightness file,
switch2_player_led_brightness_set() will execute, fetch the freed ns2
pointer, and dereference it, leading to a deterministic use-after-free.

+	if (!ns2->leds) {
+		hid_err(ns2->hdev, "Failed to allocate LEDs\n");
+		input_unregister_device(input);
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < JC_NUM_LEDS; i++) {
+		struct led_classdev *led = &ns2->leds[i];

[ ... ]

+		ret = devm_led_classdev_register(&input->dev, led);
+		if (ret < 0) {
+			dev_err(&input->dev, "Failed to register player %d LED; ret=%d\n",
+				i + 1, ret);
+			break;
+		}
+	}
+
+	rcu_assign_pointer(ns2->input, input);
+	synchronize_rcu();
+
+	switch2_init_step_done(ns2, NS2_INIT_INPUT);
+	return switch2_init_controller(ns2);
+}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929045607.2106149-1-vi@endrift.com?part=1