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

[Ulf Hansson]

On Sat, Sep 19, 2026 at 12:06 PM Nguyen Ngoc Thang
<ngocthang2710.1999@xxxxxxxxx> wrote:
>
> 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(-)

May I suggest that you split this into separate patches. For the core
and for the rtsx_usb driver. Other than that, this seems reasonable to
me.

In fact, we should probably have something similar for mmc, as
currently it's the mmc host driver responsibility to manage this
timeout itself.

Kind regards
Uffe

>
> 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
>