Re: [PATCH 1/2] HID: nintendo-switch2-usb: Fix potential deadlock on teardown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] HID: nintendo-switch2-usb: UAF and NULL deref in
  switch2_usb_free_in_urbs()
- [High] HID: nintendo-switch2-usb: UAF on switch2_controller during cfg
  teardown
- [Medium] HID: nintendo-switch2-usb: URB resubmission after
  switch2_usb_shutdown()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] HID: nintendo: missing usb_unanchor_urb() in
  switch2_usb_probe() [1]
--

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

HID: nintendo-switch2-usb: Fix potential deadlock on teardown

This commit replaces cancel_work_sync() on message_in_work with cancel_work()
in switch2_usb_free_in_urbs() and switch2_usb_shutdown(), and adds
cancel_work_sync() calls at the end of switch2_usb_probe() error handling and
switch2_usb_disconnect().

Link: https://sashiko.dev/#/message/20261006063000.A0B821F000FF%40smtp.kernel.org

> diff --git a/drivers/hid/hid-nintendo-switch2-usb.c b/drivers/hid/hid-nintendo-switch2-usb.c
> index 623473f3d2161..8eafc4fc513d4 100644
> --- a/drivers/hid/hid-nintendo-switch2-usb.c
> +++ b/drivers/hid/hid-nintendo-switch2-usb.c
> @@ -217,7 +217,7 @@ static void switch2_usb_free_in_urbs(struct switch2_usb *ns2_usb)
>  	ns2_usb->shutdown = true;
>  	spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
>  
> -	cancel_work_sync(&ns2_usb->message_in_work);
> +	cancel_work(&ns2_usb->message_in_work);
>  
>  	usb_kill_anchored_urbs(&ns2_usb->bulk_in_anchor);
>  	for (i = 0; i < NS2_IN_URBS; i++) {

[Severity: High]
Can deferring cancel_work_sync(&ns2_usb->message_in_work) until after
switch2_usb_free_in_urbs() cause a NULL pointer dereference or use-after-free
on bulk_in[i].urb and its transfer_buffer?

cancel_work() returns immediately if switch2_usb_message_in_work() is already
running on a worker CPU.  Once switch2_usb_message_in_work() drops
bulk_in_lock, switch2_usb_free_in_urbs() can clear bulk_in[i].urb and free the
coherent DMA buffer and URB while the worker is still using them:

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

In addition, switch2_usb_free_out_urbs() also runs before cancel_work_sync()
and clears bulk_out[i].urb and bulk_out[i].data while leaving
bulk_out[i].state as NS2_URB_FREE, so if switch2_receive_command() sends a
command via switch2_usb_send_cmd(), it can pick a freed slot and dereference a
NULL urb->data or urb->urb.

> @@ -276,7 +276,7 @@ static void switch2_usb_shutdown(struct switch2_cfg_intf *intf)
>  	ns2_usb->shutdown = true;
>  	spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
>  
> -	cancel_work_sync(&ns2_usb->message_in_work);
> +	cancel_work(&ns2_usb->message_in_work);
>  
>  	usb_kill_anchored_urbs(&ns2_usb->bulk_in_anchor);
>  	spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);

[Severity: Medium]
Can an in-flight switch2_usb_message_in_work() resubmit a bulk IN URB after
switch2_usb_shutdown() finishes?

drivers/hid/hid-nintendo-switch2-usb.c:switch2_usb_message_in_work() {
    ...
		spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
		if (!urb->urb)
			/* We're shutting down and this URB was freed */
			break;

		urb->state = NS2_URB_FREE;
		/*
		 * We want exactly one bulk in URB scheduled at a time, so only
		 * reschedule this immediately if nothing else is scheduled
		 * currently.
		 */
		if (!usb_anchor_empty(&ns2_usb->bulk_in_anchor))
			continue;

		usb_anchor_urb(urb->urb, &ns2_usb->bulk_in_anchor);
		err = usb_submit_urb(urb->urb, GFP_ATOMIC);
    ...
}

When switch2_usb_shutdown() runs while switch2_usb_message_in_work() is
blocked in switch2_receive_command(), cancel_work() does not wait for the work
item, and switch2_usb_shutdown() kills all anchored URBs while leaving
urb->urb non-NULL.  When switch2_usb_message_in_work() reacquires
bulk_in_lock, it only checks !urb->urb and usb_anchor_empty(), not
ns2_usb->shutdown, so it re-anchors and resubmits urb->urb after shutdown.
Note that follow-up commit "HID: nintendo-switch2-usb: Improve shutdown logic
with inbound message work" checks ns2_usb->shutdown at the start of
switch2_usb_message_in_work(), but still does not check it after reacquiring
bulk_in_lock here.

> @@ -447,6 +447,7 @@ static int switch2_usb_probe(struct usb_interface *intf, const struct usb_device
>  	switch2_usb_free_out_urbs(ns2_usb);
>  err_free_in:
>  	switch2_usb_free_in_urbs(ns2_usb);
> +	cancel_work_sync(&ns2_usb->message_in_work);
>  	devm_kfree(&intf->dev, ns2_usb);
>  
>  	return ret;

[Severity: Medium]
This problem wasn't introduced by this patch, but if usb_submit_urb() fails in
switch2_usb_probe() [1], does switch2_usb_free_in_urbs() hang forever in
usb_kill_anchored_urbs()?

drivers/hid/hid-nintendo-switch2-usb.c:switch2_usb_probe() {
    ...
	spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
	ns2_usb->bulk_in[0].state = NS2_URB_OUT;
	usb_anchor_urb(ns2_usb->bulk_in[0].urb, &ns2_usb->bulk_in_anchor);
	ret = usb_submit_urb(ns2_usb->bulk_in[0].urb, GFP_ATOMIC);
	spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);

	if (ret < 0)
		goto err_free_out;
    ...
}

Because bulk_in[0].urb is anchored before usb_submit_urb() and is not
unanchored on submission failure, usb_kill_urb() inside
usb_kill_anchored_urbs() returns immediately without unanchoring the
unsubmitted URB, leaving usb_kill_anchored_urbs() spinning in an infinite loop
waiting for bulk_in_anchor to become empty.

> @@ -466,6 +467,8 @@ static void switch2_usb_disconnect(struct usb_interface *intf)
>  		switch2_controller_detach_cfg(ns2_usb->cfg.parent);
>  
>  	switch2_usb_free_out_urbs(ns2_usb);
> +
> +	cancel_work_sync(&ns2_usb->message_in_work);
>  }
>  
>  #define SWITCH2_CONTROLLER(vend, prod) \

[Severity: High]
Can running switch2_controller_detach_cfg() (or the error path of
switch2_controller_attach_cfg()) before cancel_work_sync() cause a NULL
pointer dereference or use-after-free on struct switch2_controller?

Because switch2_usb_shutdown() no longer waits for switch2_usb_message_in_work()
to finish, switch2_controller_detach_cfg() can clear ns2->cfg->parent and drop
the last reference to ns2 while switch2_usb_message_in_work() is still running
or blocked on mutex_lock(&ns2->lock) inside switch2_receive_command():

CPU0                                  kworker
switch2_usb_disconnect()              switch2_usb_message_in_work()
  ...                                   if (ns2_usb->cfg.parent)
  switch2_controller_detach_cfg(ns2)      switch2_receive_command(ns2, ...)
    mutex_lock(&ns2->lock)
    ns2->cfg->shutdown(ns2->cfg)
      cancel_work(&message_in_work)
    ns2->cfg->parent = NULL
    mutex_unlock(&ns2->lock)
    ...
    kref_put(&ns2->refcount,
             switch2_kref_put)
      kfree(ns2)
                                            mutex_lock(&ns2->lock)
                                            <-- use-after-free
  cancel_work_sync(&message_in_work)

Similarly, if ns2->cfg->parent is cleared between the if (ns2_usb->cfg.parent)
check and the call to switch2_receive_command(ns2_usb->cfg.parent, ...),
switch2_receive_command() will dereference NULL at mutex_lock(&ns2->lock).

[1] https://sashiko.dev/#/bug/linux-278e82ab-e1db-4cb6-bba0-623a80fa475c

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