[PATCH] drivers/xillybus: fix deadlock upon cleanup_dev

[PATCH] drivers/xillybus: fix deadlock upon cleanup_dev

[Sabyrzhan Tasbolatov]

syzbot found an issue [1] when cleanup_dev() is called twice,
causing deadlock. It is called in xillyusb_probe()
in the end of wakeup_all():

	INIT_WORK(&xdev->wakeup_workitem, wakeup_all);

then on "goto fail" path it's called again, where it's deadlocked in
destroy_workqueue(xdev->workq) due to first call's work_completion.

Also the deadlock prevents UAF as cleanup_dev frees struct xillyusb_dev.

It should be also the cause of previous syzbot report which was fixed in
commit 0d151a103775 ("Bluetooth: hci_core: cancel all works upon
hci_unregister_dev()").

[1]
WARNING: possible recursive locking detected
6.11.0-rc2-syzkaller-00004-gb446a2dae984 #0 Not tainted
--------------------------------------------
kworker/0:1H/58 is trying to acquire lock:
((wq_completion)xillyusb){+.+.}-{0:0},
((wq_completion)xillyusb){+.+.}-{0:0},

but task is already holding lock:
((wq_completion)xillyusb){+.+.}-{0:0},
((wq_completion)xillyusb){+.+.}-{0:0},

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock((wq_completion)xillyusb);
  lock((wq_completion)xillyusb);

 *** DEADLOCK ***

 May be due to missing lock nesting notation

2 locks held by kworker/0:1H/58:
 #0: ((wq_completion)xillyusb){+.+.}-{0:0},
 #0: ((wq_completion)xillyusb){+.+.}-{0:0},
 #1: ((work_completion)(&xdev->wakeup_workitem)){+.+.}-{0:0},
 #1: ((work_completion)(&xdev->wakeup_workitem)){+.+.}-{0:0},

Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
Reported-by: syzbot+e528c9aad0fb5383ec83@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e528c9aad0fb5383ec83
Fixes: a53d1202aef1 ("char: xillybus: Add driver for XillyUSB (Xillybus variant for USB)")
---
 drivers/char/xillybus/xillyusb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
index 5a5afa14ca8c..099fe681aed5 100644
--- a/drivers/char/xillybus/xillyusb.c
+++ b/drivers/char/xillybus/xillyusb.c
@@ -2151,6 +2151,7 @@ static int xillyusb_probe(struct usb_interface *interface,
 	if (!xdev->workq) {
 		dev_err(&interface->dev, "Failed to allocate work queue\n");
 		rc = -ENOMEM;
+		kref_put(&xdev->kref, cleanup_dev);
 		goto fail;
 	}
 
@@ -2174,7 +2175,6 @@ static int xillyusb_probe(struct usb_interface *interface,
 
 fail:
 	usb_set_intfdata(interface, NULL);
-	kref_put(&xdev->kref, cleanup_dev);
 	return rc;
 }
 
-- 
2.34.1

Re: [PATCH] drivers/xillybus: fix deadlock upon cleanup_dev

[Eli Billauer]

Hello,

I should have sent a response to this syzbot alert, indicating that 
there is already ongoing work to fix this issue:

https://lore.kernel.org/lkml/20240801121126.60183-1-eli.billauer@gmail.com/

My apologies for not doing that.

On 13/08/2024 7:04, Sabyrzhan Tasbolatov wrote:
> syzbot found an issue [1] when cleanup_dev() is called twice,
> causing deadlock.

How is cleanup_dev() called twice? I only see it once in the stack trace.

  It is called in xillyusb_probe()
> in the end of wakeup_all():
> 
> 	INIT_WORK(&xdev->wakeup_workitem, wakeup_all);

INIT_WORK merely initializes the work item, it doesn't cause its execution.

> @@ -2174,7 +2175,6 @@ static int xillyusb_probe(struct usb_interface *interface,
>   
>   fail:
>   	usb_set_intfdata(interface, NULL);
> -	kref_put(&xdev->kref, cleanup_dev);
>   	return rc;
>   }
>   

This edit causes a memory leak, because the reference count needs to be 
decremented in other failure scenarios.

Thanks,
    Eli