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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Unsynchronized accesses to ns2->hdev allow rumble work to read a closing device pointer, leading to a TOCTOU NULL pointer dereference.
- [High] cancel_delayed_work_sync() is called on an uninitialized work structure if the configuration interface detaches without the HID interface probing.
- [High] Use-after-free of ns2 due to the input core's memless force feedback timer outliving the driver's private data.
- [Medium] Arithmetic underflow in schedule_delayed_work() delay calculation causes the rumble worker to permanently hang if its execution is delayed.
- [Low] Unused static constant array switch2_zero_data leads to dead code.
--

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

HID: nintendo: Add rumble support for Switch 2 controllers
    
This commit adds basic rumble support for the Switch 2 controllers. It
introduces delayed work and force feedback support.

> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index 6573fd31a232a..ae360f4d15d1a 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c

[ ... ]

> @@ -3133,9 +3183,12 @@ static const uint8_t switch2_init_cmd_data[] = {
>  };
>  
>  static const uint8_t switch2_one_data[] = { 0x01, 0x00, 0x00, 0x00 };
> +#if IS_ENABLED(CONFIG_NINTENDO_FF)
> +static const uint8_t switch2_zero_data[] = { 0x00, 0x00, 0x00, 0x00 };
> +#endif

[Severity: Low]
Is this unused variable intentional? It appears switch2_zero_data is never
utilized anywhere in the codebase.

[ ... ]

> +static int switch2_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect)
> +{
> +	struct switch2_controller *ns2 = input_get_drvdata(dev);
> +
> +	if (effect->type != FF_RUMBLE)
> +		return 0;
> +
> +	guard(spinlock_irqsave)(&ns2->rumble_lock);

[Severity: High]
Can this result in a use-after-free when the input device outlives the driver
data?

During device removal, switch2_controller_put() explicitly calls kfree(ns2).
However, the input device is devres-managed and its lifetime is tied to
userspace holding its file descriptor open. Since the input core's memless
force feedback timer is only destroyed when the input device is released,
the timer callback switch2_play_effect() could potentially fire after ns2
has been freed.

[ ... ]

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

[ ... ]

> +	if (active) {
> +		unsigned long interval = msecs_to_jiffies(2);
> +		uint64_t current_jiffies = get_jiffies_64();
> +
> +		if (!ns2->last_rumble_work)
> +			ns2->last_rumble_work = current_jiffies;
> +		else
> +			ns2->last_rumble_work += interval;
> +		schedule_delayed_work(&ns2->rumble_work,
> +			ns2->last_rumble_work + interval - current_jiffies);

[Severity: Medium]
Could this delay calculation underflow?

If the worker thread is delayed by the scheduler such that current_jiffies
exceeds the expected execution time (ns2->last_rumble_work + interval), the
unsigned arithmetic could underflow to a value near ULONG_MAX. This would
schedule the next rumble update weeks in the future, permanently hanging the
force feedback mechanism.

> +	} else {
> +		ns2->last_rumble_work = 0;
> +	}
> +	spin_unlock_irqrestore(&ns2->rumble_lock, flags);
> +
> +	if (!ns2->hdev)
> +		cancel_delayed_work(&ns2->rumble_work);
> +	else
> +		ret = hid_hw_output_report(ns2->hdev, ns2->rumble_buffer, 64);

[Severity: High]
Is it possible for a concurrent removal to cause a NULL pointer dereference
here?

Because ns2->hdev is read outside of any lock, there may be a window where it
is verified as non-NULL, but then set to NULL by a concurrent
switch2_remove() before being passed to hid_hw_output_report(). This appears
structurally possible if userspace invokes switch2_play_effect() and
reschedules the worker right after the initial cancellation in
switch2_remove().

[ ... ]

> @@ -3358,6 +3521,10 @@ static void switch2_controller_put(struct switch2_controller *ns2)
>  	if (input)
>  		input_unregister_device(input);
>  
> +#if IS_ENABLED(CONFIG_NINTENDO_FF)
> +	cancel_delayed_work_sync(&ns2->rumble_work);
> +#endif

[Severity: High]
Could this cancel operation act on an uninitialized work structure?

The switch2_probe() function initializes the work structure:

    INIT_DELAYED_WORK(&ns2->rumble_work, switch2_rumble_work);

If the configuration interface detaches before the HID interface probes,
ns2->rumble_work might remain zero-initialized. When switch2_controller_put()
is called, unconditionally invoking cancel_delayed_work_sync() on this
uninitialized work structure could trigger lockdep warnings and kernel panics.

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