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

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@163.com>
---
 drivers/usb/storage/usb.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.34.1

On Thu, Mar 13, 2025 at 7:28 PM daixin_tkzc <daixin_tkzc@163.com> wrote:
>
> When the urb complete (babble error occurs), the sg_complete function of urb(s) will notify the mass storage driver that the data phase of the BOT transfer is over. The rest is done by the mass storage driver, such us:

You appear very focused on a specific sequence of events which causes
the babble error, but we are telling you that you are looking in the
wrong place.

If the DWC_otc driver does, in fact, handle packet babble properly,
then it will never overflow the buffer.

For example, forget the specifics of usb-storage.  Consider a BULK IN
request to an arbitrary device with an URB that provides an iobuf of
only 32 bytes, but the device sends 512 bytes -- the reason the device
sends too much data is not important; this is a babble condition.  The
controller and controller driver is *required* NOT to overflow the
32-byte buffer.  The remaining bytes received by the host are required
to be discarded.

Thus, even when a usb-storage device gets "out of sync" (i.e. is
sending data instead of a CSW), a buffer overflow is NOT POSSIBLE if
the controller is functioning properly.  If the controller writes data
beyond the end of the buffer, then that is an error of the controller
and/or controller driver software.  The design of the Linux USB stack
places this requirement on the controllers.

Matt

On Fri, Mar 14, 2025 at 10:28:41AM +0800, daixin_tkzc wrote:
> I'm sorry for not making my point clear. 

<snip>

Sorry, please do not send html email to the mailing lists.  It causes
your emails to be rejected.

Also please do not top-post.

thanks,

greg k-h

On Fri, Mar 14, 2025 at 10:28:41AM +0800, daixin_tkzc wrote:
> How us->iobuf overflow could occur?
> 
> For 1), the USB device does not know that a Babble Error has occurred at this time (DWC_otg knows what happened), It actually continuously returns 512 bytes data through DMA write to CSW address (As can be seen in the waveform in the appendix document before). The DWC_otg controller driver cannot control how much data the device returns(13 or 512 bytes). However, the USB storage driver pre-allocates a default buffer size of 64 bytes for CBW/CSW.

If this really is true, it is a bug in the dwc driver.  Please fix it
there, otherwise you will have to modify every single USB driver in
Linux to have a larger buffer size, not just the storage one.

thanks,

greg k-h

On Fri, Mar 14, 2025 at 10:28:41AM +0800, daixin_tkzc wrote:
> I'm sorry for not making my point clear. 
> 
> DWC_otg driver can handle packet babble in the data phase properly. It provides interrupt function, dwc2_hc_babble_intr, It mainly does two things:
> 
> 1) Delete the URB request from the endpoint linked list maintained by the USB host controller, mark the URB transfer result as OVERFLOW error, and delete the remaining URB requests in the data phase of the BOT transfer.
> 
> 2) Halt the currently used channel and indicate that the reason for the channel halt is Babble Error.
> 
> When the urb complete (babble error occurs), the sg_complete function of urb(s) will notify the mass storage driver that the data phase of the BOT transfer is over. The rest is done by the mass storage driver, such us:
> 
> 1) Get CSW for device status, interpret CSW, return USB_STOR_TRANSPORT_ERROR.
> 
> 2) Handle Errors(here is USB_STOR_TRANSPORT_ERROR).
> 
> 3) Initiate port reset.
> 
> 4) Notify the SCSI layer implements a retransmission mechanism.
> 
> How us->iobuf overflow could occur?
> 
> For 1), the USB device does not know that a Babble Error has occurred at this time (DWC_otg knows what happened), It actually continuously returns 512 bytes data through DMA write to CSW address (As can be seen in the waveform in the appendix document before). The DWC_otg controller driver cannot control how much data the device returns(13 or 512 bytes).

But the DWC_otg controller _can_ control how much data gets written to 
the URB's transfer buffer.

>  However, the USB storage driver pre-allocates a default buffer size of 64 bytes for CBW/CSW.

Furthermore, the CSW URB has a transfer_length of 13.  So the DWC_otg 
controller will ensure that no more than 13 bytes are written to the 
buffer, even if the device sends 512 bytes.  Right?

Alan Stern

> For 3), the device does not realize that a babble error has occurred until the port reset is successfully completed (by interface usb_stor_port_reset). Then device will enter the enumeration process. It looks like things are heading in the right direction.
> 
> For 4), as for the urb that has a babble error, SCSI will execute a retransmission mechanism.
> 
> thanks,
> 
> Dai xin
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> At 2025-03-13 22:36:32, "Alan Stern" <stern@rowland.harvard.edu> wrote:
> >On Thu, Mar 13, 2025 at 08:12:20PM +0800, daixin_tkzc wrote:
> >> 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
> >> 
> >> |
> >
> >What is your point?
> >
> >Are you claiming that the DWC_otg driver doesn't handle packet babble 
> >properly?  If that is true then you need to fix the DWC_otg driver, not 
> >change the usb-storage driver.
> >
> >You have not done a good job of explaining how us->iobuf overflow could 
> >occur.
> >
> >Alan Stern
> 
> -- 
> You received this message because you are subscribed to the Google Groups "USB Mass Storage on Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to usb-storage+unsubscribe@lists.one-eyed-alien.net.
> To view this discussion visit https://groups.google.com/a/lists.one-eyed-alien.net/d/msgid/usb-storage/1681f087.2727.195927b7ccb.Coremail.daixin_tkzc%40163.com.

On Sat, Mar 15, 2025 at 05:05:32PM +0800, daixin_tkzc wrote:
> 

Again, please do not send html email, it is rejected by the mailing
lists.

On Sat, Mar 15, 2025 at 2:05 AM daixin_tkzc <daixin_tkzc@163.com> wrote:
> Yes, dwc_otg should indeed ensure that DMA writes memory data no more than 13 bytes (even if Rxfifo receives 512 bytes).
> But in fact, the dwc_otg manual has different configuration requirements for the XferSize register before DMA transfer:
> For BOT's OUT transaction, the HCTSIZ register is filled with as many bytes as the software requests to send; for IN transactions, no matter how many bytes the software requests to receive, the HCTSIZ register needs to be filled with 512 alignment, that is, the software requests 31 bytes in the CBW phase, and HCTSIZ is filled with 31; the software requests 13 bytes in the CSW phase, and HCTSIZ is still filled with 512.

"Alignment" is not the same thing as "size".  A buffer can be 32 bytes
and aligned on a 4MByte boundary.  Hardware devices often impose
alignment requirements as it simplifies the logic required to access
the buffer.

> Please allow us to explain the reason behind changing the US_IOBUF_SIZE macro.
> 1) The macro comment says, “But Freecom needs a 64-byte buffer, so that's the
> size we'll allocate”. We thought that "Freecom" had a similar problem, otherwise a 32-byte buffer size would be enough.

Your reasoning is incorrect.  The "Freecom" device does NOT have a
babble problem.  The Freecom device uses a vendor-specific protocol
which requires a 64-byte buffer buffer for all of its command and
status transfers.  us->iobuf is used by multiple protocols for command
and status, including CB, CBI, BOT, Freecom, and others -- as such, it
needs to be the largest size required by any of those protocols.

Matt

On Sat, Mar 15, 2025 at 07:20:37PM +0800, daixin_tkzc wrote:
> I'm sorry you may have misunderstood me.
> 
> 
> HCTSIZ register only reflects the transfer size for the Host Channel (between host and device). The dwc_otg manual explains it as follows：
> Non-Scatter/Gather DMA Mode:
> Transfer Size (XferSize)
> For an OUT, this field is the number of data bytes the host sends 
> during the transfer.
> For an IN, this field is the buffer size that the application has 
> Reserved for the transfer. The application is expected to program 
> this field as an integer multiple of the maximum packet size for IN 
> transactions (periodic and non-periodic).

In that case, the dwc_otg driver needs to use a 512-byte bounce buffer.  

The driver must _guarantee_ that no more than 13 bytes will be written 
to the URB's transfer_buffer if the URB's transfer_length is 13.  If the 
hardware cannot provide this guarantee then the driver must work around 
the hardware's deficiencies.  That is how the kernel's USB API is 
designed.

Alan Stern