Re: [linux-usb-devel] 2.6.24: NULL scatter-gather pointer in usb_storage:usb_stor_access_xfer_buf?

Re: [linux-usb-devel] 2.6.24: NULL scatter-gather pointer in usb_storage:usb_stor_access_xfer_buf?

[Boaz Harrosh]

On Thu, Jan 31 2008 at 17:08 +0200, Mark Glines <mark-6pk7R1svBr8dnm+yROfE0A@public.gmane.org> wrote:
> On Thu, 31 Jan 2008 11:27:39 +0200
> Boaz Harrosh <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org> wrote:
> 
>> Please check the below patch.
>>
>> one thing that I can see is that the isd200 does an INQUARY transfer
>> of sizeof(struct inquiry_data) which is 96 bytes, when scsi_scan.c
>> sends an INQUARY with 36 bytes buffer. So we have an underflow in 
>> usb_stor_access_xfer_buf().
>>
>> The below patch will only check my theory. I will send a proper fix
>> later, please confirm that this fixes it.
>>
>> What kills me is that this condition has existed before my patch, I'll
>> try to see why it is triggered now
> 
> I applied this patch to 2.6.24, and it now works for me.  It was
> crashing consistently whenever I'd plug this device in, now it goes
> through successfully:
> 
Yes Thanks this is grate :)

I will send a proper patch to scsi maintainer. Alan is it OK to send this
patch threw James's scsi-misc?

> 
> [24775.788039] usb 3-2: new full speed USB device using uhci_hcd and address 3
> [24775.939275] usb 3-2: configuration #1 chosen from 1 choice
> [24776.084409] usbcore: registered new interface driver libusual
> [24776.103604] Initializing USB Mass Storage driver...
> [24776.213916] scsi3 : SCSI emulation for USB Mass Storage devices
> [24776.214366] usbcore: registered new interface driver usb-storage
> [24776.214377] USB Mass Storage support registered.
> [24776.215604] usb-storage: device found at 3
> [24776.215724] usb-storage: waiting for device to settle before scanning
> [24778.333378] scsi 3:0:0:0: Direct-Access     SAMSUNG  HM120JC          YL10 PQ: 0 ANSI: 0
> [24778.333715] sd 3:0:0:0: [sdb] 234441648 512-byte hardware sectors (120034 MB)
> [24778.333841] sd 3:0:0:0: [sdb] Write Protect is off
> [24778.333848] sd 3:0:0:0: [sdb] Mode Sense: 00 00 00 00
> [24778.333853] sd 3:0:0:0: [sdb] Assuming drive cache: write through
> [24778.334196] sd 3:0:0:0: [sdb] 234441648 512-byte hardware sectors (120034 MB)
> [24778.334396] sd 3:0:0:0: [sdb] Write Protect is off
> [24778.334403] sd 3:0:0:0: [sdb] Mode Sense: 00 00 00 00
> [24778.334408] sd 3:0:0:0: [sdb] Assuming drive cache: write through
> [24778.334414]  sdb: sdb1
> [24778.824103] sd 3:0:0:0: [sdb] Attached SCSI disk
> [24778.824210] sd 3:0:0:0: Attached scsi generic sg1 type 0
> [24778.825119] usb-storage: device scan complete
> 
> 
> I'm happy to test further patches.  Let me know if you need more
> testing.
> 
> Do you still want me to try out the scsi-misc branch?
> 
No, That was my mistake, scsi-misc is now identical to mainline.

This here is a new fix that will need to go in. I will send a patch
soonish. If you can test it and send a Tested-by: it could be grate

> Mark
> 
> 
>> ---
>>  drivers/usb/storage/protocol.c |    6 ++++++
>>  1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/usb/storage/protocol.c
>> b/drivers/usb/storage/protocol.c index a41ce21..d0ff1f6 100644
>> --- a/drivers/usb/storage/protocol.c
>> +++ b/drivers/usb/storage/protocol.c
>> @@ -229,6 +229,12 @@ void usb_stor_set_xfer_buf(unsigned char *buffer,
>>  	unsigned int offset = 0;
>>  	struct scatterlist *sg = NULL;
>>  
>> +	BUG_ON(!scsi_sglist(srb));
>> +
>> +	if(buflen > scsi_bufflen(srb))
>> +		buflen = scsi_bufflen(srb);
>> +		/*FIXME: should we set an underflow condition here*/
>> +
>>  	usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
>>  			TO_XFER_BUF);
>>  	if (buflen < scsi_bufflen(srb))
>>

Thanks Mark
(CCing linux-scsi ml)

Boaz

Re: [linux-usb-devel] 2.6.24: NULL scatter-gather pointer in usb_storage:usb_stor_access_xfer_buf?

[Alan Stern]

On Thu, 31 Jan 2008, Boaz Harrosh wrote:

> >> Please check the below patch.
> >>
> >> one thing that I can see is that the isd200 does an INQUARY transfer
> >> of sizeof(struct inquiry_data) which is 96 bytes, when scsi_scan.c
> >> sends an INQUARY with 36 bytes buffer. So we have an underflow in 
> >> usb_stor_access_xfer_buf().

Maybe the isd200 routine should be changed also, so that it doesn't try
to store too much data in the buffer.

> I will send a proper patch to scsi maintainer. Alan is it OK to send this
> patch threw James's scsi-misc?

You should send patches to Matt Dharm, since he is the usb-storage 
maintainer.

Alan Stern

[PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Boaz Harrosh]

  scsi_scan is issuing a 36-byte INQUIRY request to llds. isd200 would
  volunteer 96 bytes of INQUIRY. This caused an underflow condition in
  protocol.c usb_stor_access_xfer_buf(). So first fix is to
  usb_stor_access_xfer_buf() to properly handle underflow conditions.
  Then usb_stor_set_xfer_buf() should report this condition as a negative
  resid. Should we also set cmnd->status in the underflow condition?

  Then also isd200.c is fixed to only return the type of INQUIRY && SENSE
  the upper layer asked for.

Signed-off-by: Boaz Harrosh <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/storage/isd200.c   |    7 +++++--
 drivers/usb/storage/protocol.c |   13 +++++++++----
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
index 0db4886..4394930 100644
--- a/drivers/usb/storage/isd200.c
+++ b/drivers/usb/storage/isd200.c
@@ -1261,6 +1261,7 @@ static int isd200_scsi_to_ata(struct scsi_cmnd *srb, struct us_data *us,
 	unsigned long lba;
 	unsigned long blockCount;
 	unsigned char senseData[8] = { 0, 0, 0, 0, 0, 0, 0, 0 };
+	unsigned xfer_len;
 
 	memset(ataCdb, 0, sizeof(union ata_cdb));
 
@@ -1270,8 +1271,9 @@ static int isd200_scsi_to_ata(struct scsi_cmnd *srb, struct us_data *us,
 		US_DEBUGP("   ATA OUT - INQUIRY\n");
 
 		/* copy InquiryData */
+		xfer_len = min(sizeof(info->InquiryData), scsi_bufflen(srb));
 		usb_stor_set_xfer_buf((unsigned char *) &info->InquiryData,
-				sizeof(info->InquiryData), srb);
+					xfer_len, srb);
 		srb->result = SAM_STAT_GOOD;
 		sendToTransport = 0;
 		break;
@@ -1280,7 +1282,8 @@ static int isd200_scsi_to_ata(struct scsi_cmnd *srb, struct us_data *us,
 		US_DEBUGP("   ATA OUT - SCSIOP_MODE_SENSE\n");
 
 		/* Initialize the return buffer */
-		usb_stor_set_xfer_buf(senseData, sizeof(senseData), srb);
+		xfer_len = min(sizeof(senseData), scsi_bufflen(srb));
+		usb_stor_set_xfer_buf(senseData, xfer_len, srb);
 
 		if (info->DeviceFlags & DF_MEDIA_STATUS_ENABLED)
 		{
diff --git a/drivers/usb/storage/protocol.c b/drivers/usb/storage/protocol.c
index a41ce21..6200f62 100644
--- a/drivers/usb/storage/protocol.c
+++ b/drivers/usb/storage/protocol.c
@@ -175,7 +175,7 @@ unsigned int usb_stor_access_xfer_buf(unsigned char *buffer,
 		* and the starting offset within the page, and update
 		* the *offset and **sgptr values for the next loop. */
 	cnt = 0;
-	while (cnt < buflen) {
+	while (cnt < buflen && sg) {
 		struct page *page = sg_page(sg) +
 				((sg->offset + *offset) >> PAGE_SHIFT);
 		unsigned int poff =
@@ -228,9 +228,14 @@ void usb_stor_set_xfer_buf(unsigned char *buffer,
 {
 	unsigned int offset = 0;
 	struct scatterlist *sg = NULL;
+	unsigned int count;
 
-	usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
+	count = usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
 			TO_XFER_BUF);
-	if (buflen < scsi_bufflen(srb))
-		scsi_set_resid(srb, scsi_bufflen(srb) - buflen);
+
+	/* Check for underflow */
+	if (buflen > scsi_bufflen(srb))
+		count = buflen;
+
+	scsi_set_resid(srb, scsi_bufflen(srb) - count);
 }
-- 
1.5.3.3

Re: [linux-usb-devel] 2.6.24: NULL scatter-gather pointer in usb_storage:usb_stor_access_xfer_buf?

[Boaz Harrosh]

On Thu, Jan 31 2008 at 18:45 +0200, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Thu, 31 Jan 2008, Boaz Harrosh wrote:
> 
>>>> Please check the below patch.
>>>>
>>>> one thing that I can see is that the isd200 does an INQUARY transfer
>>>> of sizeof(struct inquiry_data) which is 96 bytes, when scsi_scan.c
>>>> sends an INQUARY with 36 bytes buffer. So we have an underflow in 
>>>> usb_stor_access_xfer_buf().
> 
> Maybe the isd200 routine should be changed also, so that it doesn't try
> to store too much data in the buffer.
> 
>> I will send a proper patch to scsi maintainer. Alan is it OK to send this
>> patch threw James's scsi-misc?
> 
> You should send patches to Matt Dharm, since he is the usb-storage 
> maintainer.
> 
> Alan Stern
> 
> -
Right, Please see patch posted as reply to the original email.
I have also fixed isd200 to return what was asked. The fix
to protocol.c is also different and more general now.

Will send to Matthew Dharm.

Matthew - is it OK to send this threw James, please ACK.
Mark - this fix is different we do need testing.

Thanks to all
Boaz

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Alan Stern]

On Thu, 31 Jan 2008, Boaz Harrosh wrote:

> @@ -228,9 +228,14 @@ void usb_stor_set_xfer_buf(unsigned char *buffer,
>  {
>  	unsigned int offset = 0;
>  	struct scatterlist *sg = NULL;
> +	unsigned int count;
>  
> -	usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
> +	count = usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
>  			TO_XFER_BUF);
> -	if (buflen < scsi_bufflen(srb))
> -		scsi_set_resid(srb, scsi_bufflen(srb) - buflen);
> +
> +	/* Check for underflow */
> +	if (buflen > scsi_bufflen(srb))
> +		count = buflen;
> +
> +	scsi_set_resid(srb, scsi_bufflen(srb) - count);
>  }

This last "if" statement doesn't look right.  And since you know that
count will never be larger than scsi_bufflen(srb), you don't have to
check for underflow at all.  Just leave out the "if" and call
scsi_set_resid() directly.

Alan Stern

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Greg KH]

On Thu, Jan 31, 2008 at 07:19:57PM +0200, Boaz Harrosh wrote:
> 
>   scsi_scan is issuing a 36-byte INQUIRY request to llds. isd200 would
>   volunteer 96 bytes of INQUIRY. This caused an underflow condition in
>   protocol.c usb_stor_access_xfer_buf(). So first fix is to
>   usb_stor_access_xfer_buf() to properly handle underflow conditions.
>   Then usb_stor_set_xfer_buf() should report this condition as a negative
>   resid. Should we also set cmnd->status in the underflow condition?
> 
>   Then also isd200.c is fixed to only return the type of INQUIRY && SENSE
>   the upper layer asked for.
> 
> Signed-off-by: Boaz Harrosh <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org>

As this is a regression and hits 2.6.24, can you send the final version
of this patch to the stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org address so we can get it into the
2.6.24.y tree?

thanks,

greg k-h

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Boaz Harrosh]

On Thu, Jan 31 2008 at 20:00 +0200, Greg KH <greg@kroah.com> wrote:
> On Thu, Jan 31, 2008 at 07:19:57PM +0200, Boaz Harrosh wrote:
>>   scsi_scan is issuing a 36-byte INQUIRY request to llds. isd200 would
>>   volunteer 96 bytes of INQUIRY. This caused an underflow condition in
>>   protocol.c usb_stor_access_xfer_buf(). So first fix is to
>>   usb_stor_access_xfer_buf() to properly handle underflow conditions.
>>   Then usb_stor_set_xfer_buf() should report this condition as a negative
>>   resid. Should we also set cmnd->status in the underflow condition?
>>
>>   Then also isd200.c is fixed to only return the type of INQUIRY && SENSE
>>   the upper layer asked for.
>>
>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> 
> As this is a regression and hits 2.6.24, can you send the final version
> of this patch to the stable@kernel.org address so we can get it into the
> 2.6.24.y tree?
> 
> thanks,
> 
> greg k-h
> -

On Thu, Jan 31 2008 at 20:00 +0200, Mark Glines <mark@glines.org> wrote:
>> On Thu, 31 Jan 2008 19:20:26 +0200
>> Hi,
>> 
>> This patch fails to apply to 2.6.24.  What should I apply it to, for
>> testing?  USB Git, or SCSI git or something?
>> 
>> patching file drivers/usb/storage/isd200.c
>> Hunk #1 succeeded at 1238 (offset -23 lines).
>> Hunk #2 succeeded at 1248 (offset -23 lines).
>> Hunk #3 succeeded at 1259 (offset -23 lines).
>> patching file drivers/usb/storage/protocol.c
>> Hunk #1 FAILED at 175.
>> Hunk #2 FAILED at 228.
>> 2 out of 2 hunks FAILED -- saving rejects to file drivers/usb/storage/protocol.c.rej
>> 
>> Thanks,
>> 
>> Mark
> 

OK, I was confused with what versions this belongs to. This also answers
my original question of why it happens with my patch, as it did not
it happened before my patch. My patches only went into the 2.6.25 merge window.

So I will need to send 2 Patches. One for stable@kernel.org on top of 2.6.24.
The second is for mainline, which is the one I sent. Should go threw scsi-misc
which it is based on.

patch for 2.6.24 stable@kernel.org on the way

Boaz

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Boaz Harrosh]

On Thu, Jan 31 2008 at 19:49 +0200, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Thu, 31 Jan 2008, Boaz Harrosh wrote:
> 
>> @@ -228,9 +228,14 @@ void usb_stor_set_xfer_buf(unsigned char *buffer,
>>  {
>>  	unsigned int offset = 0;
>>  	struct scatterlist *sg = NULL;
>> +	unsigned int count;
>>  
>> -	usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
>> +	count = usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
>>  			TO_XFER_BUF);
>> -	if (buflen < scsi_bufflen(srb))
>> -		scsi_set_resid(srb, scsi_bufflen(srb) - buflen);
>> +
>> +	/* Check for underflow */
>> +	if (buflen > scsi_bufflen(srb))
>> +		count = buflen;
>> +
>> +	scsi_set_resid(srb, scsi_bufflen(srb) - count);
>>  }
> 
> This last "if" statement doesn't look right.  And since you know that
> count will never be larger than scsi_bufflen(srb), you don't have to
> check for underflow at all.  Just leave out the "if" and call
> scsi_set_resid() directly.
> 
> Alan Stern
> 
I was thinking about that hard. And I disagree. It could be that we have
sg-list length (The total of all sg->length) that does not match
scsi_bufflen() - More or less. The code in usb_stor_access_xfer_buf() will 
now correctly attempt to transfer according to buflen and what ever is available
at the passed sg's. Now this can be less or it can be more. SCSI standard defines
this as underflow/overflow. When overflow should be reported as negative values.
and an error status. (BUT not CHECK_CONDITION)

so the code is actualy
	if (buflen > scsi_bufflen(srb))
		scsi_set_resid(srb, scsi_bufflen(srb) - buflen); /* Negative overflow */
	else
		scsi_set_resid(srb, scsi_bufflen(srb) - count);

So this is actually code equivalent to above. Minus the extra call site.

The SCSI standard does allow these conditions and the system should cope
and go on. So a BUG_ON() is not accepted, I think.

But, ok, I mixed up with the comment I'll resend. underflow => overflow.

Boaz
 

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Alan Stern]

On Thu, 31 Jan 2008, Boaz Harrosh wrote:

> On Thu, Jan 31 2008 at 19:49 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Thu, 31 Jan 2008, Boaz Harrosh wrote:
> > 
> >> @@ -228,9 +228,14 @@ void usb_stor_set_xfer_buf(unsigned char *buffer,
> >>  {
> >>  	unsigned int offset = 0;
> >>  	struct scatterlist *sg = NULL;
> >> +	unsigned int count;
> >>  
> >> -	usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
> >> +	count = usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
> >>  			TO_XFER_BUF);
> >> -	if (buflen < scsi_bufflen(srb))
> >> -		scsi_set_resid(srb, scsi_bufflen(srb) - buflen);
> >> +
> >> +	/* Check for underflow */
> >> +	if (buflen > scsi_bufflen(srb))
> >> +		count = buflen;
> >> +
> >> +	scsi_set_resid(srb, scsi_bufflen(srb) - count);
> >>  }
> > 
> > This last "if" statement doesn't look right.  And since you know that
> > count will never be larger than scsi_bufflen(srb), you don't have to
> > check for underflow at all.  Just leave out the "if" and call
> > scsi_set_resid() directly.
> > 
> > Alan Stern
> > 
> I was thinking about that hard. And I disagree. It could be that we have
> sg-list length (The total of all sg->length) that does not match
> scsi_bufflen() - More or less.

If that ever happens we'll be in big trouble.  Not just here but in 
other places as well.

> The code in usb_stor_access_xfer_buf() will 
> now correctly attempt to transfer according to buflen and what ever is available
> at the passed sg's. Now this can be less or it can be more. SCSI standard defines
> this as underflow/overflow. When overflow should be reported as negative values.
> and an error status. (BUT not CHECK_CONDITION)

I don't understand.  How could you ever transfer more data than the 
requested amount?  The host won't expect to receive it and won't be 
able to handle it when it arrives.

(Furthmore, the USB mass-storage specification does not permit more 
data to be transferred than requested.  This fact isn't relevant to our 
discussion because we're talking about situations where the data was 
transferred via a non-standard protocol.  But even so...)

> so the code is actualy
> 	if (buflen > scsi_bufflen(srb))
> 		scsi_set_resid(srb, scsi_bufflen(srb) - buflen); /* Negative overflow */

Okay, let's assume that both buflen and the sum of the sg-list lengths
are somehow greater than scsi_bufflen(srb).  You still don't know that
they are equal to each other.  All you know is that the amount actually
transferred is stored in count.  Hence the negative overflow value
should be

	scsi_bufflen(srb) - count

and not

	scsi_bufflen(srb) - bufflen.

Alan Stern

[PATCH 2.6.24] bugfix for an overflow condition in usb storage & isd200.c

[Boaz Harrosh]

Greg KH <greg@kroah.com> rote:
> As this is a regression and hits 2.6.24, can you send the final version
> of this patch to the stable@kernel.org address so we can get it into the
> 2.6.24.y tree?
> 
> thanks,
> 
> greg k-h

Mark - This is for you on top of vanila v2.6.24 kernel from Linus.

---

  scsi_scan is issuing a 36-byte INQUIRY request to llds. isd200 would
  volunteer 96 bytes of INQUIRY. This caused an overflow condition in
  protocol.c usb_stor_access_xfer_buf(). So first fix is to
  usb_stor_access_xfer_buf() to properly handle overflow/underflow conditions.
  Then usb_stor_set_xfer_buf() should report this condition as a negative
  resid. Should we also set cmnd->status in the overflow condition?

  Then also isd200.c is fixed to only return the type of INQUIRY && SENSE
  the upper layer asked for.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/usb/storage/isd200.c   |    7 +++++--
 drivers/usb/storage/protocol.c |   13 +++++++++----
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
index 49ba6c0..8186e93 100644
--- a/drivers/usb/storage/isd200.c
+++ b/drivers/usb/storage/isd200.c
@@ -1238,6 +1238,7 @@ static int isd200_scsi_to_ata(struct scsi_cmnd *srb, struct us_data *us,
 	unsigned long lba;
 	unsigned long blockCount;
 	unsigned char senseData[8] = { 0, 0, 0, 0, 0, 0, 0, 0 };
+	unsigned xfer_len;
 
 	memset(ataCdb, 0, sizeof(union ata_cdb));
 
@@ -1247,8 +1248,9 @@ static int isd200_scsi_to_ata(struct scsi_cmnd *srb, struct us_data *us,
 		US_DEBUGP("   ATA OUT - INQUIRY\n");
 
 		/* copy InquiryData */
+		xfer_len = min(sizeof(info->InquiryData), scsi_bufflen(srb));
 		usb_stor_set_xfer_buf((unsigned char *) &info->InquiryData,
-				sizeof(info->InquiryData), srb);
+					xfer_len, srb);
 		srb->result = SAM_STAT_GOOD;
 		sendToTransport = 0;
 		break;
@@ -1257,7 +1259,8 @@ static int isd200_scsi_to_ata(struct scsi_cmnd *srb, struct us_data *us,
 		US_DEBUGP("   ATA OUT - SCSIOP_MODE_SENSE\n");
 
 		/* Initialize the return buffer */
-		usb_stor_set_xfer_buf(senseData, sizeof(senseData), srb);
+		xfer_len = min(sizeof(senseData), scsi_bufflen(srb));
+		usb_stor_set_xfer_buf(senseData, xfer_len, srb);
 
 		if (info->DeviceFlags & DF_MEDIA_STATUS_ENABLED)
 		{
diff --git a/drivers/usb/storage/protocol.c b/drivers/usb/storage/protocol.c
index 889622b..038a284 100644
--- a/drivers/usb/storage/protocol.c
+++ b/drivers/usb/storage/protocol.c
@@ -194,7 +194,7 @@ unsigned int usb_stor_access_xfer_buf(unsigned char *buffer,
 		 * and the starting offset within the page, and update
 		 * the *offset and *index values for the next loop. */
 		cnt = 0;
-		while (cnt < buflen) {
+		while (cnt < buflen && sg) {
 			struct page *page = sg_page(sg) +
 					((sg->offset + *offset) >> PAGE_SHIFT);
 			unsigned int poff =
@@ -248,9 +248,14 @@ void usb_stor_set_xfer_buf(unsigned char *buffer,
 {
 	unsigned int offset = 0;
 	struct scatterlist *sg = NULL;
+	unsigned count;
 
-	usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
+	count = usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
 			TO_XFER_BUF);
-	if (buflen < srb->request_bufflen)
-		srb->resid = srb->request_bufflen - buflen;
+
+	/* Check for overflow */
+	if (buflen > scsi_bufflen(srb))
+		count = buflen;
+
+	scsi_set_resid(srb, scsi_bufflen(srb) - count);
 }
-- 
1.5.3.3

Re: [PATCH 2.6.24] bugfix for an overflow condition in usb storage & isd200.c

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 4944 bytes --]

No.  No no no.

The ISD200 code was written by the ISD200 developers.  I really don't want
to go mucking about changing what commands actually get send to the ISD200
parts.  We have no idea if the will reliably accept a 36-byte INQUIRY.

Just because it happens to work for a couple of people doesn't mean it
works in the general case.  Without guidance from In-System, this is a bad
idea.

The way to deal with this is to do this via bounce buffering.  The two
commands affected (INQUIRY and MODE_SENSE) are low-performance items.
Discarding data from the end of them is also perfectly legal per spec.

Also, the entire idea of a negative resid makes my head hurt.  That sort of
change is in the category of "likely to break something else" which only
expects positive resid values.

Matt

On Thu, Jan 31, 2008 at 09:37:13PM +0200, Boaz Harrosh wrote:
> Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org> rote:
> > As this is a regression and hits 2.6.24, can you send the final version
> > of this patch to the stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org address so we can get it into the
> > 2.6.24.y tree?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Mark - This is for you on top of vanila v2.6.24 kernel from Linus.
> 
> ---
> 
>   scsi_scan is issuing a 36-byte INQUIRY request to llds. isd200 would
>   volunteer 96 bytes of INQUIRY. This caused an overflow condition in
>   protocol.c usb_stor_access_xfer_buf(). So first fix is to
>   usb_stor_access_xfer_buf() to properly handle overflow/underflow conditions.
>   Then usb_stor_set_xfer_buf() should report this condition as a negative
>   resid. Should we also set cmnd->status in the overflow condition?
> 
>   Then also isd200.c is fixed to only return the type of INQUIRY && SENSE
>   the upper layer asked for.
> 
> Signed-off-by: Boaz Harrosh <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/usb/storage/isd200.c   |    7 +++++--
>  drivers/usb/storage/protocol.c |   13 +++++++++----
>  2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
> index 49ba6c0..8186e93 100644
> --- a/drivers/usb/storage/isd200.c
> +++ b/drivers/usb/storage/isd200.c
> @@ -1238,6 +1238,7 @@ static int isd200_scsi_to_ata(struct scsi_cmnd *srb, struct us_data *us,
>  	unsigned long lba;
>  	unsigned long blockCount;
>  	unsigned char senseData[8] = { 0, 0, 0, 0, 0, 0, 0, 0 };
> +	unsigned xfer_len;
>  
>  	memset(ataCdb, 0, sizeof(union ata_cdb));
>  
> @@ -1247,8 +1248,9 @@ static int isd200_scsi_to_ata(struct scsi_cmnd *srb, struct us_data *us,
>  		US_DEBUGP("   ATA OUT - INQUIRY\n");
>  
>  		/* copy InquiryData */
> +		xfer_len = min(sizeof(info->InquiryData), scsi_bufflen(srb));
>  		usb_stor_set_xfer_buf((unsigned char *) &info->InquiryData,
> -				sizeof(info->InquiryData), srb);
> +					xfer_len, srb);
>  		srb->result = SAM_STAT_GOOD;
>  		sendToTransport = 0;
>  		break;
> @@ -1257,7 +1259,8 @@ static int isd200_scsi_to_ata(struct scsi_cmnd *srb, struct us_data *us,
>  		US_DEBUGP("   ATA OUT - SCSIOP_MODE_SENSE\n");
>  
>  		/* Initialize the return buffer */
> -		usb_stor_set_xfer_buf(senseData, sizeof(senseData), srb);
> +		xfer_len = min(sizeof(senseData), scsi_bufflen(srb));
> +		usb_stor_set_xfer_buf(senseData, xfer_len, srb);
>  
>  		if (info->DeviceFlags & DF_MEDIA_STATUS_ENABLED)
>  		{
> diff --git a/drivers/usb/storage/protocol.c b/drivers/usb/storage/protocol.c
> index 889622b..038a284 100644
> --- a/drivers/usb/storage/protocol.c
> +++ b/drivers/usb/storage/protocol.c
> @@ -194,7 +194,7 @@ unsigned int usb_stor_access_xfer_buf(unsigned char *buffer,
>  		 * and the starting offset within the page, and update
>  		 * the *offset and *index values for the next loop. */
>  		cnt = 0;
> -		while (cnt < buflen) {
> +		while (cnt < buflen && sg) {
>  			struct page *page = sg_page(sg) +
>  					((sg->offset + *offset) >> PAGE_SHIFT);
>  			unsigned int poff =
> @@ -248,9 +248,14 @@ void usb_stor_set_xfer_buf(unsigned char *buffer,
>  {
>  	unsigned int offset = 0;
>  	struct scatterlist *sg = NULL;
> +	unsigned count;
>  
> -	usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
> +	count = usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
>  			TO_XFER_BUF);
> -	if (buflen < srb->request_bufflen)
> -		srb->resid = srb->request_bufflen - buflen;
> +
> +	/* Check for overflow */
> +	if (buflen > scsi_bufflen(srb))
> +		count = buflen;
> +
> +	scsi_set_resid(srb, scsi_bufflen(srb) - count);
>  }
> -- 
> 1.5.3.3
> 

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

M:  No, Windows doesn't have any nag screens.
C:  Then what are those blue and white screens I get every day?
					-- Mike and Cobb
User Friendly, 1/4/1999

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Boaz Harrosh]

On Thu, Jan 31 2008 at 21:34 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 31 Jan 2008, Boaz Harrosh wrote:
> 
>> On Thu, Jan 31 2008 at 19:49 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:
>>> On Thu, 31 Jan 2008, Boaz Harrosh wrote:
>>>
>>>> @@ -228,9 +228,14 @@ void usb_stor_set_xfer_buf(unsigned char *buffer,
>>>>  {
>>>>  	unsigned int offset = 0;
>>>>  	struct scatterlist *sg = NULL;
>>>> +	unsigned int count;
>>>>  
>>>> -	usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
>>>> +	count = usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
>>>>  			TO_XFER_BUF);
>>>> -	if (buflen < scsi_bufflen(srb))
>>>> -		scsi_set_resid(srb, scsi_bufflen(srb) - buflen);
>>>> +
>>>> +	/* Check for underflow */
>>>> +	if (buflen > scsi_bufflen(srb))
>>>> +		count = buflen;
>>>> +
>>>> +	scsi_set_resid(srb, scsi_bufflen(srb) - count);
>>>>  }
>>> This last "if" statement doesn't look right.  And since you know that
>>> count will never be larger than scsi_bufflen(srb), you don't have to
>>> check for underflow at all.  Just leave out the "if" and call
>>> scsi_set_resid() directly.
>>>
>>> Alan Stern
>>>
>> I was thinking about that hard. And I disagree. It could be that we have
>> sg-list length (The total of all sg->length) that does not match
>> scsi_bufflen() - More or less.
> 
> If that ever happens we'll be in big trouble.  Not just here but in 
> other places as well.
Other places is other places, but you cannot put a BUG_ON here for other
places. This place here should be correct.

> 
>> The code in usb_stor_access_xfer_buf() will 
>> now correctly attempt to transfer according to buflen and what ever is available
>> at the passed sg's. Now this can be less or it can be more. SCSI standard defines
>> this as underflow/overflow. When overflow should be reported as negative values.
>> and an error status. (BUT not CHECK_CONDITION)
> 
> I don't understand.  How could you ever transfer more data than the 
> requested amount?  The host won't expect to receive it and won't be 
> able to handle it when it arrives.
> 
> (Furthmore, the USB mass-storage specification does not permit more 
> data to be transferred than requested.  This fact isn't relevant to our 
> discussion because we're talking about situations where the data was 
> transferred via a non-standard protocol.  But even so...)
That's fine then the transfer will abort with an error condition and will
be handled correctly.

> 
>> so the code is actualy
>> 	if (buflen > scsi_bufflen(srb))
>> 		scsi_set_resid(srb, scsi_bufflen(srb) - buflen); /* Negative overflow */
> 
> Okay, let's assume that both buflen and the sum of the sg-list lengths
> are somehow greater than scsi_bufflen(srb).  You still don't know that
> they are equal to each other.  All you know is that the amount actually
> transferred is stored in count.  Hence the negative overflow value
> should be
> 
> 	scsi_bufflen(srb) - count
> 
> and not
> 
> 	scsi_bufflen(srb) - bufflen.
> 
> Alan Stern
> 
No, it is possible that sg-length was good and count == bufflen,
But the CDB contains scsi_bufflen(srb) and this is an overflow
condition. with your code resid will be Zero.
Also the standard say that we should return the expected overflow and
not the actual on-the-wire overflow.

We could check and truncate beforehand, and mark a flag for overflow
and set negative resid. But since usb_stor_access_xfer_buf() properly
handles that and returns, then this form saves cycles for the common 
case and waists cycles for the very rare BUG case of overflow, but is
still correct and safe.

Boaz

Re: [PATCH 2.6.24] bugfix for an overflow condition in usb storage & isd200.c

[Boaz Harrosh]

On Thu, Jan 31 2008 at 21:49 +0200, Matthew Dharm <mdharm-scsi@one-eyed-alien.net> wrote:
> No.  No no no.
> 
> The ISD200 code was written by the ISD200 developers.  I really don't want
> to go mucking about changing what commands actually get send to the ISD200
> parts.  We have no idea if the will reliably accept a 36-byte INQUIRY.
> 
> Just because it happens to work for a couple of people doesn't mean it
> works in the general case.  Without guidance from In-System, this is a bad
> idea.
> 
> The way to deal with this is to do this via bounce buffering.  The two
> commands affected (INQUIRY and MODE_SENSE) are low-performance items.
> Discarding data from the end of them is also perfectly legal per spec.
> 
> Also, the entire idea of a negative resid makes my head hurt.  That sort of
> change is in the category of "likely to break something else" which only
> expects positive resid values.
> 
> Matt
> 
Please re-inspect the code again.
There is no device involved here. The code completely emulates this commands
with a driver made up information. the send_to_device is Zero.
(Nothing to bounce)

There is one more command like that but the standard does not permit choice
in that respect.

The negative resid is returned by iscsi for ages so I would say the scsi-ml
is fine with it. But if you want I can reset the resid and mark an overflow
condition in cmnd->status.

Boaz

Re: [PATCH 2.6.24] bugfix for an overflow condition in usb storage & isd200.c

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 1010 bytes --]

On Thu, Jan 31, 2008 at 10:05:19PM +0200, Boaz Harrosh wrote:
> On Thu, Jan 31 2008 at 21:49 +0200, Matthew Dharm <mdharm-scsi@one-eyed-alien.net> wrote:
> > No.  No no no.
> 
> Please re-inspect the code again.
> There is no device involved here. The code completely emulates this commands
> with a driver made up information. the send_to_device is Zero.
> (Nothing to bounce)

Okay, I see what you're doing there, and I can live with that.

> The negative resid is returned by iscsi for ages so I would say the scsi-ml
> is fine with it. But if you want I can reset the resid and mark an overflow
> condition in cmnd->status.

I'd be fine if one of the SCSI guru's would comment on this as being
acceptable.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

Hey Chief.  We've figured out how to save the technical department.  We 
need to be committed.
					-- The Techs
User Friendly, 1/22/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Alan Stern]

On Thu, 31 Jan 2008, Boaz Harrosh wrote:

> >> The code in usb_stor_access_xfer_buf() will 
> >> now correctly attempt to transfer according to buflen and what ever is available
> >> at the passed sg's. Now this can be less or it can be more. SCSI standard defines
> >> this as underflow/overflow. When overflow should be reported as negative values.
> >> and an error status. (BUT not CHECK_CONDITION)
> > 
> > I don't understand.  How could you ever transfer more data than the 
> > requested amount?  The host won't expect to receive it and won't be 
> > able to handle it when it arrives.
> > 
> > (Furthmore, the USB mass-storage specification does not permit more 
> > data to be transferred than requested.  This fact isn't relevant to our 
> > discussion because we're talking about situations where the data was 
> > transferred via a non-standard protocol.  But even so...)
> That's fine then the transfer will abort with an error condition and will
> be handled correctly.

I still don't understand.  Can you explain exactly what an overflow 
condition (negative residue) really means?

Does it mean that the device had more data available than the host
asked for?  If it does, then you don't need to change the isd200 code
at all.  The changes to protocol.c will be enough.

However you should realize that usb-storage, as it stands, won't work
properly with negative residues.  Look at usb_stor_Bulk_transport() in
transport.c.  The residue variable is declared to be unsigned int.  
There's also code later on in the routine which assumes the residue is
never negative.

Alan Stern

Re: [PATCH 2.6.24] bugfix for an overflow condition in usb storage & isd200.c

[Mark Glines]

On Thu, 31 Jan 2008 21:37:13 +0200
Boaz Harrosh <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org> wrote:
> 
> Mark - This is for you on top of vanila v2.6.24 kernel from Linus.

Sorry I didn't get back to you sooner.  This fixes the issue I was
having.


> ---
> 
>   scsi_scan is issuing a 36-byte INQUIRY request to llds. isd200 would
>   volunteer 96 bytes of INQUIRY. This caused an overflow condition in
>   protocol.c usb_stor_access_xfer_buf(). So first fix is to
>   usb_stor_access_xfer_buf() to properly handle overflow/underflow
> conditions. Then usb_stor_set_xfer_buf() should report this condition
> as a negative resid. Should we also set cmnd->status in the overflow
> condition?
> 
>   Then also isd200.c is fixed to only return the type of INQUIRY &&
> SENSE the upper layer asked for.
> 
> Signed-off-by: Boaz Harrosh <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org>

Tested-by: Mark Glines <mark-6pk7R1svBr8dnm+yROfE0A@public.gmane.org>

> ---
>  drivers/usb/storage/isd200.c   |    7 +++++--
>  drivers/usb/storage/protocol.c |   13 +++++++++----
>  2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/storage/isd200.c
> b/drivers/usb/storage/isd200.c index 49ba6c0..8186e93 100644
> --- a/drivers/usb/storage/isd200.c
> +++ b/drivers/usb/storage/isd200.c
> @@ -1238,6 +1238,7 @@ static int isd200_scsi_to_ata(struct scsi_cmnd
> *srb, struct us_data *us, unsigned long lba;
>  	unsigned long blockCount;
>  	unsigned char senseData[8] = { 0, 0, 0, 0, 0, 0, 0, 0 };
> +	unsigned xfer_len;
>  
>  	memset(ataCdb, 0, sizeof(union ata_cdb));
>  
> @@ -1247,8 +1248,9 @@ static int isd200_scsi_to_ata(struct scsi_cmnd
> *srb, struct us_data *us, US_DEBUGP("   ATA OUT - INQUIRY\n");
>  
>  		/* copy InquiryData */
> +		xfer_len = min(sizeof(info->InquiryData),
> scsi_bufflen(srb)); usb_stor_set_xfer_buf((unsigned char *)
> &info->InquiryData,
> -				sizeof(info->InquiryData), srb);
> +					xfer_len, srb);
>  		srb->result = SAM_STAT_GOOD;
>  		sendToTransport = 0;
>  		break;
> @@ -1257,7 +1259,8 @@ static int isd200_scsi_to_ata(struct scsi_cmnd
> *srb, struct us_data *us, US_DEBUGP("   ATA OUT -
> SCSIOP_MODE_SENSE\n"); 
>  		/* Initialize the return buffer */
> -		usb_stor_set_xfer_buf(senseData, sizeof(senseData),
> srb);
> +		xfer_len = min(sizeof(senseData), scsi_bufflen(srb));
> +		usb_stor_set_xfer_buf(senseData, xfer_len, srb);
>  
>  		if (info->DeviceFlags & DF_MEDIA_STATUS_ENABLED)
>  		{
> diff --git a/drivers/usb/storage/protocol.c
> b/drivers/usb/storage/protocol.c index 889622b..038a284 100644
> --- a/drivers/usb/storage/protocol.c
> +++ b/drivers/usb/storage/protocol.c
> @@ -194,7 +194,7 @@ unsigned int usb_stor_access_xfer_buf(unsigned
> char *buffer,
>  		 * and the starting offset within the page, and
> update
>  		 * the *offset and *index values for the next loop.
> */ cnt = 0;
> -		while (cnt < buflen) {
> +		while (cnt < buflen && sg) {
>  			struct page *page = sg_page(sg) +
>  					((sg->offset + *offset) >>
> PAGE_SHIFT); unsigned int poff =
> @@ -248,9 +248,14 @@ void usb_stor_set_xfer_buf(unsigned char *buffer,
>  {
>  	unsigned int offset = 0;
>  	struct scatterlist *sg = NULL;
> +	unsigned count;
>  
> -	usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
> +	count = usb_stor_access_xfer_buf(buffer, buflen, srb, &sg,
> &offset, TO_XFER_BUF);
> -	if (buflen < srb->request_bufflen)
> -		srb->resid = srb->request_bufflen - buflen;
> +
> +	/* Check for overflow */
> +	if (buflen > scsi_bufflen(srb))
> +		count = buflen;
> +
> +	scsi_set_resid(srb, scsi_bufflen(srb) - count);
>  }

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Boaz Harrosh]

On Thu, Jan 31 2008 at 22:56 +0200, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Thu, 31 Jan 2008, Boaz Harrosh wrote:
> 
>>>> The code in usb_stor_access_xfer_buf() will 
>>>> now correctly attempt to transfer according to buflen and what ever is available
>>>> at the passed sg's. Now this can be less or it can be more. SCSI standard defines
>>>> this as underflow/overflow. When overflow should be reported as negative values.
>>>> and an error status. (BUT not CHECK_CONDITION)
>>> I don't understand.  How could you ever transfer more data than the 
>>> requested amount?  The host won't expect to receive it and won't be 
>>> able to handle it when it arrives.
>>>
>>> (Furthmore, the USB mass-storage specification does not permit more 
>>> data to be transferred than requested.  This fact isn't relevant to our 
>>> discussion because we're talking about situations where the data was 
>>> transferred via a non-standard protocol.  But even so...)
>> That's fine then the transfer will abort with an error condition and will
>> be handled correctly.
> 
> I still don't understand.  Can you explain exactly what an overflow 
> condition (negative residue) really means?
As far as the protocol it is an error condition. But it is an error
condition that should be handled and io should continue. What it usually means
is that either target or Initiator had, like you said, more data then
expected. Usually an encoding error of the different stages of transfer
like sum of R2Ts or DATA is bigger then CDB length and so on.
The important thing is that the system should continue, and that this
is not a check_condition error.

> 
> Does it mean that the device had more data available than the host
> asked for?  If it does, then you don't need to change the isd200 code
> at all.  The changes to protocol.c will be enough.
I do in the sense that the target should return correct information requested.
overflow is still an error. A recoverable but none-standard error. The INQUIRY
has 3 flavors based on requested size. The target should comply.

> 
> However you should realize that usb-storage, as it stands, won't work
> properly with negative residues.  Look at usb_stor_Bulk_transport() in
> transport.c.  The residue variable is declared to be unsigned int.  
> There's also code later on in the routine which assumes the residue is
> never negative.
> 
> Alan Stern
> 
> -
I will change that, but if you don't mind with a FIXME: comment next
to it. I will set resid to 1 and set the cmnd->status. Setting of command
status is ineffective as it is, most probably, been set afterwards.
Should I also put a WARN_ON()? I would say yes.
I'll send a patch.

Boaz

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Alan Stern]

Sorry, I understood only about half of what you wrote -- maybe less!

On Sun, 3 Feb 2008, Boaz Harrosh wrote:

> On Thu, Jan 31 2008 at 22:56 +0200, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:

> > I still don't understand.  Can you explain exactly what an overflow 
> > condition (negative residue) really means?
> As far as the protocol it is an error condition. But it is an error
> condition that should be handled and io should continue. What it usually means
> is that either target or Initiator had, like you said, more data then
> expected. Usually an encoding error of the different stages of transfer
> like sum of R2Ts or DATA is bigger then CDB length and so on.
> The important thing is that the system should continue, and that this
> is not a check_condition error.

What is an R2T?

How does a negative residue differ from a Phase error?

What exactly do you mean by "more data then expected"?  The USB 
mass-storage protocol specifies the length of a transfer in at least 
three different places, and those places won't always agree:

    (1) For some commands, the device may have a specific amount
	of data it can supply or expect to receive.  For example,
	the device might have 90 bytes of INQUIRY data available.

    (2) Many CDBs include a field specifying how much data should
	be transferred (INQUIRY does).  The amount of data actually
	transferred is supposed to be the smaller of this value
	and the size in (1).

    (3) The USB Bulk-only transport includes fields in the CDB
	wrapper specifying the number of bytes the host expects to 
	transfer and the direction.

If (3) is different from (2) or if the direction disagrees with what
the device expects, the device is supposed to return a Phase error.
If (1) < (2) = (3) then it isn't an error; in this case the residue is
given by (2) - (1).  If (1) > (2) = (3) then according to the USB
protocol nothing special happens -- no error and residue = 0.  
(Note however that the INQUIRY response data includes a field in which 
the device can tell the host that more data is available.)

In no cases does this allow for a negative residue.

> > Does it mean that the device had more data available than the host
> > asked for?  If it does, then you don't need to change the isd200 code
> > at all.  The changes to protocol.c will be enough.
> I do in the sense that the target should return correct information requested.

Doesn't the existing code in isd200 together with your changes to
protocol.c do exactly that?

> overflow is still an error.

Once again: What exactly do you mean by "overflow"?  In what sense is 
it an error?

> A recoverable but none-standard error. The INQUIRY
> has 3 flavors based on requested size. The target should comply.

What 3 flavors are you talking about?  As far as I know there's only 
one type of INQUIRY command.

> > However you should realize that usb-storage, as it stands, won't work
> > properly with negative residues.  Look at usb_stor_Bulk_transport() in
> > transport.c.  The residue variable is declared to be unsigned int.  
> > There's also code later on in the routine which assumes the residue is
> > never negative.
> > 
> > Alan Stern
> > 
> > -
> I will change that, but if you don't mind with a FIXME: comment next
> to it. I will set resid to 1 and set the cmnd->status.

Where will you change this?  Why set resid to 1?  That means the device 
sent 1 byte less than the host expected.

> Setting of command
> status is ineffective as it is, most probably, been set afterwards.

After what?

> Should I also put a WARN_ON()? I would say yes.
> I'll send a patch.

Maybe this will be more clear once you do.  As it is, I'm totally in 
the dark.

Alan Stern

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Boaz Harrosh]

On Sun, Feb 03 2008 at 18:01 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:
> Sorry, I understood only about half of what you wrote -- maybe less!
> 
> On Sun, 3 Feb 2008, Boaz Harrosh wrote:
> 
>> On Thu, Jan 31 2008 at 22:56 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:
> 
>>> I still don't understand.  Can you explain exactly what an overflow 
>>> condition (negative residue) really means?
>> As far as the protocol it is an error condition. But it is an error
>> condition that should be handled and io should continue. What it usually means
>> is that either target or Initiator had, like you said, more data then
>> expected. Usually an encoding error of the different stages of transfer
>> like sum of R2Ts or DATA is bigger then CDB length and so on.
>> The important thing is that the system should continue, and that this
>> is not a check_condition error.
> 
> What is an R2T?
> 
> How does a negative residue differ from a Phase error?
> 
> What exactly do you mean by "more data then expected"?  The USB 
> mass-storage protocol specifies the length of a transfer in at least 
> three different places, and those places won't always agree:
> 
>     (1) For some commands, the device may have a specific amount
> 	of data it can supply or expect to receive.  For example,
> 	the device might have 90 bytes of INQUIRY data available.
> 
>     (2) Many CDBs include a field specifying how much data should
> 	be transferred (INQUIRY does).  The amount of data actually
> 	transferred is supposed to be the smaller of this value
> 	and the size in (1).
> 
>     (3) The USB Bulk-only transport includes fields in the CDB
> 	wrapper specifying the number of bytes the host expects to 
> 	transfer and the direction.
> 
> If (3) is different from (2) or if the direction disagrees with what
> the device expects, the device is supposed to return a Phase error.
> If (1) < (2) = (3) then it isn't an error; in this case the residue is
> given by (2) - (1).  If (1) > (2) = (3) then according to the USB
> protocol nothing special happens -- no error and residue = 0.  
> (Note however that the INQUIRY response data includes a field in which 
> the device can tell the host that more data is available.)
> 
> In no cases does this allow for a negative residue.
> 
>>> Does it mean that the device had more data available than the host
>>> asked for?  If it does, then you don't need to change the isd200 code
>>> at all.  The changes to protocol.c will be enough.
>> I do in the sense that the target should return correct information requested.
> 
> Doesn't the existing code in isd200 together with your changes to
> protocol.c do exactly that?
> 
>> overflow is still an error.
> 
> Once again: What exactly do you mean by "overflow"?  In what sense is 
> it an error?
> 
>> A recoverable but none-standard error. The INQUIRY
>> has 3 flavors based on requested size. The target should comply.
> 
> What 3 flavors are you talking about?  As far as I know there's only 
> one type of INQUIRY command.
> 
>>> However you should realize that usb-storage, as it stands, won't work
>>> properly with negative residues.  Look at usb_stor_Bulk_transport() in
>>> transport.c.  The residue variable is declared to be unsigned int.  
>>> There's also code later on in the routine which assumes the residue is
>>> never negative.
>>>
>>> Alan Stern
>>>
>>> -
>> I will change that, but if you don't mind with a FIXME: comment next
>> to it. I will set resid to 1 and set the cmnd->status.
> 
> Where will you change this?  Why set resid to 1?  That means the device 
> sent 1 byte less than the host expected.
> 
>> Setting of command
>> status is ineffective as it is, most probably, been set afterwards.
> 
> After what?
> 
>> Should I also put a WARN_ON()? I would say yes.
>> I'll send a patch.
> 
> Maybe this will be more clear once you do.  As it is, I'm totally in 
> the dark.
> 
> Alan Stern
> 
> -
below is what I have so far. It is for v2.6.24 if it's acceptable then
I'll also send a patch for head-of-line. Please comment.

Boaz

----
>From 3610cfa93c990bbbafb296134ac01ef6d426eb8d Mon Sep 17 00:00:00 2001
From: Boaz Harrosh <bharrosh@panasas.com>
Date: Thu, 31 Jan 2008 21:31:31 +0200
Subject: [PATCH] bugfix for an overflow condition in usb storage & isd200.c

  scsi_scan is issuing a 36-byte INQUIRY request to llds. isd200 would
  volunteer 96 bytes of INQUIRY. This caused an overflow condition in
  protocol.c usb_stor_access_xfer_buf(). So first fix is to
  usb_stor_access_xfer_buf() to properly handle overflow/underflow conditions.
  Then usb_stor_set_xfer_buf() should report this condition as cmnd->result ==
  (DID_BAD_TARGET << 16).

  Then also isd200.c is fixed to only return the type of INQUIRY && SENSE
  the upper layer asked for.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/usb/storage/isd200.c   |    7 +++++--
 drivers/usb/storage/protocol.c |   20 ++++++++++++++++----
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
index 49ba6c0..8186e93 100644
--- a/drivers/usb/storage/isd200.c
+++ b/drivers/usb/storage/isd200.c
@@ -1238,6 +1238,7 @@ static int isd200_scsi_to_ata(struct scsi_cmnd *srb, struct us_data *us,
 	unsigned long lba;
 	unsigned long blockCount;
 	unsigned char senseData[8] = { 0, 0, 0, 0, 0, 0, 0, 0 };
+	unsigned xfer_len;
 
 	memset(ataCdb, 0, sizeof(union ata_cdb));
 
@@ -1247,8 +1248,9 @@ static int isd200_scsi_to_ata(struct scsi_cmnd *srb, struct us_data *us,
 		US_DEBUGP("   ATA OUT - INQUIRY\n");
 
 		/* copy InquiryData */
+		xfer_len = min(sizeof(info->InquiryData), scsi_bufflen(srb));
 		usb_stor_set_xfer_buf((unsigned char *) &info->InquiryData,
-				sizeof(info->InquiryData), srb);
+					xfer_len, srb);
 		srb->result = SAM_STAT_GOOD;
 		sendToTransport = 0;
 		break;
@@ -1257,7 +1259,8 @@ static int isd200_scsi_to_ata(struct scsi_cmnd *srb, struct us_data *us,
 		US_DEBUGP("   ATA OUT - SCSIOP_MODE_SENSE\n");
 
 		/* Initialize the return buffer */
-		usb_stor_set_xfer_buf(senseData, sizeof(senseData), srb);
+		xfer_len = min(sizeof(senseData), scsi_bufflen(srb));
+		usb_stor_set_xfer_buf(senseData, xfer_len, srb);
 
 		if (info->DeviceFlags & DF_MEDIA_STATUS_ENABLED)
 		{
diff --git a/drivers/usb/storage/protocol.c b/drivers/usb/storage/protocol.c
index 889622b..f141819 100644
--- a/drivers/usb/storage/protocol.c
+++ b/drivers/usb/storage/protocol.c
@@ -194,7 +194,7 @@ unsigned int usb_stor_access_xfer_buf(unsigned char *buffer,
 		 * and the starting offset within the page, and update
 		 * the *offset and *index values for the next loop. */
 		cnt = 0;
-		while (cnt < buflen) {
+		while (cnt < buflen && sg) {
 			struct page *page = sg_page(sg) +
 					((sg->offset + *offset) >> PAGE_SHIFT);
 			unsigned int poff =
@@ -248,9 +248,21 @@ void usb_stor_set_xfer_buf(unsigned char *buffer,
 {
 	unsigned int offset = 0;
 	struct scatterlist *sg = NULL;
+	unsigned count;
 
-	usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
+	count = usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
 			TO_XFER_BUF);
-	if (buflen < srb->request_bufflen)
-		srb->resid = srb->request_bufflen - buflen;
+
+	/* Check for overflow */
+	if (buflen > scsi_bufflen(srb)) {
+		/* FIXME: we set resid to 0, and set status. but also put a
+		 * a WARN_ON. The status will most probably not reach upper 
+		 * layer and the WARN_ON is the only indication of this glitch.
+		 */
+		WARN_ON(1);
+		srb->result = (DID_BAD_TARGET << 16);
+		count = scsi_bufflen(srb);
+	}
+
+	scsi_set_resid(srb, scsi_bufflen(srb) - count);
 }
-- 
1.5.3.3

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 2881 bytes --]

On Sun, Feb 03, 2008 at 06:28:48PM +0200, Boaz Harrosh wrote:
> >From 3610cfa93c990bbbafb296134ac01ef6d426eb8d Mon Sep 17 00:00:00 2001
> From: Boaz Harrosh <bharrosh@panasas.com>
> Date: Thu, 31 Jan 2008 21:31:31 +0200
> Subject: [PATCH] bugfix for an overflow condition in usb storage & isd200.c
> 
>   scsi_scan is issuing a 36-byte INQUIRY request to llds. isd200 would
>   volunteer 96 bytes of INQUIRY. This caused an overflow condition in
>   protocol.c usb_stor_access_xfer_buf(). So first fix is to
>   usb_stor_access_xfer_buf() to properly handle overflow/underflow conditions.
>   Then usb_stor_set_xfer_buf() should report this condition as cmnd->result ==
>   (DID_BAD_TARGET << 16).
> 
>   Then also isd200.c is fixed to only return the type of INQUIRY && SENSE
>   the upper layer asked for.
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  drivers/usb/storage/isd200.c   |    7 +++++--
>  drivers/usb/storage/protocol.c |   20 ++++++++++++++++----
>  2 files changed, 21 insertions(+), 6 deletions(-)

Looking at this again, I think I see Alan's point.  The modifications to
ISD200 code aren't really needed.

Or, put another way, if you're going to modify the ISD200 code, you should
fix up all the other users of usb_stor_access_xfer_buf() -- there are over
a dozen or so, and it looks like none of them have sanity checks for length
(but the happen to work right now).

But, the modifications to usb_stor_access_xfer_buf() look good -- no
request from a sub-driver should be allowed to scribble into memory.  The
current code does make the implicit assumption that there is enough
storage, and will walk right off the end of the sg list if there isn't.

I'm not sure I like the mods to usb_stor_set_xfer_buf().  Any place we set
a status that we know is going to be thrown away is an invitation for a
problem later if someone changes the code to preserve that status.  It's a
jack-in-the-box, waiting to spring open in our face later.  The limit check
(which mirrors the usb_stor_access_xfer_buf modification) and WARN_ON() are
probably good.

In a strictly technical sense, the change to protocol.c are sufficient.
That is, they will prevent a serious error.  There is a justification tho
to fix all of the users of usb_stor_access_buf() to not attempt to use more
SCSI buffer than exists.

My opinion is this:  Let's make the protocol.c mods (modulo my comments
about setting useless status bits) now.  Then, let's decide if we're going
to patch all the other users of the usb_stor_*_xfer_buf() functions as a
separate discussion.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

C:  They kicked your ass, didn't they?
S:  They were cheating!
					-- The Chief and Stef
User Friendly, 11/19/1997

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 536 bytes --]

One more comment on the patch...

On Sun, Feb 03, 2008 at 06:28:48PM +0200, Boaz Harrosh wrote:
> +	/* Check for overflow */
> +	if (buflen > scsi_bufflen(srb)) {

This really should have an unlikely() around it.  This is an often-executed
code path, and we want to optimize it as much as possible.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

Type "format c:"  That should fix everything.
					-- Greg
User Friendly, 12/18/1997

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Boaz Harrosh]

On Sun, Feb 03 2008 at 21:23 +0200, Matthew Dharm <mdharm-scsi@one-eyed-alien.net> wrote:
> On Sun, Feb 03, 2008 at 06:28:48PM +0200, Boaz Harrosh wrote:
>> >From 3610cfa93c990bbbafb296134ac01ef6d426eb8d Mon Sep 17 00:00:00 2001
>> From: Boaz Harrosh <bharrosh@panasas.com>
>> Date: Thu, 31 Jan 2008 21:31:31 +0200
>> Subject: [PATCH] bugfix for an overflow condition in usb storage & isd200.c
>>
>>   scsi_scan is issuing a 36-byte INQUIRY request to llds. isd200 would
>>   volunteer 96 bytes of INQUIRY. This caused an overflow condition in
>>   protocol.c usb_stor_access_xfer_buf(). So first fix is to
>>   usb_stor_access_xfer_buf() to properly handle overflow/underflow conditions.
>>   Then usb_stor_set_xfer_buf() should report this condition as cmnd->result ==
>>   (DID_BAD_TARGET << 16).
>>
>>   Then also isd200.c is fixed to only return the type of INQUIRY && SENSE
>>   the upper layer asked for.
>>
>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
>> ---
>>  drivers/usb/storage/isd200.c   |    7 +++++--
>>  drivers/usb/storage/protocol.c |   20 ++++++++++++++++----
>>  2 files changed, 21 insertions(+), 6 deletions(-)
> 
> Looking at this again, I think I see Alan's point.  The modifications to
> ISD200 code aren't really needed.
> 
> Or, put another way, if you're going to modify the ISD200 code, you should
> fix up all the other users of usb_stor_access_xfer_buf() -- there are over
> a dozen or so, and it looks like none of them have sanity checks for length
> (but the happen to work right now).
> 
> But, the modifications to usb_stor_access_xfer_buf() look good -- no
> request from a sub-driver should be allowed to scribble into memory.  The
> current code does make the implicit assumption that there is enough
> storage, and will walk right off the end of the sg list if there isn't.
> 
> I'm not sure I like the mods to usb_stor_set_xfer_buf().  Any place we set
> a status that we know is going to be thrown away is an invitation for a
> problem later if someone changes the code to preserve that status.  It's a
> jack-in-the-box, waiting to spring open in our face later.  The limit check
> (which mirrors the usb_stor_access_xfer_buf modification) and WARN_ON() are
> probably good.
> 
If you want the WARN_ON() then isd200 code modification must stay, other 
wise the WARN_ON will trigger regularly.
You will find that most other places are naturally bound by other factors
and will not overflow. I think that those places that can, like INQUIRY,
should be fixed, and the WARN_ON should stay. If you don't fix them the
WARN_ON must go.

> In a strictly technical sense, the change to protocol.c are sufficient.
> That is, they will prevent a serious error.  There is a justification tho
> to fix all of the users of usb_stor_access_buf() to not attempt to use more
> SCSI buffer than exists.
> 
> My opinion is this:  Let's make the protocol.c mods (modulo my comments
> about setting useless status bits) now.  Then, let's decide if we're going
> to patch all the other users of the usb_stor_*_xfer_buf() functions as a
> separate discussion.
> 
> Matt
> 
I'm removing the set of result. I don't see any danger in it but it's your
code. But if the WARN_ON stays then so is the isd200 fixes.

Boaz

---
>From cd66d4d4a4a239e580714e926e9635f3426dd7fd Mon Sep 17 00:00:00 2001
From: Boaz Harrosh <bharrosh@panasas.com>
Date: Thu, 31 Jan 2008 21:31:31 +0200
Subject: [PATCH] bugfix for an overflow condition in usb storage & isd200.c

  scsi_scan is issuing a 36-byte INQUIRY request to llds. isd200 would
  volunteer 96 bytes of INQUIRY. This caused an overflow condition in
  protocol.c usb_stor_access_xfer_buf(). So first fix is to
  usb_stor_access_xfer_buf() to properly handle overflow/underflow conditions.
  Then put a WARN_ON in usb_stor_set_xfer_buf().

  isd200.c is fixed to only return the type of INQUIRY && SENSE
  the upper layer asked for.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/usb/storage/isd200.c   |    7 +++++--
 drivers/usb/storage/protocol.c |   12 ++++++++----
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
index 49ba6c0..8186e93 100644
--- a/drivers/usb/storage/isd200.c
+++ b/drivers/usb/storage/isd200.c
@@ -1238,6 +1238,7 @@ static int isd200_scsi_to_ata(struct scsi_cmnd *srb, struct us_data *us,
 	unsigned long lba;
 	unsigned long blockCount;
 	unsigned char senseData[8] = { 0, 0, 0, 0, 0, 0, 0, 0 };
+	unsigned xfer_len;
 
 	memset(ataCdb, 0, sizeof(union ata_cdb));
 
@@ -1247,8 +1248,9 @@ static int isd200_scsi_to_ata(struct scsi_cmnd *srb, struct us_data *us,
 		US_DEBUGP("   ATA OUT - INQUIRY\n");
 
 		/* copy InquiryData */
+		xfer_len = min(sizeof(info->InquiryData), scsi_bufflen(srb));
 		usb_stor_set_xfer_buf((unsigned char *) &info->InquiryData,
-				sizeof(info->InquiryData), srb);
+					xfer_len, srb);
 		srb->result = SAM_STAT_GOOD;
 		sendToTransport = 0;
 		break;
@@ -1257,7 +1259,8 @@ static int isd200_scsi_to_ata(struct scsi_cmnd *srb, struct us_data *us,
 		US_DEBUGP("   ATA OUT - SCSIOP_MODE_SENSE\n");
 
 		/* Initialize the return buffer */
-		usb_stor_set_xfer_buf(senseData, sizeof(senseData), srb);
+		xfer_len = min(sizeof(senseData), scsi_bufflen(srb));
+		usb_stor_set_xfer_buf(senseData, xfer_len, srb);
 
 		if (info->DeviceFlags & DF_MEDIA_STATUS_ENABLED)
 		{
diff --git a/drivers/usb/storage/protocol.c b/drivers/usb/storage/protocol.c
index 889622b..a3d1d8e 100644
--- a/drivers/usb/storage/protocol.c
+++ b/drivers/usb/storage/protocol.c
@@ -194,7 +194,7 @@ unsigned int usb_stor_access_xfer_buf(unsigned char *buffer,
 		 * and the starting offset within the page, and update
 		 * the *offset and *index values for the next loop. */
 		cnt = 0;
-		while (cnt < buflen) {
+		while (cnt < buflen && sg) {
 			struct page *page = sg_page(sg) +
 					((sg->offset + *offset) >> PAGE_SHIFT);
 			unsigned int poff =
@@ -248,9 +248,13 @@ void usb_stor_set_xfer_buf(unsigned char *buffer,
 {
 	unsigned int offset = 0;
 	struct scatterlist *sg = NULL;
+	unsigned count;
 
-	usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
+	count = usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
 			TO_XFER_BUF);
-	if (buflen < srb->request_bufflen)
-		srb->resid = srb->request_bufflen - buflen;
+
+	/* Check for overflow */
+	WARN_ON(buflen > scsi_bufflen(srb));
+
+	scsi_set_resid(srb, scsi_bufflen(srb) - count);
 }
-- 
1.5.3.3

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Alan Stern]

On Sun, 3 Feb 2008, Matthew Dharm wrote:

> But, the modifications to usb_stor_access_xfer_buf() look good -- no
> request from a sub-driver should be allowed to scribble into memory.  The
> current code does make the implicit assumption that there is enough
> storage, and will walk right off the end of the sg list if there isn't.
> 
> I'm not sure I like the mods to usb_stor_set_xfer_buf().  Any place we set
> a status that we know is going to be thrown away is an invitation for a
> problem later if someone changes the code to preserve that status.  It's a
> jack-in-the-box, waiting to spring open in our face later.  The limit check
> (which mirrors the usb_stor_access_xfer_buf modification) and WARN_ON() are
> probably good.
> 
> In a strictly technical sense, the change to protocol.c are sufficient.
> That is, they will prevent a serious error.  There is a justification tho
> to fix all of the users of usb_stor_access_buf() to not attempt to use more
> SCSI buffer than exists.
> 
> My opinion is this:  Let's make the protocol.c mods (modulo my comments
> about setting useless status bits) now.  Then, let's decide if we're going
> to patch all the other users of the usb_stor_*_xfer_buf() functions as a
> separate discussion.

I think the correct approach is to modify those routines so that they 
will never overrun the s-g buffer (like Boaz has done), and _document_ 
this behavior.  Then the callers can feel free to try and transfer as 
much as they want, knowing that an overrun can't occur.  There won't 
be any need for a WARN_ON or anything else.

However the interface to usb_stor_access_xfer_buf() will have to change
slightly.  Right now if it sees that *sgptr is NULL, it assumes this
means it should start at the beginning of the s-g buffer.  But with 
Boaz's change, *sgptr == NULL means the transfer has reached the end of 
the buffer.  So I'll have to go through and audit all the callers.

Alan Stern

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Boaz Harrosh]

On Mon, Feb 04 2008 at 22:05 +0200, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Sun, 3 Feb 2008, Matthew Dharm wrote:
> 
>> But, the modifications to usb_stor_access_xfer_buf() look good -- no
>> request from a sub-driver should be allowed to scribble into memory.  The
>> current code does make the implicit assumption that there is enough
>> storage, and will walk right off the end of the sg list if there isn't.
>>
>> I'm not sure I like the mods to usb_stor_set_xfer_buf().  Any place we set
>> a status that we know is going to be thrown away is an invitation for a
>> problem later if someone changes the code to preserve that status.  It's a
>> jack-in-the-box, waiting to spring open in our face later.  The limit check
>> (which mirrors the usb_stor_access_xfer_buf modification) and WARN_ON() are
>> probably good.
>>
>> In a strictly technical sense, the change to protocol.c are sufficient.
>> That is, they will prevent a serious error.  There is a justification tho
>> to fix all of the users of usb_stor_access_buf() to not attempt to use more
>> SCSI buffer than exists.
>>
>> My opinion is this:  Let's make the protocol.c mods (modulo my comments
>> about setting useless status bits) now.  Then, let's decide if we're going
>> to patch all the other users of the usb_stor_*_xfer_buf() functions as a
>> separate discussion.
> 
> I think the correct approach is to modify those routines so that they 
> will never overrun the s-g buffer (like Boaz has done), and _document_ 
> this behavior.  Then the callers can feel free to try and transfer as 
> much as they want, knowing that an overrun can't occur.  There won't 
> be any need for a WARN_ON or anything else.
> 
> However the interface to usb_stor_access_xfer_buf() will have to change
> slightly.  Right now if it sees that *sgptr is NULL, it assumes this
> means it should start at the beginning of the s-g buffer.  But with 
> Boaz's change, *sgptr == NULL means the transfer has reached the end of 
> the buffer.  So I'll have to go through and audit all the callers.
> 
> Alan Stern
> 
> -
No it does not, this as not changed. Please look again.
Note that this patch was tested and working. It is a bug
in v2.2.24 and it should be accepted already. One way or
the other.

Callers of usb_stor_access_xfer_buf() need not change.
Matthew Dharm should decide if he wants the WARN_ON in 
usb_stor_set_xfer_buf() or not and be done with it.

I have found and fixed the bug, but it is not a SCSI
related bug, and it is not do to any scsi changes. It
is a bug from the SG changes of early 2.6.24. Please
take it through the USB tree. Feel free to change it
the way you like it, and submit it.

Boaz

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Alan Stern]

On Tue, 5 Feb 2008, Boaz Harrosh wrote:

> > However the interface to usb_stor_access_xfer_buf() will have to change
> > slightly.  Right now if it sees that *sgptr is NULL, it assumes this
> > means it should start at the beginning of the s-g buffer.  But with 
> > Boaz's change, *sgptr == NULL means the transfer has reached the end of 
> > the buffer.  So I'll have to go through and audit all the callers.
> > 
> > Alan Stern
> > 
> > -
> No it does not, this as not changed. Please look again.

You look again.  Your patched code goes like this:

		struct scatterlist *sg = *sgptr;

		if (!sg)
			sg = (struct scatterlist *) srb->request_buffer;

Hence if *sgptr is NULL upon entry, it is taken to mean that the 
transfer should start at the beginning of the s-g buffer.

		/* This loop handles a single s-g list entry, which may
		 * include multiple pages.  Find the initial page structure
		 * and the starting offset within the page, and update
		 * the *offset and *index values for the next loop. */
		cnt = 0;
		while (cnt < buflen && sg) {

Hence if sg is NULL, it indicates the end of the buffer has been 
reached.  And then down near the end of the routine:

		*sgptr = sg;

Hence if the end is reached and the caller makes another call to try 
transferring more data, the additional data will get stored back at the 
beginning of the buffer.

> Note that this patch was tested and working. It is a bug
> in v2.2.24 and it should be accepted already. One way or
> the other.
> 
> Callers of usb_stor_access_xfer_buf() need not change.
> Matthew Dharm should decide if he wants the WARN_ON in 
> usb_stor_set_xfer_buf() or not and be done with it.
> 
> I have found and fixed the bug, but it is not a SCSI
> related bug, and it is not do to any scsi changes. It
> is a bug from the SG changes of early 2.6.24. Please
> take it through the USB tree. Feel free to change it
> the way you like it, and submit it.

I will post a new version of this which handles all these issues.  
Expect it in a day or so.

Alan Stern

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Boaz Harrosh]

On Tue, Feb 05 2008 at 17:42 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 5 Feb 2008, Boaz Harrosh wrote:
> 
>>> However the interface to usb_stor_access_xfer_buf() will have to change
>>> slightly.  Right now if it sees that *sgptr is NULL, it assumes this
>>> means it should start at the beginning of the s-g buffer.  But with 
>>> Boaz's change, *sgptr == NULL means the transfer has reached the end of 
>>> the buffer.  So I'll have to go through and audit all the callers.
>>>
>>> Alan Stern
>>>
>>> -
>> No it does not, this as not changed. Please look again.
> 
> You look again.  Your patched code goes like this:
> 
> 		struct scatterlist *sg = *sgptr;
> 
> 		if (!sg)
> 			sg = (struct scatterlist *) srb->request_buffer;
> 
> Hence if *sgptr is NULL upon entry, it is taken to mean that the 
> transfer should start at the beginning of the s-g buffer.
> 
> 		/* This loop handles a single s-g list entry, which may
> 		 * include multiple pages.  Find the initial page structure
> 		 * and the starting offset within the page, and update
> 		 * the *offset and *index values for the next loop. */
> 		cnt = 0;
> 		while (cnt < buflen && sg) {
> 
> Hence if sg is NULL, it indicates the end of the buffer has been 
> reached.  And then down near the end of the routine:
> 
> 		*sgptr = sg;
> 
> Hence if the end is reached and the caller makes another call to try 
> transferring more data, the additional data will get stored back at the 
> beginning of the buffer.
> 
That behavior did not change. In the likely event of sg-length matching
bufflen the last call to sg_next will return NULL, and will be returned 
in *sgptr. The end condition of an outside caller is either sum of 
returned counts reaching some target count, or *sgptr return to NULL.
The code before the sg change would have *indexptr >= some_sg_count, but 
now we do not have an index we have a pointer and the termination condition 
is *sgptr == NULL.

So I guess you are afraid that calling code that was converted from index
to pointer, was done wrong, and where something did  *indexptr >= some_sg_count
before, does not do *sgptr == NULL now.

So I guess, yes you are welcome to check. I did not do the conversion so
I can not comment.

>> Note that this patch was tested and working. It is a bug
>> in v2.2.24 and it should be accepted already. One way or
>> the other.
>>
>> Callers of usb_stor_access_xfer_buf() need not change.
>> Matthew Dharm should decide if he wants the WARN_ON in 
>> usb_stor_set_xfer_buf() or not and be done with it.
>>
>> I have found and fixed the bug, but it is not a SCSI
>> related bug, and it is not do to any scsi changes. It
>> is a bug from the SG changes of early 2.6.24. Please
>> take it through the USB tree. Feel free to change it
>> the way you like it, and submit it.
> 
> I will post a new version of this which handles all these issues.  
> Expect it in a day or so.
> 

Please do. Thanks, that would be better.
Don't forget to also submit a patch for current head-of-line. It's exactly
the same fix but has diff conflicts with surrounding code.

> Alan Stern
> 

Boaz

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 1194 bytes --]

On Mon, Feb 04, 2008 at 03:05:58PM -0500, Alan Stern wrote:
> On Sun, 3 Feb 2008, Matthew Dharm wrote:
> 
> I think the correct approach is to modify those routines so that they 
> will never overrun the s-g buffer (like Boaz has done), and _document_ 
> this behavior.  Then the callers can feel free to try and transfer as 
> much as they want, knowing that an overrun can't occur.  There won't 
> be any need for a WARN_ON or anything else.

Six of one and a half-dozen of the other.  All we're arguing over is the
definition of "correct behavior" here.  You want to change the API so that
overrun is acceptable and handled; I prefer calling it a Bad Thing(tm).

We both agree that the code shouldn't run off the end of the s-g list.

Since you've already committed to updating the patch, then we can do it
your way.  Just make sure it's very very clear in the comments.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

E:  You run this ship with Windows?!  YOU IDIOT!
L:  Give me a break, it came bundled with the computer!
					-- ESR and Lan Solaris
User Friendly, 12/8/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Alan Stern]

On Tue, 5 Feb 2008, Matthew Dharm wrote:

> Six of one and a half-dozen of the other.  All we're arguing over is the
> definition of "correct behavior" here.  You want to change the API so that
> overrun is acceptable and handled; I prefer calling it a Bad Thing(tm).
> 
> We both agree that the code shouldn't run off the end of the s-g list.
> 
> Since you've already committed to updating the patch, then we can do it
> your way.  Just make sure it's very very clear in the comments.

Okay, here's my version.  It makes some significant changes to the 
interface for usb_stor_access_xfer_buf() -- in particular, the context 
information is now stored in an opaque structure rather than in ad-hoc 
local variables.  All the callers are updated to use the new interface.

Functionally it's almost the same as before.  It's more careful about
not letting callers overrun the transfer buffer, and it can now set the
SCSI residue when a copy is finished.

This patch applies to 2.6.24.  If it's acceptable I'll send a patch for 
2.6.25-rc1 (once it has been released), complete with Changelog and 
a Signed-off-by: line.

Note that this has been compile-tested only!  Verification that it 
works okay would be appreciated.

Alan Stern


Index: 2.6.24/drivers/usb/storage/protocol.h
===================================================================
--- 2.6.24.orig/drivers/usb/storage/protocol.h
+++ 2.6.24/drivers/usb/storage/protocol.h
@@ -48,13 +48,23 @@ extern void usb_stor_ufi_command(struct 
 extern void usb_stor_transparent_scsi_command(struct scsi_cmnd*,
 		struct us_data*);
 
-/* struct scsi_cmnd transfer buffer access utilities */
+/* Utility routines for accessing data in scsi_cmnd transfer buffers */
 enum xfer_buf_dir	{TO_XFER_BUF, FROM_XFER_BUF};
 
+struct xfer_buf_params {
+	struct scsi_cmnd	*srb;
+	struct scatterlist	*sg;
+	enum xfer_buf_dir	dir;
+	unsigned int		offset;
+	unsigned int		count;
+};
+
+extern void usb_stor_init_xfer_buf(struct xfer_buf_params *xbp,
+		struct us_data *us, enum xfer_buf_dir dir);
 extern unsigned int usb_stor_access_xfer_buf(unsigned char *buffer,
-	unsigned int buflen, struct scsi_cmnd *srb, struct scatterlist **,
-	unsigned int *offset, enum xfer_buf_dir dir);
+		unsigned int buflen, struct xfer_buf_params *xbp);
+extern void usb_stor_finish_xfer_buf(struct xfer_buf_params *xbp);
 
 extern void usb_stor_set_xfer_buf(unsigned char *buffer,
-	unsigned int buflen, struct scsi_cmnd *srb);
+	unsigned int buflen, struct us_data *us);
 #endif
Index: 2.6.24/drivers/usb/storage/protocol.c
===================================================================
--- 2.6.24.orig/drivers/usb/storage/protocol.c
+++ 2.6.24/drivers/usb/storage/protocol.c
@@ -148,33 +148,66 @@ void usb_stor_transparent_scsi_command(s
  * Scatter-gather transfer buffer access routines
  ***********************************************************************/
 
-/* Copy a buffer of length buflen to/from the srb's transfer buffer.
- * (Note: for scatter-gather transfers (srb->use_sg > 0), srb->request_buffer
- * points to a list of s-g entries and we ignore srb->request_bufflen.
- * For non-scatter-gather transfers, srb->request_buffer points to the
- * transfer buffer itself and srb->request_bufflen is the buffer's length.)
- * Update the *index and *offset variables so that the next copy will
- * pick up from where this one left off. */
+/* usb_stor_init_xfer_buf - initialize an xfer_buf_params structure
+ *
+ * Store in @xbp the values needed for accessing a transfer buffer
+ * through several calls to usb_stor_access_xfer_buf().  The sg
+ * and offset members are initialized to point to the start of the
+ * transfer buffer for @us->srb.
+ */
+void usb_stor_init_xfer_buf(struct xfer_buf_params *xbp,
+		struct us_data *us, enum xfer_buf_dir dir)
+{
+	xbp->srb = us->srb;
+	xbp->sg = (struct scatterlist *) xbp->srb->request_buffer;
+	xbp->dir = dir;
+	xbp->offset = 0;
+	xbp->count = 0;
+}
 
+/* usb_stor_access_xfer_buf - copy data to/from a scsi_cmnd's transfer buffer
+ *
+ * Copy data between @buffer and @xbp->srb's transfer buffer.  Data
+ * can be copied incrementally through successive calls to this routine;
+ * each copy will pick up from the location in the transfer buffer where
+ * the previous call left off.
+ *
+ * The direction of the copy is as specified in usb_stor_init_xfer_buf(),
+ * which must have been called earlier.
+ *
+ * The amount of data to copy is given by @buflen, but it is limited
+ * by @xbp->srb->request_bufflen as well as by the total length of
+ * the scatter-gather buffers (if @xbp->srb uses scatter-gather).
+ * It's not an error for the caller to try copying too much data;
+ * when that happens only the data that fits will be copied.
+ *
+ * The sg and offset members of @xbp are updated to point to the next
+ * location in the transfer buffer.  The count member is updated to
+ * keep track of the total number of bytes copied across multiple
+ * calls.
+ *
+ * Returns the number of bytes transferred in this call.
+ */
 unsigned int usb_stor_access_xfer_buf(unsigned char *buffer,
-	unsigned int buflen, struct scsi_cmnd *srb, struct scatterlist **sgptr,
-	unsigned int *offset, enum xfer_buf_dir dir)
+	unsigned int buflen, struct xfer_buf_params *xbp)
 {
 	unsigned int cnt;
 
-	/* If not using scatter-gather, just transfer the data directly.
-	 * Make certain it will fit in the available buffer space. */
-	if (srb->use_sg == 0) {
-		if (*offset >= srb->request_bufflen)
-			return 0;
-		cnt = min(buflen, srb->request_bufflen - *offset);
-		if (dir == TO_XFER_BUF)
-			memcpy((unsigned char *) srb->request_buffer + *offset,
+	/* Make sure we don't try to copy too much data */
+	buflen = min(buflen, xbp->srb->request_bufflen - xbp->count);
+
+	/* If not using scatter-gather, just transfer the data directly */
+	if (xbp->srb->use_sg == 0) {
+		cnt = buflen;
+		if (xbp->dir == TO_XFER_BUF)
+			memcpy(xbp->offset + (unsigned char *)
+					xbp->srb->request_buffer,
 					buffer, cnt);
 		else
-			memcpy(buffer, (unsigned char *) srb->request_buffer +
-					*offset, cnt);
-		*offset += cnt;
+			memcpy(buffer, xbp->offset + (unsigned char *)
+					xbp->srb->request_buffer, cnt);
+		xbp->offset += cnt;
+	}
 
 	/* Using scatter-gather.  We have to go through the list one entry
 	 * at a time.  Each s-g entry contains some number of pages, and
@@ -183,34 +216,32 @@ unsigned int usb_stor_access_xfer_buf(un
 	 * If the page is not directly accessible -- such as a user buffer
 	 * located in high memory -- then kmap() will map it to a temporary
 	 * position in the kernel's virtual address space. */
-	} else {
-		struct scatterlist *sg = *sgptr;
-
-		if (!sg)
-			sg = (struct scatterlist *) srb->request_buffer;
+	else {
 
 		/* This loop handles a single s-g list entry, which may
 		 * include multiple pages.  Find the initial page structure
 		 * and the starting offset within the page, and update
 		 * the *offset and *index values for the next loop. */
 		cnt = 0;
-		while (cnt < buflen) {
-			struct page *page = sg_page(sg) +
-					((sg->offset + *offset) >> PAGE_SHIFT);
+		while (cnt < buflen && xbp->sg) {
+			struct page *page = sg_page(xbp->sg) +
+					((xbp->sg->offset + xbp->offset) >>
+						PAGE_SHIFT);
 			unsigned int poff =
-					(sg->offset + *offset) & (PAGE_SIZE-1);
-			unsigned int sglen = sg->length - *offset;
+					(xbp->sg->offset + xbp->offset) &
+						(PAGE_SIZE-1);
+			unsigned int sglen = xbp->sg->length - xbp->offset;
 
 			if (sglen > buflen - cnt) {
 
 				/* Transfer ends within this s-g entry */
 				sglen = buflen - cnt;
-				*offset += sglen;
+				xbp->offset += sglen;
 			} else {
 
 				/* Transfer continues to next s-g entry */
-				*offset = 0;
-				sg = sg_next(sg);
+				xbp->offset = 0;
+				xbp->sg = sg_next(xbp->sg);
 			}
 
 			/* Transfer the data for all the pages in this
@@ -221,7 +252,7 @@ unsigned int usb_stor_access_xfer_buf(un
 						PAGE_SIZE - poff);
 				unsigned char *ptr = kmap(page);
 
-				if (dir == TO_XFER_BUF)
+				if (xbp->dir == TO_XFER_BUF)
 					memcpy(ptr + poff, buffer + cnt, plen);
 				else
 					memcpy(buffer + cnt, ptr + poff, plen);
@@ -234,23 +265,31 @@ unsigned int usb_stor_access_xfer_buf(un
 				sglen -= plen;
 			}
 		}
-		*sgptr = sg;
 	}
 
 	/* Return the amount actually transferred */
+	xbp->count += cnt;
 	return cnt;
 }
 
+/* usb_stor_finish_xfer_buf - end a transfer buffer data copy
+ *
+ * Sets @xbp->srb->resid based on the total amount of data copied.
+ */
+void usb_stor_finish_xfer_buf(struct xfer_buf_params *xbp)
+{
+	if (xbp->count < xbp->srb->request_bufflen)
+		xbp->srb->resid = xbp->srb->request_bufflen - xbp->count;
+}
+
 /* Store the contents of buffer into srb's transfer buffer and set the
  * SCSI residue. */
 void usb_stor_set_xfer_buf(unsigned char *buffer,
-	unsigned int buflen, struct scsi_cmnd *srb)
+	unsigned int buflen, struct us_data *us)
 {
-	unsigned int offset = 0;
-	struct scatterlist *sg = NULL;
+	struct xfer_buf_params xbp;
 
-	usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
-			TO_XFER_BUF);
-	if (buflen < srb->request_bufflen)
-		srb->resid = srb->request_bufflen - buflen;
+	usb_stor_init_xfer_buf(&xbp, us, TO_XFER_BUF);
+	usb_stor_access_xfer_buf(buffer, buflen, &xbp);
+	usb_stor_finish_xfer_buf(&xbp);
 }
Index: 2.6.24/drivers/usb/storage/alauda.c
===================================================================
--- 2.6.24.orig/drivers/usb/storage/alauda.c
+++ 2.6.24/drivers/usb/storage/alauda.c
@@ -798,14 +798,14 @@ static int alauda_read_data(struct us_da
 {
 	unsigned char *buffer;
 	u16 lba, max_lba;
-	unsigned int page, len, offset;
+	unsigned int page, len;
 	unsigned int blockshift = MEDIA_INFO(us).blockshift;
 	unsigned int pageshift = MEDIA_INFO(us).pageshift;
 	unsigned int blocksize = MEDIA_INFO(us).blocksize;
 	unsigned int pagesize = MEDIA_INFO(us).pagesize;
 	unsigned int uzonesize = MEDIA_INFO(us).uzonesize;
-	struct scatterlist *sg;
 	int result;
+	struct xfer_buf_params xbp;
 
 	/*
 	 * Since we only read in one block at a time, we have to create
@@ -828,8 +828,7 @@ static int alauda_read_data(struct us_da
 	max_lba = MEDIA_INFO(us).capacity >> (blockshift + pageshift);
 
 	result = USB_STOR_TRANSPORT_GOOD;
-	offset = 0;
-	sg = NULL;
+	usb_stor_init_xfer_buf(&xbp, us, TO_XFER_BUF);
 
 	while (sectors > 0) {
 		unsigned int zone = lba / uzonesize; /* integer division */
@@ -874,13 +873,13 @@ static int alauda_read_data(struct us_da
 		}
 
 		/* Store the data in the transfer buffer */
-		usb_stor_access_xfer_buf(buffer, len, us->srb,
-				&sg, &offset, TO_XFER_BUF);
+		usb_stor_access_xfer_buf(buffer, len, &xbp);
 
 		page = 0;
 		lba++;
 		sectors -= pages;
 	}
+	usb_stor_finish_xfer_buf(&xbp);
 
 	kfree(buffer);
 	return result;
@@ -893,14 +892,14 @@ static int alauda_write_data(struct us_d
 		unsigned int sectors)
 {
 	unsigned char *buffer, *blockbuffer;
-	unsigned int page, len, offset;
+	unsigned int page, len;
 	unsigned int blockshift = MEDIA_INFO(us).blockshift;
 	unsigned int pageshift = MEDIA_INFO(us).pageshift;
 	unsigned int blocksize = MEDIA_INFO(us).blocksize;
 	unsigned int pagesize = MEDIA_INFO(us).pagesize;
-	struct scatterlist *sg;
 	u16 lba, max_lba;
 	int result;
+	struct xfer_buf_params xbp;
 
 	/*
 	 * Since we don't write the user data directly to the device,
@@ -932,8 +931,7 @@ static int alauda_write_data(struct us_d
 	max_lba = MEDIA_INFO(us).capacity >> (pageshift + blockshift);
 
 	result = USB_STOR_TRANSPORT_GOOD;
-	offset = 0;
-	sg = NULL;
+	usb_stor_init_xfer_buf(&xbp, us, FROM_XFER_BUF);
 
 	while (sectors > 0) {
 		/* Write as many sectors as possible in this block */
@@ -949,8 +947,7 @@ static int alauda_write_data(struct us_d
 		}
 
 		/* Get the data from the transfer buffer */
-		usb_stor_access_xfer_buf(buffer, len, us->srb,
-				&sg, &offset, FROM_XFER_BUF);
+		usb_stor_access_xfer_buf(buffer, len, &xbp);
 
 		result = alauda_write_lba(us, lba, page, pages, buffer,
 			blockbuffer);
@@ -961,6 +958,7 @@ static int alauda_write_data(struct us_d
 		lba++;
 		sectors -= pages;
 	}
+	usb_stor_finish_xfer_buf(&xbp);
 
 	kfree(buffer);
 	kfree(blockbuffer);
@@ -1053,7 +1051,7 @@ int alauda_transport(struct scsi_cmnd *s
 		((__be32 *) ptr)[0] = cpu_to_be32(capacity - 1);
 		((__be32 *) ptr)[1] = cpu_to_be32(512);
 
-		usb_stor_set_xfer_buf(ptr, 8, srb);
+		usb_stor_set_xfer_buf(ptr, 8, us);
 		return USB_STOR_TRANSPORT_GOOD;
 	}
 
@@ -1102,7 +1100,7 @@ int alauda_transport(struct scsi_cmnd *s
 		ptr[7] = 11;
 		ptr[12] = info->sense_asc;
 		ptr[13] = info->sense_ascq;
-		usb_stor_set_xfer_buf(ptr, 18, srb);
+		usb_stor_set_xfer_buf(ptr, 18, us);
 
 		return USB_STOR_TRANSPORT_GOOD;
 	}
Index: 2.6.24/drivers/usb/storage/datafab.c
===================================================================
--- 2.6.24.orig/drivers/usb/storage/datafab.c
+++ 2.6.24/drivers/usb/storage/datafab.c
@@ -98,8 +98,7 @@ static int datafab_read_data(struct us_d
 	unsigned char  thistime;
 	unsigned int totallen, alloclen;
 	int len, result;
-	unsigned int sg_offset = 0;
-	struct scatterlist *sg = NULL;
+	struct xfer_buf_params xbp;
 
 	// we're working in LBA mode.  according to the ATA spec, 
 	// we can support up to 28-bit addressing.  I don't know if Datafab
@@ -126,6 +125,7 @@ static int datafab_read_data(struct us_d
 	if (buffer == NULL)
 		return USB_STOR_TRANSPORT_ERROR;
 
+	usb_stor_init_xfer_buf(&xbp, us, TO_XFER_BUF);
 	do {
 		// loop, never allocate or transfer more than 64k at once
 		// (min(128k, 255*info->ssize) is the real limit)
@@ -155,13 +155,13 @@ static int datafab_read_data(struct us_d
 			goto leave;
 
 		// Store the data in the transfer buffer
-		usb_stor_access_xfer_buf(buffer, len, us->srb,
-				 &sg, &sg_offset, TO_XFER_BUF);
+		usb_stor_access_xfer_buf(buffer, len, &xbp);
 
 		sector += thistime;
 		totallen -= len;
 	} while (totallen > 0);
 
+	usb_stor_finish_xfer_buf(&xbp);
 	kfree(buffer);
 	return USB_STOR_TRANSPORT_GOOD;
 
@@ -182,8 +182,7 @@ static int datafab_write_data(struct us_
 	unsigned char thistime;
 	unsigned int totallen, alloclen;
 	int len, result;
-	unsigned int sg_offset = 0;
-	struct scatterlist *sg = NULL;
+	struct xfer_buf_params xbp;
 
 	// we're working in LBA mode.  according to the ATA spec, 
 	// we can support up to 28-bit addressing.  I don't know if Datafab
@@ -210,6 +209,7 @@ static int datafab_write_data(struct us_
 	if (buffer == NULL)
 		return USB_STOR_TRANSPORT_ERROR;
 
+	usb_stor_init_xfer_buf(&xbp, us, FROM_XFER_BUF);
 	do {
 		// loop, never allocate or transfer more than 64k at once
 		// (min(128k, 255*info->ssize) is the real limit)
@@ -218,8 +218,7 @@ static int datafab_write_data(struct us_
 		thistime = (len / info->ssize) & 0xff;
 
 		// Get the data from the transfer buffer
-		usb_stor_access_xfer_buf(buffer, len, us->srb,
-				&sg, &sg_offset, FROM_XFER_BUF);
+		usb_stor_access_xfer_buf(buffer, len, &xbp);
 
 		command[0] = 0;
 		command[1] = thistime;
@@ -259,6 +258,7 @@ static int datafab_write_data(struct us_
 		totallen -= len;
 	} while (totallen > 0);
 
+	usb_stor_finish_xfer_buf(&xbp);
 	kfree(buffer);
 	return USB_STOR_TRANSPORT_GOOD;
 
@@ -488,7 +488,7 @@ static int datafab_handle_mode_sense(str
 		ptr[0] = i - 1;
 	else
 		((__be16 *) ptr)[0] = cpu_to_be16(i - 2);
-	usb_stor_set_xfer_buf(ptr, i, srb);
+	usb_stor_set_xfer_buf(ptr, i, us);
 
 	return USB_STOR_TRANSPORT_GOOD;
 }
@@ -545,7 +545,7 @@ int datafab_transport(struct scsi_cmnd *
 		// we need the last sector, not the number of sectors
 		((__be32 *) ptr)[0] = cpu_to_be32(info->sectors - 1);
 		((__be32 *) ptr)[1] = cpu_to_be32(info->ssize);
-		usb_stor_set_xfer_buf(ptr, 8, srb);
+		usb_stor_set_xfer_buf(ptr, 8, us);
 
 		return USB_STOR_TRANSPORT_GOOD;
 	}
@@ -621,7 +621,7 @@ int datafab_transport(struct scsi_cmnd *
 		ptr[7] = 11;
 		ptr[12] = info->sense_asc;
 		ptr[13] = info->sense_ascq;
-		usb_stor_set_xfer_buf(ptr, 18, srb);
+		usb_stor_set_xfer_buf(ptr, 18, us);
 
 		return USB_STOR_TRANSPORT_GOOD;
 	}
Index: 2.6.24/drivers/usb/storage/isd200.c
===================================================================
--- 2.6.24.orig/drivers/usb/storage/isd200.c
+++ 2.6.24/drivers/usb/storage/isd200.c
@@ -1248,7 +1248,7 @@ static int isd200_scsi_to_ata(struct scs
 
 		/* copy InquiryData */
 		usb_stor_set_xfer_buf((unsigned char *) &info->InquiryData,
-				sizeof(info->InquiryData), srb);
+				sizeof(info->InquiryData), us);
 		srb->result = SAM_STAT_GOOD;
 		sendToTransport = 0;
 		break;
@@ -1257,7 +1257,7 @@ static int isd200_scsi_to_ata(struct scs
 		US_DEBUGP("   ATA OUT - SCSIOP_MODE_SENSE\n");
 
 		/* Initialize the return buffer */
-		usb_stor_set_xfer_buf(senseData, sizeof(senseData), srb);
+		usb_stor_set_xfer_buf(senseData, sizeof(senseData), us);
 
 		if (info->DeviceFlags & DF_MEDIA_STATUS_ENABLED)
 		{
@@ -1310,7 +1310,7 @@ static int isd200_scsi_to_ata(struct scs
 		readCapacityData.BytesPerBlock = cpu_to_be32(0x200);
 
 		usb_stor_set_xfer_buf((unsigned char *) &readCapacityData,
-				sizeof(readCapacityData), srb);
+				sizeof(readCapacityData), us);
 		srb->result = SAM_STAT_GOOD;
 		sendToTransport = 0;
 	}
Index: 2.6.24/drivers/usb/storage/jumpshot.c
===================================================================
--- 2.6.24.orig/drivers/usb/storage/jumpshot.c
+++ 2.6.24/drivers/usb/storage/jumpshot.c
@@ -119,8 +119,7 @@ static int jumpshot_read_data(struct us_
 	unsigned char  thistime;
 	unsigned int totallen, alloclen;
 	int len, result;
-	unsigned int sg_offset = 0;
-	struct scatterlist *sg = NULL;
+	struct xfer_buf_params xbp;
 
 	// we're working in LBA mode.  according to the ATA spec, 
 	// we can support up to 28-bit addressing.  I don't know if Jumpshot
@@ -141,6 +140,7 @@ static int jumpshot_read_data(struct us_
 	if (buffer == NULL)
 		return USB_STOR_TRANSPORT_ERROR;
 
+	usb_stor_init_xfer_buf(&xbp, us, TO_XFER_BUF);
 	do {
 		// loop, never allocate or transfer more than 64k at once
 		// (min(128k, 255*info->ssize) is the real limit)
@@ -170,13 +170,13 @@ static int jumpshot_read_data(struct us_
 		US_DEBUGP("jumpshot_read_data:  %d bytes\n", len);
 
 		// Store the data in the transfer buffer
-		usb_stor_access_xfer_buf(buffer, len, us->srb,
-				 &sg, &sg_offset, TO_XFER_BUF);
+		usb_stor_access_xfer_buf(buffer, len, &xbp);
 
 		sector += thistime;
 		totallen -= len;
 	} while (totallen > 0);
 
+	usb_stor_finish_xfer_buf(&xbp);
 	kfree(buffer);
 	return USB_STOR_TRANSPORT_GOOD;
 
@@ -196,8 +196,7 @@ static int jumpshot_write_data(struct us
 	unsigned char  thistime;
 	unsigned int totallen, alloclen;
 	int len, result, waitcount;
-	unsigned int sg_offset = 0;
-	struct scatterlist *sg = NULL;
+	struct xfer_buf_params xbp;
 
 	// we're working in LBA mode.  according to the ATA spec, 
 	// we can support up to 28-bit addressing.  I don't know if Jumpshot
@@ -218,6 +217,7 @@ static int jumpshot_write_data(struct us
 	if (buffer == NULL)
 		return USB_STOR_TRANSPORT_ERROR;
 
+	usb_stor_init_xfer_buf(&xbp, us, FROM_XFER_BUF);
 	do {
 		// loop, never allocate or transfer more than 64k at once
 		// (min(128k, 255*info->ssize) is the real limit)
@@ -226,8 +226,7 @@ static int jumpshot_write_data(struct us
 		thistime = (len / info->ssize) & 0xff;
 
 		// Get the data from the transfer buffer
-		usb_stor_access_xfer_buf(buffer, len, us->srb,
-				&sg, &sg_offset, FROM_XFER_BUF);
+		usb_stor_access_xfer_buf(buffer, len, &xbp);
 
 		command[0] = 0;
 		command[1] = thistime;
@@ -269,6 +268,7 @@ static int jumpshot_write_data(struct us
 		totallen -= len;
 	} while (totallen > 0);
 
+	usb_stor_finish_xfer_buf(&xbp);
 	kfree(buffer);
 	return result;
 
@@ -415,7 +415,7 @@ static int jumpshot_handle_mode_sense(st
 		ptr[0] = i - 1;
 	else
 		((__be16 *) ptr)[0] = cpu_to_be16(i - 2);
-	usb_stor_set_xfer_buf(ptr, i, srb);
+	usb_stor_set_xfer_buf(ptr, i, us);
 
 	return USB_STOR_TRANSPORT_GOOD;
 }
@@ -477,7 +477,7 @@ int jumpshot_transport(struct scsi_cmnd 
 		//
 		((__be32 *) ptr)[0] = cpu_to_be32(info->sectors - 1);
 		((__be32 *) ptr)[1] = cpu_to_be32(info->ssize);
-		usb_stor_set_xfer_buf(ptr, 8, srb);
+		usb_stor_set_xfer_buf(ptr, 8, us);
 
 		return USB_STOR_TRANSPORT_GOOD;
 	}
@@ -548,7 +548,7 @@ int jumpshot_transport(struct scsi_cmnd 
 		ptr[7] = 11;
 		ptr[12] = info->sense_asc;
 		ptr[13] = info->sense_ascq;
-		usb_stor_set_xfer_buf(ptr, 18, srb);
+		usb_stor_set_xfer_buf(ptr, 18, us);
 
 		return USB_STOR_TRANSPORT_GOOD;
 	}
Index: 2.6.24/drivers/usb/storage/sddr09.c
===================================================================
--- 2.6.24.orig/drivers/usb/storage/sddr09.c
+++ 2.6.24/drivers/usb/storage/sddr09.c
@@ -705,9 +705,9 @@ sddr09_read_data(struct us_data *us,
 	unsigned char *buffer;
 	unsigned int lba, maxlba, pba;
 	unsigned int page, pages;
-	unsigned int len, offset;
-	struct scatterlist *sg;
+	unsigned int len;
 	int result;
+	struct xfer_buf_params xbp;
 
 	// Figure out the initial LBA and page
 	lba = address >> info->blockshift;
@@ -731,8 +731,7 @@ sddr09_read_data(struct us_data *us,
 	// contiguous LBA's. Another exercise left to the student.
 
 	result = 0;
-	offset = 0;
-	sg = NULL;
+	usb_stor_init_xfer_buf(&xbp, us, TO_XFER_BUF);
 
 	while (sectors > 0) {
 
@@ -778,14 +777,14 @@ sddr09_read_data(struct us_data *us,
 		}
 
 		// Store the data in the transfer buffer
-		usb_stor_access_xfer_buf(buffer, len, us->srb,
-				&sg, &offset, TO_XFER_BUF);
+		usb_stor_access_xfer_buf(buffer, len, &xbp);
 
 		page = 0;
 		lba++;
 		sectors -= pages;
 	}
 
+	usb_stor_finish_xfer_buf(&xbp);
 	kfree(buffer);
 	return result;
 }
@@ -933,9 +932,9 @@ sddr09_write_data(struct us_data *us,
 	unsigned int pagelen, blocklen;
 	unsigned char *blockbuffer;
 	unsigned char *buffer;
-	unsigned int len, offset;
-	struct scatterlist *sg;
+	unsigned int len;
 	int result;
+	struct xfer_buf_params xbp;
 
 	// Figure out the initial LBA and page
 	lba = address >> info->blockshift;
@@ -971,8 +970,7 @@ sddr09_write_data(struct us_data *us,
 	}
 
 	result = 0;
-	offset = 0;
-	sg = NULL;
+	usb_stor_init_xfer_buf(&xbp, us, FROM_XFER_BUF);
 
 	while (sectors > 0) {
 
@@ -990,8 +988,7 @@ sddr09_write_data(struct us_data *us,
 		}
 
 		// Get the data from the transfer buffer
-		usb_stor_access_xfer_buf(buffer, len, us->srb,
-				&sg, &offset, FROM_XFER_BUF);
+		usb_stor_access_xfer_buf(buffer, len, &xbp);
 
 		result = sddr09_write_lba(us, lba, page, pages,
 				buffer, blockbuffer);
@@ -1003,6 +1000,7 @@ sddr09_write_data(struct us_data *us,
 		sectors -= pages;
 	}
 
+	usb_stor_finish_xfer_buf(&xbp);
 	kfree(buffer);
 	kfree(blockbuffer);
 
@@ -1480,7 +1478,7 @@ int sddr09_transport(struct scsi_cmnd *s
 		ptr[2] = sensekey;
 		ptr[7] = 11;
 		ptr[12] = sensecode;
-		usb_stor_set_xfer_buf(ptr, 18, srb);
+		usb_stor_set_xfer_buf(ptr, 18, us);
 		sensekey = sensecode = havefakesense = 0;
 		return USB_STOR_TRANSPORT_GOOD;
 	}
@@ -1532,7 +1530,7 @@ int sddr09_transport(struct scsi_cmnd *s
 		// Report page size
 
 		((__be32 *) ptr)[1] = cpu_to_be32(info->pagesize);
-		usb_stor_set_xfer_buf(ptr, 8, srb);
+		usb_stor_set_xfer_buf(ptr, 8, us);
 
 		return USB_STOR_TRANSPORT_GOOD;
 	}
@@ -1550,7 +1548,7 @@ int sddr09_transport(struct scsi_cmnd *s
 			memcpy(ptr, mode_page_01, sizeof(mode_page_01));
 			((__be16*)ptr)[0] = cpu_to_be16(sizeof(mode_page_01) - 2);
 			ptr[3] = (info->flags & SDDR09_WP) ? 0x80 : 0;
-			usb_stor_set_xfer_buf(ptr, sizeof(mode_page_01), srb);
+			usb_stor_set_xfer_buf(ptr, sizeof(mode_page_01), us);
 			return USB_STOR_TRANSPORT_GOOD;
 		}
 
Index: 2.6.24/drivers/usb/storage/sddr55.c
===================================================================
--- 2.6.24.orig/drivers/usb/storage/sddr55.c
+++ 2.6.24/drivers/usb/storage/sddr55.c
@@ -167,8 +167,8 @@ static int sddr55_read_data(struct us_da
 	unsigned long address;
 
 	unsigned short pages;
-	unsigned int len, offset;
-	struct scatterlist *sg;
+	unsigned int len;
+	struct xfer_buf_params xbp;
 
 	// Since we only read in one block at a time, we have to create
 	// a bounce buffer and move the data a piece at a time between the
@@ -179,8 +179,7 @@ static int sddr55_read_data(struct us_da
 	buffer = kmalloc(len, GFP_NOIO);
 	if (buffer == NULL)
 		return USB_STOR_TRANSPORT_ERROR; /* out of memory */
-	offset = 0;
-	sg = NULL;
+	usb_stor_init_xfer_buf(&xbp, us, TO_XFER_BUF);
 
 	while (sectors>0) {
 
@@ -256,14 +255,14 @@ static int sddr55_read_data(struct us_da
 		}
 
 		// Store the data in the transfer buffer
-		usb_stor_access_xfer_buf(buffer, len, us->srb,
-				&sg, &offset, TO_XFER_BUF);
+		usb_stor_access_xfer_buf(buffer, len, &xbp);
 
 		page = 0;
 		lba++;
 		sectors -= pages >> info->smallpageshift;
 	}
 
+	usb_stor_finish_xfer_buf(&xbp);
 	result = USB_STOR_TRANSPORT_GOOD;
 
 leave:
@@ -289,8 +288,8 @@ static int sddr55_write_data(struct us_d
 
 	unsigned short pages;
 	int i;
-	unsigned int len, offset;
-	struct scatterlist *sg;
+	unsigned int len;
+	struct xfer_buf_params xbp;
 
 	/* check if we are allowed to write */
 	if (info->read_only || info->force_read_only) {
@@ -307,8 +306,7 @@ static int sddr55_write_data(struct us_d
 	buffer = kmalloc(len, GFP_NOIO);
 	if (buffer == NULL)
 		return USB_STOR_TRANSPORT_ERROR;
-	offset = 0;
-	sg = NULL;
+	usb_stor_init_xfer_buf(&xbp, us, FROM_XFER_BUF);
 
 	while (sectors > 0) {
 
@@ -325,8 +323,7 @@ static int sddr55_write_data(struct us_d
 		len = pages << info->pageshift;
 
 		// Get the data from the transfer buffer
-		usb_stor_access_xfer_buf(buffer, len, us->srb,
-				&sg, &offset, FROM_XFER_BUF);
+		usb_stor_access_xfer_buf(buffer, len, &xbp);
 
 		US_DEBUGP("Write %02X pages, to PBA %04X"
 			" (LBA %04X) page %02X\n",
@@ -472,6 +469,7 @@ static int sddr55_write_data(struct us_d
 		sectors -= pages >> info->smallpageshift;
 	}
 	result = USB_STOR_TRANSPORT_GOOD;
+	usb_stor_finish_xfer_buf(&xbp);
 
  leave:
 	kfree(buffer);
@@ -770,7 +768,7 @@ int sddr55_transport(struct scsi_cmnd *s
 		memcpy (ptr, info->sense_data, sizeof info->sense_data);
 		ptr[0] = 0x70;
 		ptr[7] = 11;
-		usb_stor_set_xfer_buf (ptr, sizeof info->sense_data, srb);
+		usb_stor_set_xfer_buf(ptr, sizeof info->sense_data, us);
 		memset (info->sense_data, 0, sizeof info->sense_data);
 
 		return USB_STOR_TRANSPORT_GOOD;
@@ -835,7 +833,7 @@ int sddr55_transport(struct scsi_cmnd *s
 
 		((__be32 *) ptr)[0] = cpu_to_be32(capacity);
 		((__be32 *) ptr)[1] = cpu_to_be32(PAGESIZE);
-		usb_stor_set_xfer_buf(ptr, 8, srb);
+		usb_stor_set_xfer_buf(ptr, 8, us);
 
 		sddr55_read_map(us);
 
@@ -846,7 +844,7 @@ int sddr55_transport(struct scsi_cmnd *s
 
 		memcpy(ptr, mode_page_01, sizeof mode_page_01);
 		ptr[3] = (info->read_only || info->force_read_only) ? 0x80 : 0;
-		usb_stor_set_xfer_buf(ptr, sizeof(mode_page_01), srb);
+		usb_stor_set_xfer_buf(ptr, sizeof(mode_page_01), us);
 
 		if ( (srb->cmnd[2] & 0x3F) == 0x01 ) {
 			US_DEBUGP(
Index: 2.6.24/drivers/usb/storage/shuttle_usbat.c
===================================================================
--- 2.6.24.orig/drivers/usb/storage/shuttle_usbat.c
+++ 2.6.24/drivers/usb/storage/shuttle_usbat.c
@@ -993,8 +993,7 @@ static int usbat_flash_read_data(struct 
 	unsigned char  thistime;
 	unsigned int totallen, alloclen;
 	int len, result;
-	unsigned int sg_offset = 0;
-	struct scatterlist *sg = NULL;
+	struct xfer_buf_params xbp;
 
 	result = usbat_flash_check_media(us, info);
 	if (result != USB_STOR_TRANSPORT_GOOD)
@@ -1023,6 +1022,7 @@ static int usbat_flash_read_data(struct 
 	if (buffer == NULL)
 		return USB_STOR_TRANSPORT_ERROR;
 
+	usb_stor_init_xfer_buf(&xbp, us, TO_XFER_BUF);
 	do {
 		/*
 		 * loop, never allocate or transfer more than 64k at once
@@ -1047,13 +1047,13 @@ static int usbat_flash_read_data(struct 
 		US_DEBUGP("usbat_flash_read_data:  %d bytes\n", len);
 	
 		/* Store the data in the transfer buffer */
-		usb_stor_access_xfer_buf(buffer, len, us->srb,
-					 &sg, &sg_offset, TO_XFER_BUF);
+		usb_stor_access_xfer_buf(buffer, len, &xbp);
 
 		sector += thistime;
 		totallen -= len;
 	} while (totallen > 0);
 
+	usb_stor_finish_xfer_buf(&xbp);
 	kfree(buffer);
 	return USB_STOR_TRANSPORT_GOOD;
 
@@ -1084,8 +1084,7 @@ static int usbat_flash_write_data(struct
 	unsigned char  thistime;
 	unsigned int totallen, alloclen;
 	int len, result;
-	unsigned int sg_offset = 0;
-	struct scatterlist *sg = NULL;
+	struct xfer_buf_params xbp;
 
 	result = usbat_flash_check_media(us, info);
 	if (result != USB_STOR_TRANSPORT_GOOD)
@@ -1114,6 +1113,7 @@ static int usbat_flash_write_data(struct
 	if (buffer == NULL)
 		return USB_STOR_TRANSPORT_ERROR;
 
+	usb_stor_init_xfer_buf(&xbp, us, FROM_XFER_BUF);
 	do {
 		/*
 		 * loop, never allocate or transfer more than 64k at once
@@ -1123,8 +1123,7 @@ static int usbat_flash_write_data(struct
 		thistime = (len / info->ssize) & 0xff;
 
 		/* Get the data from the transfer buffer */
-		usb_stor_access_xfer_buf(buffer, len, us->srb,
-					 &sg, &sg_offset, FROM_XFER_BUF);
+		usb_stor_access_xfer_buf(buffer, len, &xbp);
 
 		/* ATA command 0x30 (WRITE SECTORS) */
 		usbat_pack_ata_sector_cmd(command, thistime, sector, 0x30);
@@ -1143,6 +1142,7 @@ static int usbat_flash_write_data(struct
 		totallen -= len;
 	} while (totallen > 0);
 
+	usb_stor_finish_xfer_buf(&xbp);
 	kfree(buffer);
 	return result;
 
@@ -1164,8 +1164,7 @@ static int usbat_hp8200e_handle_read10(s
 	unsigned char *buffer;
 	unsigned int len;
 	unsigned int sector;
-	unsigned int sg_offset = 0;
-	struct scatterlist *sg = NULL;
+	struct xfer_buf_params xbp;
 
 	US_DEBUGP("handle_read10: transfersize %d\n",
 		srb->transfersize);
@@ -1222,6 +1221,7 @@ static int usbat_hp8200e_handle_read10(s
 	sector |= short_pack(data[7+5], data[7+4]);
 	transferred = 0;
 
+	usb_stor_init_xfer_buf(&xbp, us, TO_XFER_BUF);
 	while (transferred != srb->request_bufflen) {
 
 		if (len > srb->request_bufflen - transferred)
@@ -1253,8 +1253,7 @@ static int usbat_hp8200e_handle_read10(s
 			break;
 
 		/* Store the data in the transfer buffer */
-		usb_stor_access_xfer_buf(buffer, len, srb,
-				 &sg, &sg_offset, TO_XFER_BUF);
+		usb_stor_access_xfer_buf(buffer, len, &xbp);
 
 		/* Update the amount transferred and the sector number */
 
@@ -1263,6 +1262,7 @@ static int usbat_hp8200e_handle_read10(s
 
 	} /* while transferred != srb->request_bufflen */
 
+	usb_stor_finish_xfer_buf(&xbp);
 	kfree(buffer);
 	return result;
 }
@@ -1603,7 +1603,7 @@ static int usbat_flash_transport(struct 
 		 */
 		((__be32 *) ptr)[0] = cpu_to_be32(info->sectors - 1);
 		((__be32 *) ptr)[1] = cpu_to_be32(info->ssize);
-		usb_stor_set_xfer_buf(ptr, 8, srb);
+		usb_stor_set_xfer_buf(ptr, 8, us);
 
 		return USB_STOR_TRANSPORT_GOOD;
 	}
@@ -1681,7 +1681,7 @@ static int usbat_flash_transport(struct 
 		ptr[7] = 11;
 		ptr[12] = info->sense_asc;
 		ptr[13] = info->sense_ascq;
-		usb_stor_set_xfer_buf(ptr, 18, srb);
+		usb_stor_set_xfer_buf(ptr, 18, us);
 
 		return USB_STOR_TRANSPORT_GOOD;
 	}
Index: 2.6.24/drivers/usb/storage/usb.c
===================================================================
--- 2.6.24.orig/drivers/usb/storage/usb.c
+++ 2.6.24/drivers/usb/storage/usb.c
@@ -299,7 +299,7 @@ void fill_inquiry_response(struct us_dat
 		data[35] = 0x30 + ((bcdDevice) & 0x0F);
 	}
 
-	usb_stor_set_xfer_buf(data, data_len, us->srb);
+	usb_stor_set_xfer_buf(data, data_len, us);
 }
 
 static int usb_stor_control_thread(void * __us)

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 1329 bytes --]

On Wed, Feb 06, 2008 at 03:23:39PM -0500, Alan Stern wrote:
> On Tue, 5 Feb 2008, Matthew Dharm wrote:
> 
> > Six of one and a half-dozen of the other.  All we're arguing over is the
> > definition of "correct behavior" here.  You want to change the API so that
> > overrun is acceptable and handled; I prefer calling it a Bad Thing(tm).
> > 
> > We both agree that the code shouldn't run off the end of the s-g list.
> > 
> > Since you've already committed to updating the patch, then we can do it
> > your way.  Just make sure it's very very clear in the comments.
> 
> Okay, here's my version.  It makes some significant changes to the 
> interface for usb_stor_access_xfer_buf() -- in particular, the context 
> information is now stored in an opaque structure rather than in ad-hoc 
> local variables.  All the callers are updated to use the new interface.

Maybe this is a crazy question, but...

Why is this not in the SCSI core?  It's hardly USB-specific, and I'm
willing to bet that there are other HCDs (at least spb2) which need to do
this sort of thing...

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

YOU SEE!!?? It's like being born with only one nipple!
					-- Erwin
User Friendly, 10/19/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Alan Stern]

On Wed, 6 Feb 2008, Matthew Dharm wrote:

> Maybe this is a crazy question, but...
> 
> Why is this not in the SCSI core?

Or even in the block core?

>  It's hardly USB-specific, and I'm
> willing to bet that there are other HCDs (at least spb2) which need to do
> this sort of thing...

James, do you have any idea?

What we're talking about is a routine that provides drivers a simple
way to access the data in a scatter-gather buffer (which may lie in
highmem or otherwise not be easily reachable).  The idea is that some 
commands are emulated by the driver rather than carried out by the 
device, and the driver needs some way to stick the results in the 
transfer buffer.

Alan Stern

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[James Bottomley]

On Wed, 2008-02-06 at 17:18 -0500, Alan Stern wrote:
> On Wed, 6 Feb 2008, Matthew Dharm wrote:
> 
> > Maybe this is a crazy question, but...
> > 
> > Why is this not in the SCSI core?
> 
> Or even in the block core?
> 
> >  It's hardly USB-specific, and I'm
> > willing to bet that there are other HCDs (at least spb2) which need to do
> > this sort of thing...
> 
> James, do you have any idea?
> 
> What we're talking about is a routine that provides drivers a simple
> way to access the data in a scatter-gather buffer (which may lie in
> highmem or otherwise not be easily reachable).  The idea is that some 
> commands are emulated by the driver rather than carried out by the 
> device, and the driver needs some way to stick the results in the 
> transfer buffer.

Isn't that what scsi_kmap_sg() is designed for ... or do you need
something slightly different?

James

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Alan Stern]

On Wed, 6 Feb 2008, James Bottomley wrote:

> > What we're talking about is a routine that provides drivers a simple
> > way to access the data in a scatter-gather buffer (which may lie in
> > highmem or otherwise not be easily reachable).  The idea is that some 
> > commands are emulated by the driver rather than carried out by the 
> > device, and the driver needs some way to stick the results in the 
> > transfer buffer.
> 
> Isn't that what scsi_kmap_sg() is designed for ... or do you need
> something slightly different?

I don't know -- I've never heard of scsi_kmap_sg().  And it doesn't 
appear to exist anywhere in my kernel source.

Did you mean scsi_kmap_atomic_sg()?  That appears to do only part of
what usb_stor_access_xfer_buf() does.  In fact, all it does is map a
single page.

Alan Stern

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[James Bottomley]

On Wed, 2008-02-06 at 18:25 -0500, Alan Stern wrote:
> On Wed, 6 Feb 2008, James Bottomley wrote:
> 
> > > What we're talking about is a routine that provides drivers a simple
> > > way to access the data in a scatter-gather buffer (which may lie in
> > > highmem or otherwise not be easily reachable).  The idea is that some 
> > > commands are emulated by the driver rather than carried out by the 
> > > device, and the driver needs some way to stick the results in the 
> > > transfer buffer.
> > 
> > Isn't that what scsi_kmap_sg() is designed for ... or do you need
> > something slightly different?
> 
> I don't know -- I've never heard of scsi_kmap_sg().  And it doesn't 
> appear to exist anywhere in my kernel source.
> 
> Did you mean scsi_kmap_atomic_sg()?

Yes .. I replied from memory rather than looking in the source.

>   That appears to do only part of
> what usb_stor_access_xfer_buf() does.  In fact, all it does is map a
> single page.

Um, it does everything you asked about above.  It's designed as a helper
for SCSI devices that need to modify data for automatically generated
returns (RAID devices and INQUIRY strings for instance).  It's also used
by some of the less well automated devices that might need to do an
effective PIO feed on DMA engine halts.  It allows you to address a sg
list by offset and length (although it will adjust the length if there's
a mapping problem).  Since the only way to guarantee access to highmem
is by kmap_atomic (which has limited slots), any API that does this sort
of thing is necessarily page based and does single page mappings at a
time.  Why don't you tell me what you think is missing rather than me
having to dig around in the usb sources to try to work it out.

James

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Alan Stern]

On Wed, 6 Feb 2008, James Bottomley wrote:

> > Did you mean scsi_kmap_atomic_sg()?
> 
> Yes .. I replied from memory rather than looking in the source.
> 
> >   That appears to do only part of
> > what usb_stor_access_xfer_buf() does.  In fact, all it does is map a
> > single page.
> 
> Um, it does everything you asked about above.  It's designed as a helper
> for SCSI devices that need to modify data for automatically generated
> returns (RAID devices and INQUIRY strings for instance).  It's also used
> by some of the less well automated devices that might need to do an
> effective PIO feed on DMA engine halts.  It allows you to address a sg
> list by offset and length (although it will adjust the length if there's
> a mapping problem).  Since the only way to guarantee access to highmem
> is by kmap_atomic (which has limited slots), any API that does this sort
> of thing is necessarily page based and does single page mappings at a
> time.  Why don't you tell me what you think is missing rather than me
> having to dig around in the usb sources to try to work it out.

Sure.  The USB routine takes as arguments a pointer to a memory buffer,
a length, a scsi_cmnd, an offset value, and a direction flag.  It
copies data between the memory buffer and the scsi_cmnd's transfer
buffer, starting at the specified offset in the transfer buffer.  It
works regardless of whether use_sg is 0 in the scsi_cmnd, by mapping
the pages in the scatter-gather list one at a time (if use_sg > 0) and
looping over these pages as needed.  It also updates the offset value
so that a subsequent call will pick up from where the current call left
off, allowing a large transfer buffer to be read/written in multiple
smaller increments.

The added value here is that the USB routine automatically keeps track
of all the details and automatically does the copying.  The caller
doesn't have to worry about anything other than supplying the memory
buffer and the length.

Comparing the USB routine with the SCSI routine shows one potential 
weakness: The USB routine uses kmap() and not kmap_atomic().  Perhaps 
it should call scsi_kmap_atomic_sg() to do that part of its work.  (And 
perhaps the whole thing should be moved into the SCSI core.)

Alan Stern

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Alan Stern]

On Tue, 5 Feb 2008, Matthew Dharm wrote:

> We both agree that the code shouldn't run off the end of the s-g list.

Incidentally, if people want a simple bugfix patch for 2.6.24.stable, 
this should do it.  Mark, can you confirm that this patch alone fixes 
the problem?

Alan Stern



Index: 2.6.24/drivers/usb/storage/protocol.c
===================================================================
--- 2.6.24.orig/drivers/usb/storage/protocol.c
+++ 2.6.24/drivers/usb/storage/protocol.c
@@ -194,7 +194,7 @@ unsigned int usb_stor_access_xfer_buf(un
 		 * and the starting offset within the page, and update
 		 * the *offset and *index values for the next loop. */
 		cnt = 0;
-		while (cnt < buflen) {
+		while (cnt < buflen && sg) {
 			struct page *page = sg_page(sg) +
 					((sg->offset + *offset) >> PAGE_SHIFT);
 			unsigned int poff =
@@ -249,6 +249,7 @@ void usb_stor_set_xfer_buf(unsigned char
 	unsigned int offset = 0;
 	struct scatterlist *sg = NULL;
 
+	buflen = min(buflen, srb->request_bufflen);
 	usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
 			TO_XFER_BUF);
 	if (buflen < srb->request_bufflen)

Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c

[Mark Glines]

On Fri, 8 Feb 2008 11:46:13 -0500 (EST)
Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:

> On Tue, 5 Feb 2008, Matthew Dharm wrote:
> 
> > We both agree that the code shouldn't run off the end of the s-g
> > list.
> 
> Incidentally, if people want a simple bugfix patch for 2.6.24.stable, 
> this should do it.  Mark, can you confirm that this patch alone fixes 
> the problem?

Confirmed: works just fine.

Tested-by: Mark Glines <mark-6pk7R1svBr8dnm+yROfE0A@public.gmane.org>


> 
> Alan Stern
> 
> 
> 
> Index: 2.6.24/drivers/usb/storage/protocol.c
> ===================================================================
> --- 2.6.24.orig/drivers/usb/storage/protocol.c
> +++ 2.6.24/drivers/usb/storage/protocol.c
> @@ -194,7 +194,7 @@ unsigned int usb_stor_access_xfer_buf(un
>  		 * and the starting offset within the page, and
> update
>  		 * the *offset and *index values for the next loop.
> */ cnt = 0;
> -		while (cnt < buflen) {
> +		while (cnt < buflen && sg) {
>  			struct page *page = sg_page(sg) +
>  					((sg->offset + *offset) >>
> PAGE_SHIFT); unsigned int poff =
> @@ -249,6 +249,7 @@ void usb_stor_set_xfer_buf(unsigned char
>  	unsigned int offset = 0;
>  	struct scatterlist *sg = NULL;
>  
> +	buflen = min(buflen, srb->request_bufflen);
>  	usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
>  			TO_XFER_BUF);
>  	if (buflen < srb->request_bufflen)
>