Re: [PATCH 1/2] usb: storage: Fix memory leak in USB bulk transport

[Alan Stern]

On Thu, Oct 30, 2025 at 01:42:43AM -0300, Desnes Nunes wrote:
> Hello Alan,
> 
> On Wed, Oct 29, 2025 at 9:36 PM Desnes Nunes <desnesn@xxxxxxxxxx> wrote:
> >
> > Hello Alan,
> >
> > On Wed, Oct 29, 2025 at 6:49 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Wed, Oct 29, 2025 at 04:14:13PM -0300, Desnes Nunes wrote:
> > > > A kernel memory leak was identified by the 'ioctl_sg01' test from Linux
> > > > Test Project (LTP). The following bytes were maily 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
> > > > leads 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.
> > > >
> > > > Fixes: a45b599ad808 ("scsi: sg: allocate with __GFP_ZERO in sg_build_indirect()")
> > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > Signed-off-by: Desnes Nunes <desnesn@xxxxxxxxxx>
> > > > ---
> > > >  drivers/usb/storage/transport.c | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > >
> > > > diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
> > > > index 1aa1bd26c81f..8e9f6459e197 100644
> > > > --- a/drivers/usb/storage/transport.c
> > > > +++ b/drivers/usb/storage/transport.c
> > > > @@ -1200,7 +1200,17 @@ 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];
> > >
> > > You don't have to define another buffer here.  bcs is still available
> > > and it is exactly the right size.
> > >
> > > Alan Stern
> >
> > Sure - will send a v2 using bcs instead of the new buffer.
> 
> Actually, my original strategy to avoid the leak was copying a new
> zeroed buf over srb's transfer_buffer, as soon as the skipped data
> phase was identified.
> 
> It is true that the cs wrapper is the right size, but bcs at this
> point contains validated CSW data, which is needed later in the code
> when handling the skipped_data_phase of the device.
> 
> I think zeroing 13 bytes of bcs at this point, instead of creating a
> new buffer, would delete USB protocol information that is necessary
> later in usb_stor_Bulk_transport().
> 
> Can you please elaborate on how I can zero srb's transfer buffer using
> bcs, but without zeroing bcs?
> I may be missing something.

You're right -- I completely missed the fact that bcs gets used later.  
All right, ignore that criticism; the patch is fine.

Alan Stern