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

[Desnes Nunes <desnesn@redhat.com>]

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