Re: [PATCH v10 1/3] 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] Calling `input_free_device()` on a successfully registered `input_dev` when subsequent allocations fail.
- [High] Use-After-Free of `ns2->leds` due to devm lifetime mismatch when reusing the global controller structure.
- [High] Race condition causing NULL pointer dereference of `ns2->cfg` during USB initialization error.
- [High] Unconditional `ida_free()` with `U32_MAX` causes a kernel BUG_ON.
--

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

HID: nintendo: Add preliminary Switch 2 controller driver

This commit adds a new driver for the Switch 2 controllers. It utilizes a
split-interface design where input and rumble occur on the main HID
interface, while all other communication uses a configuration interface.
The current implementation supports general input for Joy-Con 2, Pro
Controller 2, and GameCube NSO controllers over USB.

> 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
> @@ -2838,7 +2845,1206 @@ static int nintendo_hid_suspend(struct hid_device *hdev, pm_message_t message)
> [ ... ]
> +static int switch2_init_input(struct switch2_controller *ns2)
> +{
> [ ... ]
> +	ret = input_register_device(input);
> +	if (ret < 0) {
> +		hid_err(ns2->hdev, "Failed to register input; ret=%d\n", ret);
> +		input_free_device(input);
> +		return ret;
> +	}
> +
> +	player_led_pattern = ns2->player_id % JC_NUM_LED_PATTERNS;
> +	hid_dbg(hdev, "assigned player %d led pattern", player_led_pattern + 1);
> +
> +	ns2->leds = devm_kcalloc(&input->dev, JC_NUM_LEDS, sizeof(*ns2->leds), GFP_KERNEL);

[Severity: High]
Since the ns2 structure is reused per physical port via
switch2_get_controller(), could there be a lifetime mismatch here?

If a device is unplugged but its evdev file descriptor is kept open by
userspace, input->dev stays alive. If a new device is then plugged into the
same port, ns2 is reused and ns2->leds is overwritten with an allocation
tied to the new input_dev.

When the new device is unplugged, freeing its devm resources, would a write
to the original device's LED sysfs node in
switch2_player_led_brightness_set() iterate over freed memory?

> +	if (!ns2->leds) {
> +		hid_err(ns2->hdev, "Failed to allocate LEDs\n");
> +		input_free_device(input);

[Severity: High]
Is it safe to use input_free_device() here since input_register_device()
has already successfully registered the device above?

Once the input device is registered, won't calling input_free_device() just
drop the initial reference rather than properly unregistering the device,
leaving dangling sysfs nodes and leaking the device?

> +		return -ENOMEM;
> +	}
> [ ... ]
> +int switch2_receive_command(struct switch2_controller *ns2,
> +	const uint8_t *message, size_t length)
> +{
> +	const struct switch2_cmd_header *header;
> +	int ret = 0;
> [ ... ]
> +	header = (const struct switch2_cmd_header *)message;
> +	if (!(header->flags & NS2_FLAG_OK)) {
> +		dev_warn(ns2->cfg->dev, "Packet error %02x replying to command %x:%x",

[Severity: High]
Could this lead to a NULL pointer dereference in the error logging path?

In switch2_usb_probe(), we submit the bulk IN URB before calling
switch2_controller_attach_cfg(). If the device immediately returns an error
packet, switch2_receive_command() might run concurrently.

If attach_cfg fails and clears ns2->cfg to NULL, could the workqueue attempt
to log the error using dev_warn(ns2->cfg->dev, ...) and crash?

> +			header->flags, header->command, header->subcommand);
> +		ret = -EIO;
> +		goto exit;
> +	}
> [ ... ]
> +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);

[Severity: High]
If ida_alloc() fails, ns2->player_id is left as U32_MAX.

If the driver probe subsequently fails and enters this cleanup path, won't
ida_free() with U32_MAX (which casts to a negative integer) trigger a BUG_ON
in the IDA core?

> +	ns2->hdev = NULL;
> +	mutex_unlock(&ns2->lock);
> +	switch2_controller_put(ns2);
> +	kref_put(&ns2->refcount, switch2_kref_put);
> +
> +	return ret;
> +}

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