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

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing `usb_unanchor_urb()` on `usb_submit_urb()` failure in `switch2_usb_probe()` causes an infinite loop during cleanup.
- [High] Player LED class devices are registered using devm helpers attached to an unmanaged `input_dev`, causing a Use-After-Free on device unplug.
- [High] Calling `ida_free()` with an unallocated ID (`U32_MAX`) on error and unbind paths triggers a `BUG_ON()` kernel panic.
- [High] The controller context (`ns2`) is freed before stopping incoming HID reports during device removal.
- [High] Submitting the bulk-in URB before successfully attaching the controller configuration causes a TOCTOU race and Use-After-Free if `switch2_controller_attach_cfg()` fails.
--

--- Patch [1]: [PATCH v13 1/3] HID: nintendo: Add preliminary Switch 2 controller driver ---
commit d121d655a283cdffdd144741982d049db4fd8c05
Author: Vicki Pfau <[email protected]>
Subject: HID: nintendo: Add preliminary Switch 2 controller driver

This adds a new driver for the Switch 2 controllers. The Switch 2 uses an
unusual split-interface design such that input and rumble occur on the main
HID interface, but all other communication occurs over a configuration
interface. This initial work supports general input for the Switch 2 Joy-Con,
Pro Controller, and GameCube NSO controllers.

> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index e7302ec01ff1f..78c9ad3c16104 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c

[ ... ]

> +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)
> +{

[ ... ]

> +	ns2->leds = devm_kcalloc(&input->dev, JC_NUM_LEDS, sizeof(*ns2->leds), GFP_KERNEL);
> +	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;
> +		}
> +	}

[Severity: High]
Is it safe to tie the LED devm allocations to the unmanaged input device here?

If userspace holds an open file descriptor for the input device when it is
unregistered during unplug, the devres cleanup is deferred. Could a subsequent
sysfs write to the LED brightness trigger a use-after-free when
switch2_player_led_brightness_set() dereferences the already freed controller
context?

[ ... ]

> +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;

[ ... ]

> +err_cleanup:
> +	ida_free(&nintendo_player_id_allocator, ns2->player_id);
> +	ns2->hdev = NULL;
> +	mutex_unlock(&ns2->lock);
> +	switch2_controller_put(ns2);
> +	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);
> +
> +	switch2_controller_put(ns2);
> +	mutex_lock(&ns2->lock);
> +	ns2->hdev = NULL;
> +	ida_free(&nintendo_player_id_allocator, ns2->player_id);

[Severity: High]
If ida_alloc() fails or is skipped, ns2->player_id remains U32_MAX. Since
ida_free() expects a valid ID and asserts on negative values, could
unconditionally freeing U32_MAX cause a kernel panic here?

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

[Severity: High]
If the hardware continues sending events during device removal, could a report
arrive after kref_put() frees the controller context but before hid_hw_stop()
completes?

This appears to leave a window for a use-after-free in switch2_event() when
dereferencing the controller context.

> +}

[ ... ]

> diff --git a/drivers/input/joystick/nintendo-switch2-usb.c b/drivers/input/joystick/nintendo-switch2-usb.c
> new file mode 100644
> index 0000000000000..36a5ffbd6a756
> --- /dev/null
> +++ b/drivers/input/joystick/nintendo-switch2-usb.c

[ ... ]

> +static void switch2_usb_message_in_work(struct work_struct *work)
> +{

[ ... ]

> +	spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
> +	for (i = 0; i < NS2_IN_URBS; i++) {
> +		urb = &ns2_usb->bulk_in[i];
> +		if (urb->state != NS2_URB_IN)
> +			continue;
> +		spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
> +
> +		if (ns2_usb->cfg.parent) {
> +			err = switch2_receive_command(ns2_usb->cfg.parent,
> +				urb->urb->transfer_buffer, urb->urb->actual_length);

[Severity: High]
Is it possible for the endpoint to respond immediately after the bulk-in URB
is submitted in switch2_usb_probe(), scheduling message_in_work?

If switch2_controller_attach_cfg() subsequently fails, it will clean up and
free the controller context. Could this cause a use-after-free if the worker
thread evaluates ns2_usb->cfg.parent as non-NULL just before the failure frees
it, passing a dangling pointer to switch2_receive_command()?

[ ... ]

> +static int switch2_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
> +{

[ ... ]

> +	spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
> +	ns2_usb->bulk_in[0].state = NS2_URB_OUT;
> +	usb_anchor_urb(ns2_usb->bulk_in[0].urb, &ns2_usb->bulk_in_anchor);
> +	ret = usb_submit_urb(ns2_usb->bulk_in[0].urb, GFP_ATOMIC);
> +	spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
> +
> +	if (ret < 0)
> +		goto err_free_out;

[Severity: High]
When usb_submit_urb() fails, does the URB stay on the anchor list?

Since usb_kill_anchored_urbs() does not manually unanchor unsubmitted URBs,
could this cause an infinite loop when the error path eventually calls
usb_kill_anchored_urbs(&ns2_usb->bulk_in_anchor)?

> +
> +	ret = switch2_controller_attach_cfg(phys, &ns2_usb->cfg);
> +	if (ret < 0)
> +		goto err_free_out;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1