* [PATCH] USB: serial: keyspan: fix control urb double submission
@ 2026-10-10 17:16 Ting-Han Hou
0 siblings, 0 replies; only message in thread
From: Ting-Han Hou @ 2026-10-10 17:16 UTC (permalink / raw)
To: Johan Hovold
Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Ting-Han Hou,
syzbot+e69c25cf38a53d0cf64c, stable
The control urbs used to send port configuration messages can be
submitted concurrently from several contexts: open(), close(),
dtr_rts(), set_termios() and break_ctl() run in process context under
per-port or per-tty locks, while the outcont and glocont completion
handlers resubmit when a resend has been requested. For the USA-49 and
USA-67 message formats the control urb is also shared by all ports of
the device.
The send_setup() helpers check whether the urb is busy by comparing its
status with -EINPROGRESS without any locking, so two callers can both
find the urb idle and submit it. The second submission adds the urb to
the endpoint urb list a second time, which corrupts the list and can
lead to the host controller driver spinning forever with interrupts
disabled when the endpoint is later flushed on disconnect:
list_add double add: new=ffff88800ce63818, prev=ffff88800ce63818, next=ffff88800be7c078.
WARNING: lib/list_debug.c:35 at __list_add_valid_or_report+0x157/0x200
Call Trace:
<IRQ>
usb_hcd_link_urb_to_ep+0x1c6/0x330
dummy_urb_enqueue+0x21c/0x750
usb_hcd_submit_urb+0x228/0x1c00
keyspan_usa67_send_setup.isra.0+0x531/0x710
__usb_hcd_giveback_urb+0x241/0x400
dummy_timer+0x1402/0x3470
The urb->hcpriv check in usb_submit_urb() only catches a resubmission
after the urb has been queued and does not prevent this.
Serialise control message submission using a spinlock so that the busy
check and the submission are atomic with respect to other callers.
This was found while testing the reproducer for syzbot report
5fabc1ae99ff40690d84 (a keyspan_close() use-after-free) under QEMU,
which emulates a USA-28XG using raw-gadget and dummy_hcd. syzbot has
also reported the same corruption from keyspan_usa49_send_setup().
Tested under QEMU with that reproducer and DEBUG_LIST, KASAN and lockdep
enabled: the list corruption was hit in 4 out of 38 20-minute runs
without this patch and in none out of 36 runs with it. As the race is
hard to hit, this is an indication rather than proof. Only the USA-67
path is exercised by the reproducer; the other message formats have the
same pattern but have not been tested.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: syzbot+e69c25cf38a53d0cf64c@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=e69c25cf38a53d0cf64c
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Ting-Han Hou <ue081723@gmail.com>
---
drivers/usb/serial/keyspan.c | 29 ++++++++++++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/serial/keyspan.c b/drivers/usb/serial/keyspan.c
index 4d3746c7a94e..84a428fc102a 100644
--- a/drivers/usb/serial/keyspan.c
+++ b/drivers/usb/serial/keyspan.c
@@ -537,7 +537,7 @@ struct keyspan_serial_private {
struct urb *indat_urb;
char *indat_buf;
- /* XXX this one probably will need a lock */
+ spinlock_t ctrl_lock; /* serialises control urb submission */
struct urb *glocont_urb;
char *glocont_buf;
char *ctrl_buf; /* for EP0 control message */
@@ -2036,6 +2036,7 @@ static int keyspan_usa26_send_setup(struct usb_serial *serial,
struct keyspan_port_private *p_priv;
const struct keyspan_device_details *d_details;
struct urb *this_urb;
+ unsigned long flags;
int device_port, err;
dev_dbg(&port->dev, "%s reset=%d\n", __func__, reset_port);
@@ -2056,11 +2057,14 @@ static int keyspan_usa26_send_setup(struct usb_serial *serial,
dev_dbg(&port->dev, "%s - endpoint %x\n",
__func__, usb_pipeendpoint(this_urb->pipe));
+ 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;
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;
@@ -2169,6 +2173,7 @@ static int keyspan_usa26_send_setup(struct usb_serial *serial,
this_urb->transfer_buffer_length = sizeof(msg);
err = usb_submit_urb(this_urb, GFP_ATOMIC);
+ spin_unlock_irqrestore(&s_priv->ctrl_lock, flags);
if (err != 0)
dev_dbg(&port->dev, "%s - usb_submit_urb(setup) failed (%d)\n", __func__, err);
return 0;
@@ -2183,6 +2188,7 @@ static int keyspan_usa28_send_setup(struct usb_serial *serial,
struct keyspan_port_private *p_priv;
const struct keyspan_device_details *d_details;
struct urb *this_urb;
+ unsigned long flags;
int device_port, err;
s_priv = usb_get_serial_data(serial);
@@ -2197,11 +2203,14 @@ static int keyspan_usa28_send_setup(struct usb_serial *serial,
return -1;
}
+ 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;
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;
@@ -2287,6 +2296,7 @@ static int keyspan_usa28_send_setup(struct usb_serial *serial,
this_urb->transfer_buffer_length = sizeof(msg);
err = usb_submit_urb(this_urb, GFP_ATOMIC);
+ spin_unlock_irqrestore(&s_priv->ctrl_lock, flags);
if (err != 0)
dev_dbg(&port->dev, "%s - usb_submit_urb(setup) failed\n", __func__);
@@ -2303,6 +2313,7 @@ static int keyspan_usa49_send_setup(struct usb_serial *serial,
struct keyspan_port_private *p_priv;
const struct keyspan_device_details *d_details;
struct urb *this_urb;
+ unsigned long flags;
int err, device_port;
s_priv = usb_get_serial_data(serial);
@@ -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;
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;
@@ -2464,6 +2478,7 @@ static int keyspan_usa49_send_setup(struct usb_serial *serial,
this_urb->transfer_buffer_length = sizeof(msg);
}
err = usb_submit_urb(this_urb, GFP_ATOMIC);
+ spin_unlock_irqrestore(&s_priv->ctrl_lock, flags);
if (err != 0)
dev_dbg(&port->dev, "%s - usb_submit_urb(setup) failed (%d)\n", __func__, err);
@@ -2479,6 +2494,7 @@ static int keyspan_usa90_send_setup(struct usb_serial *serial,
struct keyspan_port_private *p_priv;
const struct keyspan_device_details *d_details;
struct urb *this_urb;
+ unsigned long flags;
int err;
u8 prescaler;
@@ -2493,11 +2509,14 @@ static int keyspan_usa90_send_setup(struct usb_serial *serial,
return -1;
}
+ 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;
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;
@@ -2595,6 +2614,7 @@ static int keyspan_usa90_send_setup(struct usb_serial *serial,
this_urb->transfer_buffer_length = sizeof(msg);
err = usb_submit_urb(this_urb, GFP_ATOMIC);
+ spin_unlock_irqrestore(&s_priv->ctrl_lock, flags);
if (err != 0)
dev_dbg(&port->dev, "%s - usb_submit_urb(setup) failed (%d)\n", __func__, err);
return 0;
@@ -2609,6 +2629,7 @@ static int keyspan_usa67_send_setup(struct usb_serial *serial,
struct keyspan_port_private *p_priv;
const struct keyspan_device_details *d_details;
struct urb *this_urb;
+ unsigned long flags;
int err, device_port;
s_priv = usb_get_serial_data(serial);
@@ -2626,11 +2647,14 @@ static int keyspan_usa67_send_setup(struct usb_serial *serial,
return -1;
}
+ 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;
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;
@@ -2738,6 +2762,7 @@ static int keyspan_usa67_send_setup(struct usb_serial *serial,
this_urb->transfer_buffer_length = sizeof(msg);
err = usb_submit_urb(this_urb, GFP_ATOMIC);
+ spin_unlock_irqrestore(&s_priv->ctrl_lock, flags);
if (err != 0)
dev_dbg(&port->dev, "%s - usb_submit_urb(setup) failed (%d)\n", __func__, err);
return 0;
@@ -2795,6 +2820,8 @@ static int keyspan_startup(struct usb_serial *serial)
if (!s_priv)
return -ENOMEM;
+ spin_lock_init(&s_priv->ctrl_lock);
+
s_priv->instat_buf = kzalloc(INSTAT_BUFLEN, GFP_KERNEL);
if (!s_priv->instat_buf)
goto err_instat_buf;
--
2.53.0
^ permalink raw reply [flat|nested] only message in thread
only message in thread, other threads:[~2026-10-10 17:16 UTC | newest]
Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-10 17:16 [PATCH] USB: serial: keyspan: fix control urb double submission Ting-Han Hou
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®