Hello Alan,

On Thu, Oct 30, 2025 at 10:48 PM Alan Stern stern@rowland.harvard.edu wrote:

On Thu, Oct 30, 2025 at 06:48:33PM -0300, Desnes Nunes wrote:

...

A kernel memory leak was identified by the 'ioctl_sg01' test from Linux Test Project (LTP). The following bytes were mainly observed: 0x53425355.

When USB storage devices incorrectly skip the data phase with status data, the code extracts/validates the CSW from the sg buffer, but fails to clear it afterwards. This leaves status protocol data in srb's transfer buffer, such as the US_BULK_CS_SIGN 'USBS' signature observed here. Thus, this can lead to USB protocols leaks to user space through SCSI generic (/dev/sg*) interfaces, such as the one seen here when the LTP test requested 512 KiB.

Fix the leak by zeroing the CSW data in srb's transfer buffer immediately after the validation of devices that skip data phase.

Note: Differently from CVE-2018-1000204, which fixed a big leak by zero- ing pages at allocation time, this leak occurs after allocation, when USB protocol data is written to already-allocated sg pages.

v2: Use the same code style found on usb_stor_Bulk_transport()

Minor nit: The version information is supposed to go below the "---" line. It's not really part of the patch; when people in the future see this patch in the git history, they won't care how many previous versions it went through or what the changes were.

Noted and thanks for letting me know!

...

Fixes: a45b599ad808 ("scsi: sg: allocate with __GFP_ZERO in sg_build_indirect()") Cc: stable@vger.kernel.org Signed-off-by: Desnes Nunes desnesn@redhat.com

---

drivers/usb/storage/transport.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)

diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c index 1aa1bd26c81f..ee6b89f7f9ac 100644 --- a/drivers/usb/storage/transport.c +++ b/drivers/usb/storage/transport.c @@ -1200,7 +1200,19 @@ int usb_stor_Bulk_transport(struct scsi_cmnd *srb, struct us_data *us) US_BULK_CS_WRAP_LEN && bcs->Signature == cpu_to_le32(US_BULK_CS_SIGN)) {

•                         unsigned char buf[US_BULK_CS_WRAP_LEN];
  

•                         sg = NULL;
  

•                         offset = 0;
  

•                         memset(buf, 0, US_BULK_CS_WRAP_LEN);
                          usb_stor_dbg(us, "Device skipped data phase\n");
  

Another nit: Logically the comment belongs before the three new lines, because it notes that there was a problem whereas the new lines are part of the scheme to then mitigate the problem. It might also be worthwhile to add a comment explaining the reason for overwriting the CSW data, namely, to avoid leaking protocol information to userspace. This point is not immediately obvious.

Agree that it makes more sense to move the dbg comment before the declarations. Also concur that a comment about the fix of this leak is good to have in the code.

...

•                         if (usb_stor_access_xfer_buf(buf,
  

•                                         US_BULK_CS_WRAP_LEN, srb, &sg,
  

•                                         &offset, TO_XFER_BUF) !=
  

•                                                 US_BULK_CS_WRAP_LEN)
  

Yet another nit: Don't people recommend using sizeof(buf) instead of US_BULK_CS_WRAP_LEN in places like these? Particularly in memset()?

I wanted to make clear the size I was zeroing it, but it is literally a few lines above. Will change it to sizeof(buf).

...

•                                 usb_stor_dbg(us, "Failed to clear CSW data\n");
  

•                         scsi_set_resid(srb, transfer_length);
                          goto skipped_data_phase;
                  }
  

Regardless of the nits:

Reviewed-by: Alan Stern stern@rowland.harvard.edu

Alan Stern

v3 under the '---' is a charm!

Thanks for the review Alan.

Desnes Nunes