ene_ub6250_probe() reads the card type with ene_get_card_type() right after usb_stor_probe2() has added the SCSI host. The query is a bulk-only transaction (CBW, one-byte data-in, CSW) sent through ene_send_scsi_cmd(), which uses usb_stor_bulk_transfer_buf() and therefore waits for URB completion without any timeout. That is safe only for transfers belonging to a SCSI command, where the command's timeout and abort handling terminate the wait. A card-type query has no SCSI command behind it, so a device which enumerates but never services bulk transfers blocks the probe forever. commit 445fc368c6bc ("usb-storage: ene_ub6250: fix race between scan work and probe") added us->dev_mutex around the query to keep it from racing with the scan work for us->current_urb. usb_stor_probe2() queues that work with a one-second delay, so it now waits for a transfer which may never finish. The query itself has been unbounded since commit 41e568d14ec0 ("Staging: Merge ENE UB6250 SD card codes from keucr to drivers/usb/storage"), but only the latter turns the hang into the lockup reported here, hence the Fixes tag. syzbot reports: INFO: task kworker/0:2:53 blocked for more than 143 seconds. Workqueue: events_freezable usb_stor_scan_dwork Call Trace: __mutex_lock+0x7c4/0x15a0 usb_stor_scan_dwork+0xec/0x2c0 ... INFO: task syz-executor339:5675 blocked for more than 143 seconds. Call Trace: __mutex_lock+0x7c4/0x15a0 device_lock usbdev_open+0x151/0x760 with the hub worker holding both locks stalled in ene_ub6250_probe() -> ene_get_card_type() -> usb_stor_msg_common(). Run the query from ->initFunction instead. usb_stor_acquire_resources() calls it before the control thread is started and before the scan work is queued, so nothing else can be using us->current_urb and the dev_mutex from that commit can go away. A failed query now unwinds through usb_stor_probe2() before the SCSI host is registered, instead of going through usb_stor_disconnect() and cancel_delayed_work_sync() on a scan work which is itself stuck on the dead device. Also give each transfer of the query a 10-second timeout. A timeout at any stage fails the query immediately, so the probe cannot block forever. The retry chain can run five transfers at most (CBW, data, CSW and the two CSW retries), which bounds the query to about 50 seconds without keeping a shared budget. Reported-by: syzbot+30552b4cbe99d6d91306@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=30552b4cbe99d6d91306 Fixes: 445fc368c6bc ("usb-storage: ene_ub6250: fix race between scan work and probe") Cc: Liu Qi Cc: stable@vger.kernel.org # 7.3.x: e350e5b223e1: add usb_stor_bulk_transfer_buf_timeout() Signed-off-by: Hongyu Xie --- v5: - Split into a two-patch series: the transport helper first, the driver change second. - Replace the shared 10-second budget with a per-transfer timeout. A timeout in any stage fails the query immediately and the total time stays bounded, so the remaining-time bookkeeping is not needed. - State the worst-case bound of that choice (about 50 seconds, at most five transfers) in the message; record the transport helper as a stable prerequisite so the dependency is available there as well. - Shorten the comment above ENE_PROBE_TIMEOUT, as the reasoning is in ene_ub6250_init(). - Drop the unused initializer of the bulk pipe variable. - Keep the forward declaration of ene_ub6250_init(): the unusual-device table is built by including unusual_ene_ub6250.h before the function definition and takes its address, so removing the declaration fails to build; realtek_cr.c has the same declaration for init_realtek_cr(). v4: https://lore.kernel.org/all/20261109085449.83117-1-xiehongyu1@kylinos.cn/ v3: https://lore.kernel.org/r/20261001123135.38831-1-xy521521@gmail.com/ v2: https://lore.kernel.org/r/20261001091523.16934-1-xy521521@gmail.com/ v1: https://lore.kernel.org/r/20260929144640.2028-1-xy521521@gmail.com/ --- drivers/usb/storage/ene_ub6250.c | 110 +++++++++++++++-------- drivers/usb/storage/unusual_ene_ub6250.h | 2 +- 2 files changed, 73 insertions(+), 39 deletions(-) diff --git a/drivers/usb/storage/ene_ub6250.c b/drivers/usb/storage/ene_ub6250.c index fea2614e767c7..bf661d8e2c540 100644 --- a/drivers/usb/storage/ene_ub6250.c +++ b/drivers/usb/storage/ene_ub6250.c @@ -24,6 +24,9 @@ #define DRV_NAME "ums_eneub6250" +/* Timeout for each transfer of the probe-time card-type query, in jiffies. */ +#define ENE_PROBE_TIMEOUT (10 * HZ) + MODULE_DESCRIPTION("Driver for ENE UB6250 reader"); MODULE_LICENSE("GPL"); MODULE_IMPORT_NS("USB_STORAGE"); @@ -34,6 +37,8 @@ MODULE_FIRMWARE(MS_INIT_FIRMWARE); MODULE_FIRMWARE(MSP_RW_FIRMWARE); MODULE_FIRMWARE(MS_RW_FIRMWARE); +static int ene_ub6250_init(struct us_data *us); + /* * The table of devices */ @@ -487,7 +492,12 @@ static void ene_ub6250_info_destructor(void *extra) kfree(info->bbuf); } -static int ene_send_scsi_cmd(struct us_data *us, u8 fDir, void *buf, int use_sg) +/* + * Send an ENE SCSI command. @timeout bounds the wait for each transfer of + * the transaction, in jiffies; zero means no limit. + */ +static int ene_send_scsi_cmd_timeout(struct us_data *us, u8 fdir, void *buf, + int use_sg, unsigned int timeout) { struct bulk_cb_wrap *bcb = (struct bulk_cb_wrap *) us->iobuf; struct bulk_cs_wrap *bcs = (struct bulk_cs_wrap *) us->iobuf; @@ -499,27 +509,33 @@ static int ene_send_scsi_cmd(struct us_data *us, u8 fDir, void *buf, int use_sg) /* usb_stor_dbg(us, "transport --- ene_send_scsi_cmd\n"); */ /* send cmd to out endpoint */ - result = usb_stor_bulk_transfer_buf(us, us->send_bulk_pipe, - bcb, US_BULK_CB_WRAP_LEN, NULL); + result = usb_stor_bulk_transfer_buf_timeout(us, us->send_bulk_pipe, + bcb, US_BULK_CB_WRAP_LEN, + NULL, timeout); if (result != USB_STOR_XFER_GOOD) { usb_stor_dbg(us, "send cmd to out endpoint fail ---\n"); return USB_STOR_TRANSPORT_ERROR; } if (buf) { - unsigned int pipe = fDir; + unsigned int pipe; - if (fDir == FDIR_READ) + if (fdir == FDIR_READ) pipe = us->recv_bulk_pipe; else pipe = us->send_bulk_pipe; /* Bulk */ if (use_sg) { + /* + * Scatter-gather transfers belong to SCSI commands, + * whose timeout and abort machinery bound the wait. + */ result = usb_stor_bulk_srb(us, pipe, us->srb); } else { - result = usb_stor_bulk_transfer_sg(us, pipe, buf, - transfer_length, 0, &partial); + result = usb_stor_bulk_transfer_buf_timeout(us, pipe, + buf, transfer_length, + &partial, timeout); } if (result != USB_STOR_XFER_GOOD) { usb_stor_dbg(us, "data transfer fail ---\n"); @@ -528,20 +544,25 @@ static int ene_send_scsi_cmd(struct us_data *us, u8 fDir, void *buf, int use_sg) } /* Get CSW for device status */ - result = usb_stor_bulk_transfer_buf(us, us->recv_bulk_pipe, bcs, - US_BULK_CS_WRAP_LEN, &cswlen); + result = usb_stor_bulk_transfer_buf_timeout(us, us->recv_bulk_pipe, + bcs, US_BULK_CS_WRAP_LEN, + &cswlen, timeout); if (result == USB_STOR_XFER_SHORT && cswlen == 0) { usb_stor_dbg(us, "Received 0-length CSW; retrying...\n"); - result = usb_stor_bulk_transfer_buf(us, us->recv_bulk_pipe, - bcs, US_BULK_CS_WRAP_LEN, &cswlen); + result = usb_stor_bulk_transfer_buf_timeout(us, + us->recv_bulk_pipe, bcs, + US_BULK_CS_WRAP_LEN, + &cswlen, timeout); } if (result == USB_STOR_XFER_STALLED) { /* get the status again */ usb_stor_dbg(us, "Attempting to get CSW (2nd try)...\n"); - result = usb_stor_bulk_transfer_buf(us, us->recv_bulk_pipe, - bcs, US_BULK_CS_WRAP_LEN, NULL); + result = usb_stor_bulk_transfer_buf_timeout(us, + us->recv_bulk_pipe, bcs, + US_BULK_CS_WRAP_LEN, NULL, + timeout); } if (result != USB_STOR_XFER_GOOD) @@ -567,6 +588,15 @@ static int ene_send_scsi_cmd(struct us_data *us, u8 fDir, void *buf, int use_sg) return USB_STOR_TRANSPORT_GOOD; } +/* + * Unbounded variant for the command path: a SCSI command's timeout and + * abort machinery terminate the transfer if the device stops responding. + */ +static int ene_send_scsi_cmd(struct us_data *us, u8 fdir, void *buf, int use_sg) +{ + return ene_send_scsi_cmd_timeout(us, fdir, buf, use_sg, 0); +} + static int do_scsi_request_sense(struct us_data *us, struct scsi_cmnd *srb) { struct ene_ub6250_info *info = (struct ene_ub6250_info *) us->extra; @@ -1824,7 +1854,8 @@ static int ms_scsi_write(struct us_data *us, struct scsi_cmnd *srb) * ENE MS Card */ -static int ene_get_card_type(struct us_data *us, u16 index, void *buf) +static int ene_get_card_type(struct us_data *us, u16 index, void *buf, + unsigned int timeout) { struct bulk_cb_wrap *bcb = (struct bulk_cb_wrap *) us->iobuf; int result; @@ -1837,7 +1868,7 @@ static int ene_get_card_type(struct us_data *us, u16 index, void *buf) bcb->CDB[2] = (unsigned char)(index>>8); bcb->CDB[3] = (unsigned char)index; - result = ene_send_scsi_cmd(us, FDIR_READ, buf, 0); + result = ene_send_scsi_cmd_timeout(us, FDIR_READ, buf, 0, timeout); return result; } @@ -2191,7 +2222,7 @@ static int ene_init(struct us_data *us) struct ene_ub6250_info *info = (struct ene_ub6250_info *)(us->extra); u8 *bbuf = info->bbuf; - result = ene_get_card_type(us, REG_CARD_STATUS, bbuf); + result = ene_get_card_type(us, REG_CARD_STATUS, bbuf, 0); if (result != USB_STOR_XFER_GOOD) return USB_STOR_TRANSPORT_ERROR; @@ -2319,13 +2350,35 @@ static int ene_transport(struct scsi_cmnd *srb, struct us_data *us) static struct scsi_host_template ene_ub6250_host_template; +/* + * Probe-time card-type query. It runs from usb_stor_acquire_resources(), + * before the usb-storage control thread and the SCSI scan work exist, so + * nothing else can be using us->current_urb. Give every transfer a + * timeout: there is no SCSI command whose timeout or abort handling could + * end the wait, and a timeout at any stage fails the whole query. + */ +static int ene_ub6250_init(struct us_data *us) +{ + struct ene_ub6250_info *info = (struct ene_ub6250_info *)us->extra; + int result; + + result = ene_get_card_type(us, REG_CARD_STATUS, info->bbuf, + ENE_PROBE_TIMEOUT); + if (result != USB_STOR_XFER_GOOD) + return -EIO; + + if (!(info->bbuf[0] & 0x01)) + pr_info("ums_eneub6250: SM cards are not supported\n"); + + return 0; +} + static int ene_ub6250_probe(struct usb_interface *intf, const struct usb_device_id *id) { - int result; - u8 misc_reg03; struct us_data *us; struct ene_ub6250_info *info; + int result; result = usb_stor_probe1(&us, intf, id, (id - ene_ub6250_usb_ids) + ene_ub6250_unusual_dev_list, @@ -2352,26 +2405,7 @@ static int ene_ub6250_probe(struct usb_interface *intf, us->transport_reset = usb_stor_Bulk_reset; us->max_lun = 0; - result = usb_stor_probe2(us); - if (result) - return result; - - /* probe card type */ - mutex_lock(&us->dev_mutex); - result = ene_get_card_type(us, REG_CARD_STATUS, info->bbuf); - mutex_unlock(&us->dev_mutex); - if (result != USB_STOR_XFER_GOOD) { - usb_stor_disconnect(intf); - return USB_STOR_TRANSPORT_ERROR; - } - - misc_reg03 = info->bbuf[0]; - if (!(misc_reg03 & 0x01)) { - pr_info("ums_eneub6250: This driver only supports SD/MS cards. " - "It does not support SM cards.\n"); - } - - return result; + return usb_stor_probe2(us); } diff --git a/drivers/usb/storage/unusual_ene_ub6250.h b/drivers/usb/storage/unusual_ene_ub6250.h index a3b32abc2b2ff..bc9d7ea4e398b 100644 --- a/drivers/usb/storage/unusual_ene_ub6250.h +++ b/drivers/usb/storage/unusual_ene_ub6250.h @@ -5,6 +5,6 @@ UNUSUAL_DEV(0x0cf2, 0x6250, 0x0000, 0x9999, "ENE", "ENE UB6250 reader", - USB_SC_DEVICE, USB_PR_DEVICE, NULL, 0), + USB_SC_DEVICE, USB_PR_DEVICE, ene_ub6250_init, 0), #endif /* defined(CONFIG_USB_STORAGE_ENE_UB6250) || ... */ -- 2.25.1