Re: [PATCH v9 1/3] HID: nintendo: Add preliminary Switch 2 controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Potential NULL pointer dereference in `switch2_handle_flash_read()` when receiving spontaneous or delayed USB messages.
- [High] Race condition in `switch2_remove()` clearing drvdata before hardware stop causes NULL pointer dereference.
- [High] Unconditional `ida_free()` with `U32_MAX` triggers a `BUG_ON` panic on probe failure or device removal.
- [High] Using `devm_led_classdev_register` with `&input->dev` causes a devres circular reference, leaking devices and causing a Use-After-Free.
- [High] The `-EBUSY` error path in `switch2_probe` erroneously destroys the active input device of the existing interface.
- [High] TOCTOU race condition in `switch2_usb_disconnect` causes UAF due to missing `usb_kill_urb`.
- [Medium] URB slot leak and stuck polling on transient USB errors, plus processing of invalid data.
--

commit ef162798bccd0977cafe2dea531f9e56194e0992
Author: Vicki Pfau <vi@endrift.com>
Subject: HID: nintendo: Add preliminary Switch 2 controller driver

This patch adds a new driver for Nintendo Switch 2 controllers, using a
split-driver design for HID and USB configuration interfaces. It brings
support for Joy-Con 2, Pro Controller 2, and GameCube NSO controllers via USB.

I found a few regressions while reviewing this patch.

> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index e7302ec01ff1f..5aacb8e88b704 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
> @@ -2838,7 +2845,1194 @@ static int nintendo_hid_suspend(struct hid_device *hdev, pm_message_t message)
> [ ... ]
> +static void switch2_handle_flash_read(struct switch2_controller *ns2, uint8_t size,
> +	uint32_t address, const uint8_t *data)
> +{
> +	bool ok;
> +
> +	switch (address) {
> [ ... ]
> +	case NS2_FLASH_ADDR_FACTORY_PRIMARY_CALIB:
> +		if (size != NS2_FLASH_SIZE_FACTORY_AXIS_CALIB)
> +			return;
> +		switch2_init_step_done(ns2, NS2_INIT_READ_FACTORY_PRIMARY_CALIB);
> +		ok = switch2_parse_stick_calibration(&ns2->stick_calib[0], data);
> +		if (ok) {

[Severity: High]
Can this cause a NULL pointer dereference? If the USB configuration
interface receives a flash read reply before the HID interface finishes
probing, or if a delayed response arrives from a previous session,
ns2->hdev will be NULL when hid_dbg is called.

> +			hid_dbg(ns2->hdev, "Got factory primary stick calibration:\n");
> +			hid_dbg(ns2->hdev, "Left max: %i, neutral: %i, right max: %i\n",
> [ ... ]
> +static int switch2_init_input(struct switch2_controller *ns2)
> +{
> [ ... ]
> +	for (i = 0; i < JC_NUM_LEDS; i++) {
> +		struct led_classdev *led = &ns2->leds[i];
> +		char *name = devm_kasprintf(&input->dev, GFP_KERNEL, "%s:%s:%s",
> +				      dev_name(&input->dev),
> +				      "green",
> +				      joycon_player_led_names[i]);
> +
> +		if (!name) {
> +			dev_err(&input->dev, "Failed to allocate name for player %d LED; ret=%d\n",
> +				i + 1, ret);
> +			break;
> +		}
> +
> +		led->name = name;

[Severity: High]
Could this create a circular devres reference? The LED class devices hold
references to their parent (the input device). When the controller is
removed, the input device's devres cleanup won't run until its refcount
hits 0, which cannot happen because the LED devices still exist.

Since the led_classdev structures are embedded in ns2 (which is freed on
removal), might this cause a use-after-free when the leaked LED sysfs
nodes are later accessed?

> +		ret = devm_led_classdev_register(&input->dev, led);
> +		if (ret < 0) {
> +			dev_err(&input->dev, "Failed to register player %d LED; ret=%d\n",
> [ ... ]
> +static int switch2_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> [ ... ]
> +	ns2->player_id = U32_MAX;
> +	ret = ida_alloc(&nintendo_player_id_allocator, GFP_KERNEL);
> +	if (ret < 0)
> +		hid_warn(hdev, "Failed to allocate player ID, skipping; ret=%d\n", ret);
> +	else
> +		ns2->player_id = ret;
> [ ... ]
> +	hid_hw_close(hdev);
> +err_stop:
> +	hid_hw_stop(hdev);
> +err_cleanup:
> +	ida_free(&nintendo_player_id_allocator, ns2->player_id);
> +	ns2->hdev = NULL;

[Severity: High]
Does this error path break the already active controller? If a second HID
interface for the same physical controller probes and detects a conflict,
it returns -EBUSY and jumps to err_put.

However, unconditionally calling switch2_controller_put(ns2) here tears
down the input device for the existing, bound interface.

> +err_put:
> +	switch2_controller_put(ns2);
> +	mutex_unlock(&ns2->lock);
> +	kref_put(&ns2->refcount, switch2_kref_put);
> +
> +	return ret;
> +}
> +
> +static void switch2_remove(struct hid_device *hdev)
> +{
> +	struct switch2_controller *ns2 = hid_get_drvdata(hdev);
> +
> +	mutex_lock(&ns2->lock);
> +	switch2_controller_put(ns2);
> +	ns2->hdev = NULL;

[Severity: High]
Can this cause a race condition leading to a NULL pointer dereference?
The drvdata is set to NULL before hid_hw_stop() halts the hardware.

If an input interrupt triggers switch2_event() in this window, it will
retrieve a NULL pointer from hid_get_drvdata() and immediately dereference
it when calling rcu_dereference(ns2->input).

> +	hid_set_drvdata(hdev, NULL);

[Severity: High]
Does this trigger a panic if ida_alloc() failed during probe? If the probe
fails to allocate the ID, ns2->player_id remains U32_MAX.

Unconditionally calling ida_free() during removal passes this value, which
casts to -1 and will trigger the BUG_ON((int)id < 0) inside the ida_free()
implementation.

> +	ida_free(&nintendo_player_id_allocator, ns2->player_id);
> +	mutex_unlock(&ns2->lock);
> +	kref_put(&ns2->refcount, switch2_kref_put);
> +	hid_hw_close(hdev);
> +	hid_hw_stop(hdev);
> +}

> diff --git a/drivers/input/joystick/nintendo-switch2-usb.c b/drivers/input/joystick/nintendo-switch2-usb.c
> new file mode 100644
> index 0000000000000..0a004918c1f10
> --- /dev/null
> +++ b/drivers/input/joystick/nintendo-switch2-usb.c
> @@ -0,0 +1,464 @@
> [ ... ]
> +static void switch2_bulk_in(struct urb *urb)
> +{
> [ ... ]
> +	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;
> +	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);
> +	for (i = 0; i < NS2_IN_URBS; i++) {
> +		int err;
> +		struct switch2_urb *ns2_urb;
> +

[Severity: Medium]
Does this leak the URB slot and process invalid data? If an URB completes
with an error, the schedule flag remains false, but the state is set to
NS2_URB_IN. If both IN URBs fail, the workqueue is never scheduled again.

If the workqueue is scheduled by the other URB, switch2_usb_message_in_work()
will process this failed URB without validating urb->status, passing
potentially garbage data to switch2_receive_command().

> +		if (ns2_usb->bulk_in[i].urb == urb) {
> +			ns2_usb->bulk_in[i].state = NS2_URB_IN;
> +			continue;
> +		}
> [ ... ]
> +static void switch2_usb_disconnect(struct usb_interface *intf)
> +{
> [ ... ]
> +	/* Prevent any further IN URBs from being scheduled */
> +	spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
> +	ns2_usb->shutdown = true;
> +	spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
> +

[Severity: High]
Is there a TOCTOU race condition here during disconnect? The USB core
unanchors URBs before invoking their completion handler.

If switch2_bulk_in() is running concurrently, usb_kill_anchored_urbs()
won't wait for it. cancel_work_sync() might return immediately if the
handler hasn't yet called schedule_work().

Could the handler then schedule work on the freed ns2_usb structure,
leading to a use-after-free?

> +	usb_kill_anchored_urbs(&ns2_usb->bulk_in_anchor);
> +	cancel_work_sync(&ns2_usb->message_in_work);
> +	for (i = 0; i < NS2_IN_URBS; i++) {
> +		spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);

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