Re: [PATCH v11 2/3] HID: nintendo: Add rumble support for Switch 2 controllers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Use-After-Free of rumble_work and controller state due to devm lifecycle mismatch

Pre-existing issues:
- [High] Circular reference in LED devm registration causes permanent memory leak and Use-After-Free
--

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

HID: nintendo: Add rumble support for Switch 2 controllers

This adds rumble support for both the "HD Rumble" linear resonant actuator
type as used in the Joy-Cons and Pro Controller, as well as the eccentric
rotating mass type used in the GameCube controller.

> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index e21c369218325..a36f4fd9a1da5 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
[ ... ]
> @@ -3209,6 +3260,125 @@ static void switch2_kref_put(struct kref *refcount)
>  	kfree(ns2);
>  }
>  
> +#if IS_ENABLED(CONFIG_NINTENDO_FF)
[ ... ]
> +static void switch2_rumble_work(struct work_struct *work)
> +{
> +	struct switch2_controller *ns2 = container_of(to_delayed_work(work),
> +						      struct switch2_controller, rumble_work);
> +	unsigned long flags;
> +	bool active;
> +	int ret = 0;
> +
> +	spin_lock_irqsave(&ns2->rumble_lock, flags);

[Severity: High]
If the device is unplugged and `ns2` is manually freed, can this work
function execute concurrently and attempt to acquire `ns2->rumble_lock`
on freed memory?

> +	ns2->rumble_buffer[0x1] = 0x50 | ns2->rumble_seq;
[ ... ]
> @@ -3332,6 +3502,26 @@ static int switch2_init_input(struct switch2_controller *ns2)
>  		return -EINVAL;
>  	}
>  

[Severity: High]
This is a pre-existing issue, but does registering the LED devices with
devm create a circular reference by binding the LED devres to the input
device while simultaneously setting it as the parent?

The driver core's device_add() function automatically takes a reference to
the parent device (&input->dev). Since devres is attached to the same
parent, it waits for the parent's refcount to drop to 0 before unregistering
the LEDs, which means neither can be released. 

Does this permanently leak the input device, LED devices, and devm
allocations? 

Furthermore, because `ns2` is freed manually upon device removal, could
lingering LED sysfs attributes trigger a use-after-free crash if a user
writes to them, causing switch2_player_led_brightness_set() to dereference
the freed `ns2` pointer?

> +#if IS_ENABLED(CONFIG_NINTENDO_FF)
> +	ns2->rumble_buffer = devm_kzalloc(&input->dev, 64, GFP_KERNEL);
> +	if (!ns2->rumble_buffer) {
> +		input_free_device(input);
> +		return -ENOMEM;
> +	}
> +	ret = devm_delayed_work_autocancel(&input->dev, &ns2->rumble_work, switch2_rumble_work);

[Severity: High]
Does binding the rumble work lifecycle to &input->dev cause a
use-after-free when `ns2` is manually freed?

If a user unplugs the controller while holding the input device open,
`ns2` is freed in the removal path, but the devres cleanup for the input
device will run later. 

Will this result in the devres cleanup unconditionally calling
cancel_delayed_work_sync(&ns2->rumble_work) on the already-freed
`ns2` pointer?

> +	if (ret < 0) {
> +		input_free_device(input);
> +		return ret;
> +	}
> +
> +	input_set_capability(input, EV_FF, FF_RUMBLE);
> +	ret = input_ff_create_memless(input, NULL, switch2_play_effect);
[ ... ]

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