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

[Vicki Pfau <vi@endrift.com>]

On 6/24/26 10:18 PM, sashiko-bot@kernel.org wrote:
> 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.

Looks like leftover from a previous iteration. Removing.

> 
> [ ... ]
> 
>> +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.

I will see about tying this into the ref system I added for patch 1 to delay the free.> 
> [ ... ]
> 
>> +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.

I think I've seen this. I have trouble reproducing it but I'll fix this and hopefully it'll stop reproducing.

> 
>> +	} 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().
> 
> [ ... ]

Fixing.

> 
>> @@ -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.
> 

This will be fixed by my probe refactor already.