[PATCH v2] usb: storage: Fix memory leak in USB bulk transport

[PATCH v2] usb: storage: Fix memory leak in USB bulk transport

[Desnes Nunes]

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()

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");
+
+				if (usb_stor_access_xfer_buf(buf,
+						US_BULK_CS_WRAP_LEN, srb, &sg,
+						&offset, TO_XFER_BUF) !=
+							US_BULK_CS_WRAP_LEN)
+					usb_stor_dbg(us, "Failed to clear CSW data\n");
+
 				scsi_set_resid(srb, transfer_length);
 				goto skipped_data_phase;
 			}
-- 
2.51.0

Re: [PATCH v2] usb: storage: Fix memory leak in USB bulk transport

[Alan Stern]

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.

> 
> 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.

> +
> +				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()?

> +					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

Re: [PATCH v2] usb: storage: Fix memory leak in USB bulk transport

[Desnes Nunes]

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