[PATCH] memstick: core: reclaim the request before freeing a timed-out card

[Nguyen Ngoc Thang]

memstick_alloc_card() hands card->current_mrq to the host and waits 500 ms
for it. If the host is still busy, the wait times out, the card is freed,
but the host keeps its pointer to the freed request.

rtsx_usb_ms hits this easily: its handle_req work can block in USB
transfers for seconds. When it returns, it reads and writes the freed
request, including the retry path in memstick_next_req():

  BUG: KASAN: slab-use-after-free in rtsx_usb_ms_handle_req+0x17ff/0x1a00
  Read of size 1 by task kworker/1:3
  Workqueue: events rtsx_usb_ms_handle_req
  Allocated by task 1656:
   memstick_alloc_card
   memstick_check
  Freed by task 1656:
   memstick_alloc_card
   memstick_check

Add an optional host->cancel() hook that stops the host from using the
current request, and call it from a common wait helper on timeout, before
the request's owner can go away. rtsx_usb_ms implements it by draining its
work item. The helper also covers memstick_set_rw_addr() and both waits in
memstick_alloc_card().

Reported-by: syzbot+3ee5da0319ca17ef1f4e@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=3ee5da0319ca17ef1f4e
Fixes: 99451dceeb5f ("memstick: Add realtek USB memstick host driver")
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@xxxxxxxxx>
---
Testing: no hardware. A raw-gadget emulation of an RTS5129 (0bda:0129) on
dummy_hcd delays every bulk-IN reply by 550 ms (above the core's 500 ms wait,
below the driver's 600 ms USB timeout). On a KASAN kernel the unpatched tree
reproduces the report (same offset and alloc/free stacks); the patched tree
takes the timeout+cancel path (~0.6 s) with no KASAN.

I first considered a second unbounded wait_for_completion() instead of a hook,
but rtsx_usb_ms_request() skips scheduling once host->eject is set and
memstick_remove_host() flushes the workqueue, so that can deadlock.

Known limit: cancel waits for the worker's remaining retries (a few seconds
with a device that never answers) while memstick_check() holds host->lock.
Other hosts leave ->cancel NULL, so their behaviour is unchanged.

 drivers/memstick/core/memstick.c    | 22 ++++++++++++++++------
 drivers/memstick/host/rtsx_usb_ms.c |  8 ++++++++
 include/linux/memstick.h            |  2 ++
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
index e03989c4e99e..ea09a63286bb 100644
--- a/drivers/memstick/core/memstick.c
+++ b/drivers/memstick/core/memstick.c
@@ -359,6 +359,20 @@ static int h_memstick_set_rw_addr(struct memstick_dev *card,
 	}
 }
 
+/* On timeout the host may still hold current_mrq; make it let go first. */
+static void memstick_wait_req(struct memstick_dev *card)
+{
+	struct memstick_host *host = card->host;
+
+	if (wait_for_completion_timeout(&card->mrq_complete,
+					msecs_to_jiffies(500)))
+		return;
+
+	if (host->cancel)
+		host->cancel(host);
+	card->current_mrq.error = -ETIMEDOUT;
+}
+
 /**
  * memstick_set_rw_addr - issue SET_RW_REG_ADDR request and wait for it to
  *                        complete
@@ -370,9 +384,7 @@ int memstick_set_rw_addr(struct memstick_dev *card)
 {
 	card->next_request = h_memstick_set_rw_addr;
 	memstick_new_req(card->host);
-	if (!wait_for_completion_timeout(&card->mrq_complete,
-			msecs_to_jiffies(500)))
-		card->current_mrq.error = -ETIMEDOUT;
+	memstick_wait_req(card);
 
 	return card->current_mrq.error;
 }
@@ -405,9 +417,7 @@ static struct memstick_dev *memstick_alloc_card(struct memstick_host *host)
 
 		card->next_request = h_memstick_read_dev_id;
 		memstick_new_req(host);
-		if (!wait_for_completion_timeout(&card->mrq_complete,
-				msecs_to_jiffies(500)))
-			card->current_mrq.error = -ETIMEDOUT;
+		memstick_wait_req(card);
 
 		if (card->current_mrq.error)
 			goto err_out;
diff --git a/drivers/memstick/host/rtsx_usb_ms.c b/drivers/memstick/host/rtsx_usb_ms.c
index beadc389f15f..403144a39a53 100644
--- a/drivers/memstick/host/rtsx_usb_ms.c
+++ b/drivers/memstick/host/rtsx_usb_ms.c
@@ -551,6 +551,13 @@ static void rtsx_usb_ms_request(struct memstick_host *msh)
 		schedule_work(&host->handle_req);
 }
 
+static void rtsx_usb_ms_cancel(struct memstick_host *msh)
+{
+	struct rtsx_usb_ms *host = memstick_priv(msh);
+
+	cancel_work_sync(&host->handle_req);
+}
+
 static int rtsx_usb_ms_set_param(struct memstick_host *msh,
 		enum memstick_param param, int value)
 {
@@ -787,6 +794,7 @@ static int rtsx_usb_ms_drv_probe(struct platform_device *pdev)
 	INIT_DELAYED_WORK(&host->poll_card, rtsx_usb_ms_poll_card);
 
 	msh->request = rtsx_usb_ms_request;
+	msh->cancel = rtsx_usb_ms_cancel;
 	msh->set_param = rtsx_usb_ms_set_param;
 	msh->caps = MEMSTICK_CAP_PAR4;
 
diff --git a/include/linux/memstick.h b/include/linux/memstick.h
index 107bdcbedf79..e86f7f6e3e4b 100644
--- a/include/linux/memstick.h
+++ b/include/linux/memstick.h
@@ -285,6 +285,8 @@ struct memstick_host {
 
 	/* Notify the host that some requests are pending. */
 	void                (*request)(struct memstick_host *host);
+	/* Stop using the current request; may sleep until the host is idle. */
+	void                (*cancel)(struct memstick_host *host);
 	/* Set host IO parameters (power, clock, etc).     */
 	int                 (*set_param)(struct memstick_host *host,
 					 enum memstick_param param,
-- 
2.43.0