[PATCH] mmc: vub300: fix sleeping function called from invalid context

[Ömer Mete Kaya]

syzbot reports:

  BUG: sleeping function called from invalid context at kernel/workqueue.c:4487
  in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 0, name: swapper/1
  ...
   <IRQ>
   __might_resched
   __cancel_work_sync
   mmc_free_host+0x19/0x30 [drivers/mmc/core/host.c:700]
   call_timer_fn+0x192/0x5e0 [kernel/time/timer.c:1748]
   run_timer_softirq
   ...

vub300_inactivity_timer_expired() runs in softirq (timer) context.
When the USB interface had already gone away (->interface == NULL,
cleared by vub300_disconnect() or the probe() error path), the timer
handler dropped the object's last kref via
kref_put(&vub300->kref, vub300_delete). If that was the last
reference, vub300_delete() ran from softirq context and called
mmc_free_host(), which calls cancel_delayed_work_sync() - a sleeping
function, illegal from softirq/timer context.

Root cause: inactivity_timer is armed in probe() and continuously
re-armed via mod_timer(), but - unlike sg_transfer_timer, which is
explicitly deleted after each use - it is never stopped when the
device is torn down, so it can still fire after ->interface has
been cleared.

Fix this by decoupling inactivity_timer from the object's kref
entirely: drop the kref_get() taken on its behalf in probe(); make
vub300_inactivity_timer_expired() a no-op when ->interface is NULL
instead of dropping a reference; and in both vub300_disconnect() and
the probe() err_stop_io path, call
timer_delete_sync(&vub300->inactivity_timer) right after clearing
->interface and before the final kref_put(). Since ->interface is
already NULL at that point, any concurrently running timer instance
takes the no-op branch, so timer_delete_sync() is guaranteed to
return with the timer stopped for good - removing any race with the
final kref_put()/vub300_delete()/mmc_free_host(). Before this
patch, a successful probe() left two references on the kref (one
from kref_init(), one from the timer's kref_get()); after it, only
the initial kref_init() reference remains, matching the single
kref_put() in vub300_disconnect() and err_stop_io.

While auditing the driver for the same class of bug, also switch
sg_transfer_timer's two timer_delete() call sites (in
__command_read_data() and __command_write_data()) to
timer_delete_sync(), since usb_sg_wait() returning does not
guarantee a concurrently running vub300_sg_timed_out() has finished.
__command_write_data() additionally only deleted the timer on the
success path, leaking an armed timer on the cmd->error path; the
(now synchronous) delete is moved before that check so it always
runs.

Reported-by: syzbot+0e06aa1bdc6495bac24b@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=0e06aa1bdc6495bac24b
Fixes: 88095e7b473a ("mmc: Add new VUB300 USB-to-SD/SDIO/MMC driver")
Signed-off-by: Ömer Mete Kaya <omermetekaya0@xxxxxxxxx>
---
 drivers/mmc/host/vub300.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/vub300.c b/drivers/mmc/host/vub300.c
index 2dae474dcd06..def8c7a29e91 100644
--- a/drivers/mmc/host/vub300.c
+++ b/drivers/mmc/host/vub300.c
@@ -744,7 +744,7 @@ static void vub300_inactivity_timer_expired(struct timer_list *t)
 	struct vub300_mmc_host *vub300 = timer_container_of(vub300, t,
 							    inactivity_timer);
 	if (!vub300->interface) {
-		kref_put(&vub300->kref, vub300_delete);
+		/* Intentional no-op; see commit message. */
 	} else if (vub300->cmd) {
 		mod_timer(&vub300->inactivity_timer, jiffies + HZ);
 	} else {
@@ -1453,7 +1453,8 @@ static int __command_read_data(struct vub300_mmc_host *vub300,
 						  (linear_length / 16384));
 			add_timer(&vub300->sg_transfer_timer);
 			usb_sg_wait(&vub300->sg_request);
-			timer_delete(&vub300->sg_transfer_timer);
+			/* Sync variant needed; see commit message. */
+			timer_delete_sync(&vub300->sg_transfer_timer);
 			if (vub300->sg_request.status < 0) {
 				cmd->error = vub300->sg_request.status;
 				data->bytes_xfered = 0;
@@ -1570,10 +1571,11 @@ static int __command_write_data(struct vub300_mmc_host *vub300,
 							   linear_length / 16384);
 			add_timer(&vub300->sg_transfer_timer);
 			usb_sg_wait(&vub300->sg_request);
+			/* Unconditional + sync; see commit message. */
+			timer_delete_sync(&vub300->sg_transfer_timer);
 			if (cmd->error) {
 				data->bytes_xfered = 0;
 			} else {
-				timer_delete(&vub300->sg_transfer_timer);
 				if (vub300->sg_request.status < 0) {
 					cmd->error = vub300->sg_request.status;
 					data->bytes_xfered = 0;
@@ -2327,7 +2329,7 @@ static int vub300_probe(struct usb_interface *interface,
 	INIT_WORK(&vub300->deadwork, vub300_deadwork_thread);
 	kref_init(&vub300->kref);
 	timer_setup(&vub300->sg_transfer_timer, vub300_sg_timed_out, 0);
-	kref_get(&vub300->kref);
+	/* No kref for inactivity_timer; see commit message. */
 	timer_setup(&vub300->inactivity_timer,
 		    vub300_inactivity_timer_expired, 0);
 	vub300->inactivity_timer.expires = jiffies + HZ;
@@ -2350,6 +2352,8 @@ static int vub300_probe(struct usb_interface *interface,
 
 err_stop_io:
 	vub300->interface = NULL;
+	/* Must precede kref_put(); see commit message. */
+	timer_delete_sync(&vub300->inactivity_timer);
 	kref_put(&vub300->kref, vub300_delete);
 
 	return retval;
@@ -2384,6 +2388,8 @@ static void vub300_disconnect(struct usb_interface *interface)
 			usb_set_intfdata(interface, NULL);
 			/* prevent more I/O from starting */
 			vub300->interface = NULL;
+			/* Must precede kref_put(); see commit message. */
+			timer_delete_sync(&vub300->inactivity_timer);
 			mmc_remove_host(mmc);
 			kref_put(&vub300->kref, vub300_delete);
 			pr_info("USB vub300 remote SDIO host controller[%d]"
-- 
2.55.0