Re: [PATCH 2/2] HID: nintendo-switch2-usb: Improve shutdown logic with inbound message work

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] HID: nintendo-switch2-usb: race in
  switch2_usb_message_in_work() on shutdown
--

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

HID: nintendo-switch2-usb: Improve shutdown logic with inbound message work

This commit moves the schedule_work() call for message_in_work under
bulk_in_lock in switch2_bulk_in() and adds a shutdown check at the start
of switch2_usb_message_in_work().

Link: https://lore.kernel.org/linux-input/a2dc00e7-9cab-4315-87a4-795f256c1e03@endrift.com/T/

> diff --git a/drivers/hid/hid-nintendo-switch2-usb.c b/drivers/hid/hid-nintendo-switch2-usb.c
> index 8eafc4fc513d4..97b7bca790f8e 100644
> --- a/drivers/hid/hid-nintendo-switch2-usb.c
> +++ b/drivers/hid/hid-nintendo-switch2-usb.c

[ ... ]

> @@ -294,6 +287,10 @@ static void switch2_usb_message_in_work(struct work_struct *work)
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
> +	if (ns2_usb->shutdown) {
> +		spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
> +		return;
> +	}
>  	for (i = 0; i < NS2_IN_URBS; i++) {
>  		urb = &ns2_usb->bulk_in[i];
>  		if (urb->state != NS2_URB_IN)

[Severity: High]
Can switch2_usb_message_in_work() still race with teardown and shutdown
once it drops bulk_in_lock inside the loop?

CPU0 (kworker)                        CPU1 (disconnect / shutdown)
switch2_usb_message_in_work()
  spin_lock_irqsave(bulk_in_lock)
  if (ns2_usb->shutdown) // false
  spin_unlock_irqrestore(bulk_in_lock)
                                      switch2_usb_disconnect()
                                        switch2_usb_free_in_urbs()
                                          cancel_work(&message_in_work)
                                          spin_lock_irqsave(bulk_in_lock)
                                          bulk_in[i].urb = NULL
                                          spin_unlock_irqrestore(...)
                                          usb_free_coherent(..., data, ...)
                                          usb_free_urb(urb)
  switch2_receive_command(...,
    urb->urb->transfer_buffer, ...)
    <-- NULL deref or use-after-free

switch2_usb_free_in_urbs() and switch2_usb_shutdown() call non-blocking
cancel_work(), and switch2_usb_disconnect() only calls cancel_work_sync()
after switch2_usb_free_in_urbs() and switch2_controller_detach_cfg() have
already completed.  Because an URB in state NS2_URB_IN has already
completed, usb_kill_anchored_urbs() and usb_kill_urb() do not wait for the
worker either, allowing bulk_in[i].urb, its coherent DMA buffer, and
ns2_usb->cfg.parent to be cleared and freed while bulk_in_lock is dropped.

In addition, when switch2_usb_message_in_work() re-acquires bulk_in_lock
after switch2_receive_command(), it only checks !urb->urb and never
re-checks ns2_usb->shutdown.  Because switch2_usb_shutdown() sets
ns2_usb->shutdown = true and empties bulk_in_anchor without clearing
bulk_in[i].urb, an in-flight worker will re-anchor and resubmit urb->urb
via usb_submit_urb() and continue iterating after shutdown.

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