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

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>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CACaw+eyU8jDVibLk4QQX54D6y_-owByXpo9w0p_dk7Wv7hE4iQ@mail.gmail.com>
Date: Thu, 30 Oct 2025 18:20:40 -0300
From: Desnes Nunes <desnesn@...hat.com>
To: Alan Stern <stern@...land.harvard.edu>
Cc: linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org, 
	gregkh@...uxfoundation.org, stable@...r.kernel.org
Subject: Re: [PATCH 1/2] usb: storage: Fix memory leak in USB bulk transport

Hello Alan,

On Thu, Oct 30, 2025 at 10:52 AM Alan Stern <stern@...land.harvard.edu> wrote:
>
> 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@...hat.com> wrote:
> > >
> > > Hello Alan,
> > >
> > > On Wed, Oct 29, 2025 at 6:49 PM Alan Stern <stern@...land.harvard.edu> 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@...r.kernel.org
> > > > > Signed-off-by: Desnes Nunes <desnesn@...hat.com>
> > > > > ---
> > > > >  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

Thanks for taking the time to review my concerns.

Since this patch is using the style of the patch 2/2 patch of this
series, which I'll drop, I'll send a v2 of this patch using the
current code style.

Desnes Nunes