Re: [PATCH] USB: serial: keyspan: fix control urb double submission

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: keyspan: TOCTOU race on resend_cont reinstates closed ports
- [Medium] usb: serial: keyspan: 5ms busy-wait in atomic context inside URB completion callback
--

commit 69cc5a68fa9dd8c58b7e866f241254a8f999f052
Author: Ting-Han Hou <ue081723@gmail.com>
Subject: USB: serial: keyspan: fix control urb double submission

This patch fixes a control URB double submission by adding a ctrl_lock spinlock
to serialise the -EINPROGRESS status check and the usb_submit_urb() call.

Link: https://syzkaller.appspot.com/bug?extid=e69c25cf38a53d0cf64c

> --- a/drivers/usb/serial/keyspan.c
> +++ b/drivers/usb/serial/keyspan.c
> @@ -2323,12 +2334,15 @@ static int keyspan_usa49_send_setup(struct usb_serial *serial,
>  	dev_dbg(&port->dev, "%s - endpoint %x (%d)\n",
>  		__func__, usb_pipeendpoint(this_urb->pipe), device_port);
>  
> +	spin_lock_irqsave(&s_priv->ctrl_lock, flags);
> +
>  	/* Save reset port val for resend.
>  	   Don't overwrite resend for open/close condition. */
>  	if ((reset_port + 1) > p_priv->resend_cont)
>  		p_priv->resend_cont = reset_port + 1;

[Severity: High]
Does this code introduce a TOCTOU race that can inadvertently resurrect a closed
port?

Looking at usa49_glocont_callback() and keyspan_usa49_send_setup() in
drivers/usb/serial/keyspan.c, if a concurrent close happens:

CPU1 (usa49_glocont_callback)
    if (p_priv->resend_cont) {
        // lockless read returns 1, so reset_port = 0
        keyspan_usa49_send_setup(..., p_priv->resend_cont - 1);

CPU2 (keyspan_close thread)
    // inside keyspan_usa49_send_setup(..., 2)
    spin_lock_irqsave(&s_priv->ctrl_lock, flags);
    p_priv->resend_cont = 3;
    ...
    spin_unlock_irqrestore(&s_priv->ctrl_lock, flags);
    // eventually completes, resets p_priv->resend_cont to 0, port is closed

CPU1 (keyspan_usa49_send_setup with reset_port = 0)
    spin_lock_irqsave(&s_priv->ctrl_lock, flags);
    if ((reset_port + 1) > p_priv->resend_cont) // 1 > 0 is true
        p_priv->resend_cont = reset_port + 1;   // resend_cont is now 1

Could CPU1 erroneously overwrite resend_cont with a stale reset_port value,
undoing the port closure and resurrecting the TX/RX configuration when the URB
completes and the callback resubmits?

>  
>  	if (this_urb->status == -EINPROGRESS) {
> +		spin_unlock_irqrestore(&s_priv->ctrl_lock, flags);
>  		/*  dev_dbg(&port->dev, "%s - already writing\n", __func__); */
>  		mdelay(5);
>  		return -1;
>  	}

[Severity: Medium]
Can this mdelay(5) cause a 5ms busy-wait in atomic context?

If the shared glocont_urb is submitted by a concurrent thread just before
ctrl_lock is acquired here, this_urb->status will be -EINPROGRESS. The
usa49_glocont_callback() calls keyspan_usa49_send_setup() directly from the
atomic softirq completion context.

Will executing a 5ms busy-wait in this atomic context cause severe latency
spikes and violate kernel atomic context rules? This pattern appears identically
across other send_setup functions modified in this patch in
drivers/usb/serial/keyspan.c.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010171619.1709-1-ue081723@gmail.com?part=1