Re:[PATCH] usb: storage: Fix `us->iobuf` size for BOT transmission to prevent memory overflow

[daixin_tkzc]

Thank you for reviewing my patch.

I'm sorry I just responded individually.

Of course, when the USB device and host are transmitting normally, us->iobuf size is 64, which is enough for CBW/CSW and there will be no problem. 

Howerver, we encountered a problem in the FPGA verification environment, that is, the DWC otg controller detected a Babble Error, and we believe that the processing flow of the device driver will cause the risk of us->iobuf overflow. 

Regarding USB Babble Error, the DWC_otg_programming manual describes it as follows:

3.8.1 Handling Babble Conditions

DWC_otg handles two cases of babble: packet babble and port babble. Packet babble occurs if the device sends more data than the maximum packet size for the channel. Port babble occurs if the controller continues to receive data from the device at EOF2 (the end of frame 2, which is very close to SOF).

When DWC_otg detects a packet babble, it stops writing data into the Rx buffer and waits for the end of packet (EOP). When it detects an EOP, it flushes already-written data in the Rx buffer and generates a Babble interrupt to the application

When the usb_stor_bulk_transport interface calls usb_stor_bulk_srb for data transmission (such as the SCSI layer 120K data read request), the controller detects a Babble Error and enters the interrupt processing function dwc2_hc_babble_intr, which eventually parses the data stage transmission result of the BOT as USB_STOR_XFER_LONG, and also halts the transmission channel. However, the USB device does not know that a Babble Error has occurred at this time.

In order to ensure the integrity of the BOT transfer , the usb_stor_bulk_transfer_buf interface is called for status stage transmission(as the comments say: get CSW for device status), and an IN transfer transaction is requested: HCTSIZ register value of the host channel transmission is configured to be 512 bytes, and the DMA address is 0x7000_4000 (CBW/CSW address, see the appendix document). Under normal circumstances, the device should return 13 bytes.

However, what we observed is 512 bytes are actually returned (see the appendix document). According to our analysis, when a Babble Error occurs, the device will continuously return data.

According to the mass storage driver flow, software will parse the data header 13 bytes, which is not the expected CSW format. It is considered that the complete BOT transfer result is USB_STOR_XFER_ERROR, and the SCSI layer result is returned as DID_ERROR. The usb_stor_invoke_transport interface will initiate a port reset, that is, usb_stor_port_reset, to notify the device that a problem has occurred, then device will enter the enumeration process.

thanks,

Dai Xin

At 2025-03-11 16:41:11, "Xin Dai" <daixin_tkzc@xxxxxxx> wrote:

>When the DWC2 controller detects a packet Babble Error, where a device
>transmits more data over USB than the host controller anticipates for a
>transaction. It follows this process:
>
>1. The interrupt handler marks the transfer result of the URB as
>   `OVERFLOW` and returns it to the USB storage driver.
>2. The USB storage driver interprets the data phase transfer result of
>   the BOT (Bulk-Only Transport) as `USB_STOR_XFER_LONG`.
>3. The USB storage driver initiates the CSW (Command Status Wrapper)
>   phase of the BOT, requests an IN transaction, and retrieves the
>   execution status of the corresponding CBW (Command Block Wrapper)
>   command.
>4. The USB storage driver evaluates the CSW and finds it does not meet
>   expectations. It marks the entire BOT transfer result as
>   `USB_STOR_XFER_ERROR` and notifies the SCSI layer that a `DID_ERROR`
>   has occurred during the transfer.
>5. The USB storage driver requests the DWC2 controller to initiate a
>   port reset, notifying the device of an issue with the previous
>   transmission.
>6. The SCSI layer implements a retransmission mechanism.
>
>In step 3, the device remains unaware of the Babble Error until the
>connected port is reset. We observed that the device continues to send
>512 bytes of data to the host (according to the BBB Transport protocol,
>it should send only 13 bytes). However, the USB storage driver
>pre-allocates a default buffer size of 64 bytes for CBW/CSW, posing a
>risk of memory overflow. To mitigate this risk, we have adjusted the
>buffer size to 512 bytes to prevent potential errors.
>
>Signed-off-by: Xin Dai <daixin_tkzc@xxxxxxx>
>---
> drivers/usb/storage/usb.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/usb/storage/usb.h b/drivers/usb/storage/usb.h
>index 97c6196d639b..fd8dcb21137a 100644
>--- a/drivers/usb/storage/usb.h
>+++ b/drivers/usb/storage/usb.h
>@@ -71,7 +71,7 @@ struct us_unusual_dev {
>  * size we'll allocate.
>  */
>
>-#define US_IOBUF_SIZE		64	/* Size of the DMA-mapped I/O buffer */
>+#define US_IOBUF_SIZE		512	/* Size of the DMA-mapped I/O buffer */
> #define US_SENSE_SIZE		18	/* Size of the autosense data buffer */
>
> typedef int (*trans_cmnd)(struct scsi_cmnd *, struct us_data*);
>--
>2.34.1

Attachment: Appendix document.docx
Description: application/vnd.openxmlformats-officedocument.wordprocessingml.document