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

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

[Vicki Pfau]

As reported by Sashiko, there is a narrow window in which cancel_work_sync
on the message in work can block due to the work waiting on the mutex that
the canceling thread holds. We work around this by replacing it with a
non-sync cancel and a sync cancel later on, to ensure it's actually exited.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/message/20261006063000.A0B821F000FF%40smtp.kernel.org
Fixes: 9417132df63e ("HID: nintendo: Add preliminary Switch 2 controller driver")
Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-nintendo-switch2-usb.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-nintendo-switch2-usb.c b/drivers/hid/hid-nintendo-switch2-usb.c
index 8b3b6841035d..d747000336dc 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++) {
@@ -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);
@@ -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;
@@ -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) \
-- 
2.54.0

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

[Vicki Pfau]

When detaching the USB interface the driver marks the interface as shutdown
to prevent work that may be executing in parallel from attempting to use
the URBs before freeing the URBs. There is theoretically a very narrow
window in which the work could be scheduled while the shutdown is running
in a separate thread, starting to execute before the shutdown cancels the
work.

By ensuring the scheduling happens within the locked scope and adding an
additional shutdown check, this closes the window entirely.

Reported-by: Noah Weinberger <noahweinberger2003@proton.me>
Link: https://lore.kernel.org/linux-input/a2dc00e7-9cab-4315-87a4-795f256c1e03@endrift.com/T/
Fixes: 9417132df63e ("HID: nintendo: Add preliminary Switch 2 controller driver")
Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-nintendo-switch2-usb.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/hid-nintendo-switch2-usb.c b/drivers/hid/hid-nintendo-switch2-usb.c
index d747000336dc..5dba25c738e1 100644
--- a/drivers/hid/hid-nintendo-switch2-usb.c
+++ b/drivers/hid/hid-nintendo-switch2-usb.c
@@ -52,12 +52,10 @@ static void switch2_bulk_in(struct urb *urb)
 {
 	struct switch2_usb *ns2_usb = urb->context;
 	int i;
-	bool schedule = false;
 	unsigned long flags;
 
 	switch (urb->status) {
 	case 0:
-		schedule = true;
 		break;
 	case -ECONNRESET:
 	case -ENOENT:
@@ -71,16 +69,14 @@ static void switch2_bulk_in(struct urb *urb)
 	}
 
 	spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
-	if (ns2_usb->shutdown)
-		schedule = false;
-
 	for (i = 0; i < NS2_IN_URBS; i++) {
 		int err;
 		struct switch2_urb *ns2_urb;
 
 		if (ns2_usb->bulk_in[i].urb == urb) {
-			if (schedule) {
+			if (urb->status == 0 && !ns2_usb->shutdown) {
 				ns2_usb->bulk_in[i].state = NS2_URB_IN;
+				schedule_work(&ns2_usb->message_in_work);
 				continue;
 			} else {
 				ns2_usb->bulk_in[i].state = NS2_URB_FREE;
@@ -115,9 +111,6 @@ static void switch2_bulk_in(struct urb *urb)
 		}
 	}
 	spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
-
-	if (schedule)
-		schedule_work(&ns2_usb->message_in_work);
 }
 
 static void switch2_bulk_out(struct urb *urb)
@@ -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)
-- 
2.54.0

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

[sashiko-bot]

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

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

[sashiko-bot]

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