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

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

[Desnes Nunes]

This series mainly fixes an usb storage memory leak that was identified by
the LTP ioctl_sg01 test. In short, a big enough SG_IO request can trick a
device into sending a CSW status during the data phase, which will in turn
leak USB protocol data to user-space. Differently from the big leak that
also started with the US_BULK_CS_SIGN from CVE-2018-1000204, this only
happens after the allocation of sg pages for the srb transfer-buffer.

Desnes Nunes (2):
  usb: storage: Fix memory leak in USB bulk transport
  usb: storage: rearrange triple nested CSW data phase check

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

-- 
2.50.1

[PATCH 1/2] 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 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@vger.kernel.org
Signed-off-by: Desnes Nunes <desnesn@redhat.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];
+
+				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 1/2] usb: storage: Fix memory leak in USB bulk transport

[Alan Stern]

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@vger.kernel.org
> Signed-off-by: Desnes Nunes <desnesn@redhat.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

> +
> +				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 1/2] usb: storage: Fix memory leak in USB bulk transport

[Desnes Nunes]

Hello Alan,

On Wed, Oct 29, 2025 at 6:49 PM Alan Stern <stern@rowland.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@vger.kernel.org
> > Signed-off-by: Desnes Nunes <desnesn@redhat.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.

Thanks for the review.

-- 
Desnes Nunes

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

[Desnes Nunes]

Hello Alan,

On Wed, Oct 29, 2025 at 9:36 PM Desnes Nunes <desnesn@redhat.com> wrote:
>
> Hello Alan,
>
> On Wed, Oct 29, 2025 at 6:49 PM Alan Stern <stern@rowland.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@vger.kernel.org
> > > Signed-off-by: Desnes Nunes <desnesn@redhat.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.

Thanks & Regards,

-- 
Desnes Nunes

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@redhat.com> wrote:
> >
> > Hello Alan,
> >
> > On Wed, Oct 29, 2025 at 6:49 PM Alan Stern <stern@rowland.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@vger.kernel.org
> > > > Signed-off-by: Desnes Nunes <desnesn@redhat.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

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

[Desnes Nunes]

Hello Alan,

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

[PATCH 2/2] usb: storage: rearrange triple nested CSW data phase check

[Desnes Nunes]

This rearranges the triple nested CSW data phase if clause, in order to
make usb_stor_Bulk_transport() code more readlable. No functional change.

Signed-off-by: Desnes Nunes <desnesn@redhat.com>
---
 drivers/usb/storage/transport.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
index 96b81cf6adc7..3f2e1df5ad1e 100644
--- a/drivers/usb/storage/transport.c
+++ b/drivers/usb/storage/transport.c
@@ -1188,18 +1188,17 @@ int usb_stor_Bulk_transport(struct scsi_cmnd *srb, struct us_data *us)
 		 * check whether it really is a CSW.
 		 */
 		if (result == USB_STOR_XFER_SHORT &&
-				srb->sc_data_direction == DMA_FROM_DEVICE &&
-				transfer_length - scsi_get_resid(srb) ==
-					US_BULK_CS_WRAP_LEN) {
+		    srb->sc_data_direction == DMA_FROM_DEVICE &&
+		    transfer_length - scsi_get_resid(srb) == US_BULK_CS_WRAP_LEN) {
 			struct scatterlist *sg = NULL;
-			unsigned int offset = 0;
-
-			if (usb_stor_access_xfer_buf((unsigned char *) bcs,
-					US_BULK_CS_WRAP_LEN, srb, &sg,
-					&offset, FROM_XFER_BUF) ==
-						US_BULK_CS_WRAP_LEN &&
-					bcs->Signature ==
-						cpu_to_le32(US_BULK_CS_SIGN)) {
+			unsigned int offset = 0, buflen = 0;
+
+			buflen = usb_stor_access_xfer_buf((unsigned char *) bcs,
+						US_BULK_CS_WRAP_LEN, srb, &sg,
+						&offset, FROM_XFER_BUF);
+
+			if (buflen == US_BULK_CS_WRAP_LEN &&
+			    bcs->Signature == cpu_to_le32(US_BULK_CS_SIGN)) {
 				unsigned char buf[US_BULK_CS_WRAP_LEN];
 
 				sg = NULL;
-- 
2.50.1

Re: [PATCH 2/2] usb: storage: rearrange triple nested CSW data phase check

[Alan Stern]

On Wed, Oct 29, 2025 at 04:14:14PM -0300, Desnes Nunes wrote:
> This rearranges the triple nested CSW data phase if clause, in order to
> make usb_stor_Bulk_transport() code more readlable. No functional change.
> 
> Signed-off-by: Desnes Nunes <desnesn@redhat.com>
> ---
>  drivers/usb/storage/transport.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
> index 96b81cf6adc7..3f2e1df5ad1e 100644
> --- a/drivers/usb/storage/transport.c
> +++ b/drivers/usb/storage/transport.c
> @@ -1188,18 +1188,17 @@ int usb_stor_Bulk_transport(struct scsi_cmnd *srb, struct us_data *us)
>  		 * check whether it really is a CSW.
>  		 */
>  		if (result == USB_STOR_XFER_SHORT &&
> -				srb->sc_data_direction == DMA_FROM_DEVICE &&
> -				transfer_length - scsi_get_resid(srb) ==
> -					US_BULK_CS_WRAP_LEN) {
> +		    srb->sc_data_direction == DMA_FROM_DEVICE &&
> +		    transfer_length - scsi_get_resid(srb) == US_BULK_CS_WRAP_LEN) {

This change has nothing to do with the subject of the patch.  Please 
leave the code the way it was.

>  			struct scatterlist *sg = NULL;
> -			unsigned int offset = 0;
> -
> -			if (usb_stor_access_xfer_buf((unsigned char *) bcs,
> -					US_BULK_CS_WRAP_LEN, srb, &sg,
> -					&offset, FROM_XFER_BUF) ==
> -						US_BULK_CS_WRAP_LEN &&
> -					bcs->Signature ==
> -						cpu_to_le32(US_BULK_CS_SIGN)) {
> +			unsigned int offset = 0, buflen = 0;

It seems silly to initialize buflen to 0 when the very next statement is 
going to overwrite that value.

Also, "buflen" is not a good name for this variable, because the 
variable does not contain the length of a buffer.  Rather, it will 
contain the amount of data that got transferred by the 
usb_stor_access_xfer_buf() routine.  The following "if" statement then 
tests whether that amount is equal to the buffer length.

Alan Stern

> +
> +			buflen = usb_stor_access_xfer_buf((unsigned char *) bcs,
> +						US_BULK_CS_WRAP_LEN, srb, &sg,
> +						&offset, FROM_XFER_BUF);
> +
> +			if (buflen == US_BULK_CS_WRAP_LEN &&
> +			    bcs->Signature == cpu_to_le32(US_BULK_CS_SIGN)) {
>  				unsigned char buf[US_BULK_CS_WRAP_LEN];
>  
>  				sg = NULL;
> -- 
> 2.50.1
> 

Re: [PATCH 2/2] usb: storage: rearrange triple nested CSW data phase check

[Desnes Nunes]

Hello Alan,

On Wed, Oct 29, 2025 at 6:54 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, Oct 29, 2025 at 04:14:14PM -0300, Desnes Nunes wrote:
> > This rearranges the triple nested CSW data phase if clause, in order to
> > make usb_stor_Bulk_transport() code more readlable. No functional change.
> >
> > Signed-off-by: Desnes Nunes <desnesn@redhat.com>
> > ---
> >  drivers/usb/storage/transport.c | 21 ++++++++++-----------
> >  1 file changed, 10 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
> > index 96b81cf6adc7..3f2e1df5ad1e 100644
> > --- a/drivers/usb/storage/transport.c
> > +++ b/drivers/usb/storage/transport.c
> > @@ -1188,18 +1188,17 @@ int usb_stor_Bulk_transport(struct scsi_cmnd *srb, struct us_data *us)
> >                * check whether it really is a CSW.
> >                */
> >               if (result == USB_STOR_XFER_SHORT &&
> > -                             srb->sc_data_direction == DMA_FROM_DEVICE &&
> > -                             transfer_length - scsi_get_resid(srb) ==
> > -                                     US_BULK_CS_WRAP_LEN) {
> > +                 srb->sc_data_direction == DMA_FROM_DEVICE &&
> > +                 transfer_length - scsi_get_resid(srb) == US_BULK_CS_WRAP_LEN) {
>
> This change has nothing to do with the subject of the patch.  Please
> leave the code the way it was.
>
> >                       struct scatterlist *sg = NULL;
> > -                     unsigned int offset = 0;
> > -
> > -                     if (usb_stor_access_xfer_buf((unsigned char *) bcs,
> > -                                     US_BULK_CS_WRAP_LEN, srb, &sg,
> > -                                     &offset, FROM_XFER_BUF) ==
> > -                                             US_BULK_CS_WRAP_LEN &&
> > -                                     bcs->Signature ==
> > -                                             cpu_to_le32(US_BULK_CS_SIGN)) {
> > +                     unsigned int offset = 0, buflen = 0;
>
> It seems silly to initialize buflen to 0 when the very next statement is
> going to overwrite that value.
>
> Also, "buflen" is not a good name for this variable, because the
> variable does not contain the length of a buffer.  Rather, it will
> contain the amount of data that got transferred by the
> usb_stor_access_xfer_buf() routine.  The following "if" statement then
> tests whether that amount is equal to the buffer length.
>
> Alan Stern

I tried to borrow some code from usb storage protocol, but after these
observations I do agree it is not a good name here.
Nonetheless, I will drop this patch from v2 as requested.

Thanks for the review,

-- 
Desnes Nunes

Re: [PATCH 2/2] usb: storage: rearrange triple nested CSW data phase check

[Alan Stern]

On Wed, Oct 29, 2025 at 09:39:36PM -0300, Desnes Nunes wrote:
> Hello Alan,
> 
> On Wed, Oct 29, 2025 at 6:54 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Wed, Oct 29, 2025 at 04:14:14PM -0300, Desnes Nunes wrote:
> > > This rearranges the triple nested CSW data phase if clause, in order to
> > > make usb_stor_Bulk_transport() code more readlable. No functional change.
> > >
> > > Signed-off-by: Desnes Nunes <desnesn@redhat.com>
> > > ---
> > >  drivers/usb/storage/transport.c | 21 ++++++++++-----------
> > >  1 file changed, 10 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
> > > index 96b81cf6adc7..3f2e1df5ad1e 100644
> > > --- a/drivers/usb/storage/transport.c
> > > +++ b/drivers/usb/storage/transport.c
> > > @@ -1188,18 +1188,17 @@ int usb_stor_Bulk_transport(struct scsi_cmnd *srb, struct us_data *us)
> > >                * check whether it really is a CSW.
> > >                */
> > >               if (result == USB_STOR_XFER_SHORT &&
> > > -                             srb->sc_data_direction == DMA_FROM_DEVICE &&
> > > -                             transfer_length - scsi_get_resid(srb) ==
> > > -                                     US_BULK_CS_WRAP_LEN) {
> > > +                 srb->sc_data_direction == DMA_FROM_DEVICE &&
> > > +                 transfer_length - scsi_get_resid(srb) == US_BULK_CS_WRAP_LEN) {
> >
> > This change has nothing to do with the subject of the patch.  Please
> > leave the code the way it was.
> >
> > >                       struct scatterlist *sg = NULL;
> > > -                     unsigned int offset = 0;
> > > -
> > > -                     if (usb_stor_access_xfer_buf((unsigned char *) bcs,
> > > -                                     US_BULK_CS_WRAP_LEN, srb, &sg,
> > > -                                     &offset, FROM_XFER_BUF) ==
> > > -                                             US_BULK_CS_WRAP_LEN &&
> > > -                                     bcs->Signature ==
> > > -                                             cpu_to_le32(US_BULK_CS_SIGN)) {
> > > +                     unsigned int offset = 0, buflen = 0;
> >
> > It seems silly to initialize buflen to 0 when the very next statement is
> > going to overwrite that value.
> >
> > Also, "buflen" is not a good name for this variable, because the
> > variable does not contain the length of a buffer.  Rather, it will
> > contain the amount of data that got transferred by the
> > usb_stor_access_xfer_buf() routine.  The following "if" statement then
> > tests whether that amount is equal to the buffer length.
> >
> > Alan Stern
> 
> I tried to borrow some code from usb storage protocol, but after these
> observations I do agree it is not a good name here.
> Nonetheless, I will drop this patch from v2 as requested.

I didn't mean that the entire patch should be dropped, just the changes 
to the indentation of the first few lines.

As for the variable name, num_written or something like that would be 
preferable to buflen.  You can make up something better, or you can drop 
the entire patch -- your choice.

Alan Stern