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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <516c8f89-45f2-4d3f-b1e7-29aecfc8cd3c@rowland.harvard.edu>
Date: Fri, 14 Mar 2025 10:16:48 -0400
From: Alan Stern <stern@...land.harvard.edu>
To: daixin_tkzc <daixin_tkzc@....com>
Cc: Greg KH <gregkh@...uxfoundation.org>,
	matthew dharm <mdharm-usb@...-eyed-alien.net>,
	linux-usb@...r.kernel.org, usb-storage@...ts.one-eyed-alien.net,
	linux-kernel@...r.kernel.org
Subject: Re: [usb-storage] Re:Re:[PATCH] usb: storage: Fix `us->iobuf` size
 for BOT transmission to prevent memory overflow

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@...land.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@...ts.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.