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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] hid: nintendo: devm_led_classdev_register causes circular reference and UAF
- [High] hid: nintendo: switch2_usb_message_in_work accesses ns2 without kref leading to UAF
- [High] hid: nintendo: state machine restart in switch2_remove leads to UAF and leak
- [High] hid: nintendo: switch2_usb_disconnect UAF due to missing usb_kill_urb
--

commit c3c5a054be7d9d99ff307ce80f5e14be33bc3411
Author: Vicki Pfau <vi@endrift.com>

HID: nintendo: Add preliminary Switch 2 controller driver

This commit adds a new driver for Nintendo Switch 2 controllers utilizing a
split-interface design. It supports USB communication for Switch 2 Joy-Cons,
Pro Controllers, and GameCube NSO controllers.

> diff --git a/drivers/hid/hid-nintendo-switch2-usb.c b/drivers/hid/hid-nintendo-switch2-usb.c
> --- /dev/null
> +++ b/drivers/hid/hid-nintendo-switch2-usb.c
[ ... ]
> +static void switch2_usb_message_in_work(struct work_struct *work)
> +{
> +	struct switch2_usb *ns2_usb = container_of(work, struct switch2_usb, message_in_work);
> +	struct switch2_urb *urb;
> +	int err;
> +	int i;
> +	unsigned long flags;
> +
> +	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]
Does this access the parent controller without holding a reference?

If switch2_controller_attach_cfg() fails, the kref on the parent controller
is dropped before cancelling this worker. At the same time, if the HID device
unbinds, switch2_remove() can drop the final kref and free the structure.

If this worker thread is preempted after reading the pointer but before
acquiring the lock, could it wake up and call switch2_receive_command() on
the freed structure?

> +			if (err)
> +				dev_dbg(&ns2_usb->udev->dev, "receive command failed: %d\n", err);
> +		} else {
[ ... ]
> +static void switch2_usb_disconnect(struct usb_interface *intf)
> +{
> +	struct switch2_usb *ns2_usb = usb_get_intfdata(intf);
> +	unsigned long flags;
> +	struct urb *urb;
> +	uint8_t *data;
> +	int i;
> +
> +	/* 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);
> +
> +	usb_kill_anchored_urbs(&ns2_usb->bulk_in_anchor);
> +	cancel_work_sync(&ns2_usb->message_in_work);

[Severity: High]
Could this lead to a use-after-free if an unanchored URB completes?

The USB core unanchors URBs before invoking their completion handlers.
If switch2_bulk_in() is executing, usb_kill_anchored_urbs() will not find it.

This allows cancel_work_sync() to return immediately. When the preempted
completion handler resumes, it will execute schedule_work() on the devres
managed structure that may have already been freed.

Should this explicitly wait for unanchored URBs by tracking them and calling
usb_kill_urb() on each?

> +	for (i = 0; i < NS2_IN_URBS; i++) {
> +		spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
> +		urb = ns2_usb->bulk_in[i].urb;

> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
[ ... ]
> +static int switch2_init_input(struct switch2_controller *ns2)
> +{
> +	struct input_dev *input;
> +	struct hid_device *hdev = ns2->hdev;
> +	int player_led_pattern;
> +	int i;
> +	int ret;
[ ... ]
> +		led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE | LED_RETAIN_AT_SHUTDOWN;
> +		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;
> +		ret = devm_led_classdev_register(&input->dev, led);

[Severity: High]
Does this create an unbreakable devres circular reference?

Using the input device as the parent for devm_led_classdev_register() links
the devres action to the input device, but the LED classdev child inherently
holds a reference to its parent.

When input_unregister_device() runs during removal, the input device's
refcount never drops to zero, leaking it and preventing devres cleanup.
The LED sysfs attributes remain active.

If userspace writes to the leaked LED attributes after switch2_remove() frees
the controller structure, switch2_player_led_brightness_set() will access
freed memory.

> +		if (ret < 0) {
> +			dev_err(&input->dev, "Failed to register player %d LED; ret=%d\n",
> +				i + 1, ret);
[ ... ]
> +static void switch2_controller_put(struct switch2_controller *ns2)
> +{
> +	struct input_dev *input;
> +
> +	mutex_lock(&ns2->lock);
> +	rcu_read_lock();
> +	input = rcu_dereference(ns2->input);
> +	rcu_read_unlock();
> +
> +	rcu_assign_pointer(ns2->input, NULL);
> +	synchronize_rcu();
> +
> +	ns2->init_step = 0;
> +	mutex_unlock(&ns2->lock);
> +
> +	if (input)
> +		input_unregister_device(input);

[Severity: High]
Does clearing the initialization step before the blocking unregister open
a race condition that leaks the input device?

By dropping the lock before calling the long-running input_unregister_device(),
concurrent USB packets can trigger switch2_receive_command(). Since the lock is
re-acquired and init_step is 0, this restarts the initialization state machine.

Because switch2_remove() hasn't cleared hdev yet:

switch2_remove()
   switch2_controller_put(ns2);
   mutex_lock(&ns2->lock);
   ns2->hdev = NULL;

The state machine successfully registers a completely new input_dev using the
soon-to-be-destroyed hdev as its parent. This leaks the newly registered
device and leaves it with a dangling parent pointer.

> +}
> +
> +static void switch2_kref_put(struct kref *refcount)

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