diff mbox series

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

Message ID 20250311084111.322351-1-daixin_tkzc@163.com (mailing list archive)
State New
Headers show
Series usb: storage: Fix `us->iobuf` size for BOT transmission to prevent memory overflow | expand

Commit Message

Xin Dai March 11, 2025, 8:41 a.m. UTC 
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

Comments

Greg KH March 11, 2025, 9:48 a.m. UTC | #1 
On Tue, Mar 11, 2025 at 04:41:11PM +0800, Xin Dai 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.

Where is this memory being overflowed?  I see it being used in the
usb_stor_CB_transport() call, should we just be checking the buffer size
there?

Your change just bumps the buffer up, it does not actually check any
tests for when the buffer is written to, which feels like it is not the
correct fix.  What's to prevent a device from sending a bigger message
to overflow it?

But again, where exactly is the overflow happening?

thanks,

greg k-h
Alan Stern March 11, 2025, 2:12 p.m. UTC | #2 
On Tue, Mar 11, 2025 at 04:41:11PM +0800, Xin Dai 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.

There is no risk of memory overflow.  The length of the transfer for the 
CSW is limited to US_BULK_CS_WRAP_LEN, which is 13.  And the length of a 
CBW transfer is limited to US_BULK_CB_WRAP_LEN, which is 31 (or to 32 
if the US_FL_BULK32 quirk flag is set).  Therefore a 64-byte buffer is 
more than enough.

Alan Stern
Matthew Dharm March 12, 2025, 1:09 a.m. UTC | #3 
On Tue, Mar 11, 2025 at 7:12 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Tue, Mar 11, 2025 at 04:41:11PM +0800, Xin Dai 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:
> >
> There is no risk of memory overflow.  The length of the transfer for the
> CSW is limited to US_BULK_CS_WRAP_LEN, which is 13.  And the length of a
> CBW transfer is limited to US_BULK_CB_WRAP_LEN, which is 31 (or to 32
> if the US_FL_BULK32 quirk flag is set).  Therefore a 64-byte buffer is
> more than enough.

There is no risk of memory overflow *unless* the DWC controller
doesn't respect the buffer length as given in the URB.  If there is an
overflow issue here, it is an issue with the controller level.
Matt
diff mbox series

Patch

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*);