2.6.24: NULL scatter-gather pointer in usb_storage:usb_stor_access_xfer_buf?

Quoted 10 lines · On Thu, 31 Jan 2008 11:27:39 +0200 Boaz Harrosh…

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.

Quoted 2 lines

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:

[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?

Mark

Quoted 12 lines

---
 drivers/usb/storage/protocol.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

(quoted diff) +0 -0

b/drivers/usb/storage/protocol.c index a41ce21..d0ff1f6 100644

drivers/usb/storage/protocol.c +0 -0

@@ -229,6 +229,12 @@ void usb_stor_set_xfer_buf(unsigned char *buffer,
 	unsigned int offset = 0;
 	struct scatterlist *sg = NULL;
 

drivers/usb/storage/protocol.c void usb_stor_set_xfer_buf(unsigned char *buffer, · line 240

Quoted 10 lines

drivers/usb/storage/protocol.c +6 -0

+	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))

Quoted 6 lines

Quoted 6 lines

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().

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

Quoted 6 lines · Boaz Harrosh

Quoted 6 lines

Quoted 6 lines

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.

Quoted 2 lines

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

-

drivers/usb/storage/protocol.c +2 -1

@@ -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,

Quoted 8 lines · Boaz Harrosh

drivers/usb/storage/protocol.c +2 -1

@@ -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,

drivers/usb/storage/protocol.c void usb_stor_set_xfer_buf(unsigned char *buffer, · line 241

Quoted 10 lines

drivers/usb/storage/protocol.c +6 -2

 			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

Quoted 26 lines · Alan Stern

Quoted 8 lines · Boaz Harrosh

drivers/usb/storage/protocol.c +2 -1

@@ -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,

drivers/usb/storage/protocol.c void usb_stor_set_xfer_buf(unsigned char *buffer, · line 241

Quoted 10 lines

drivers/usb/storage/protocol.c +6 -2

 			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.

Quoted 2 lines

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 */

Quoted 26 lines · Boaz Harrosh

Quoted 25 lines · Alan Stern

Quoted 8 lines · Boaz Harrosh

drivers/usb/storage/protocol.c +2 -1

@@ -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,

drivers/usb/storage/protocol.c void usb_stor_set_xfer_buf(unsigned char *buffer, · line 241

Quoted 10 lines

drivers/usb/storage/protocol.c +6 -2

 			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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 6 lines

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...)

Quoted 4 lines

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

Quoted 10 lines

Quoted 5 lines

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.

Quoted 4 lines

(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.

Quoted 13 lines · Boaz Harrosh

Quoted 13 lines

Quoted 5 lines

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...)

Quoted 2 lines

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

-

Quoted 2 lines · Alan Stern

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.

Quoted 3 lines

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.

Quoted 9 lines

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.

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

Quoted 11 lines · Boaz Harrosh

Quoted 2 lines · Alan Stern

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.

Quoted 4 lines

Quoted 3 lines

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?

Quoted 1 line

overflow is still an error.

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

Quoted 2 lines

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.

Quoted 11 lines

Quoted 9 lines

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.

Quoted 2 lines

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

After what?

Quoted 2 lines

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

-

From 3610cfa93c990bbbafb296134ac01ef6d426eb8d Mon Sep 17 00:00:00 2001

Quoted 1 line

From 3610cfa93c990bbbafb296134ac01ef6d426eb8d Mon Sep 17 00:00:00 2001

From: Boaz Harrosh <[email protected]>
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 <[email protected]>

---
 drivers/usb/storage/isd200.c   |    7 +++++--
 drivers/usb/storage/protocol.c |   20 ++++++++++++++++----
 2 files changed, 21 insertions(+), 6 deletions(-)

Quoted 12 lines · Boaz Harrosh

Quoted 1 line

From 3610cfa93c990bbbafb296134ac01ef6d426eb8d Mon Sep 17 00:00:00 2001

From: Boaz Harrosh <[email protected]>
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).

Quoted 8 lines

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

Signed-off-by Boaz Harrosh <[email protected]>

---
 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

From cd66d4d4a4a239e580714e926e9635f3426dd7fd Mon Sep 17 00:00:00 2001

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.

Quoted 17 lines · Matthew Dharm

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.

Quoted 4 lines

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

-

Quoted 9 lines

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.

Quoted 10 lines · Boaz Harrosh

Quoted 9 lines

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.

Quoted 8 lines

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.

Quoted 5 lines

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

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.

Quoted 6 lines · Matthew Dharm

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.

Quoted 2 lines

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...

Quoted 3 lines · Matthew Dharm

Maybe this is a crazy question, but...

Why is this not in the SCSI core?

Or even in the block core?

Quoted 3 lines

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.

Quoted 6 lines

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?

Quoted 7 lines · James Bottomley

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 1 line

Did you mean scsi_kmap_atomic_sg()?

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

Quoted 3 lines

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.

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

Quoted 2 lines · Matthew Dharm

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
===================================================================

drivers/usb/storage/protocol.c +0 -0

@@ -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;

drivers/usb/storage/protocol.c +1 -1

-		while (cnt < buflen) {
+		while (cnt < buflen && sg) {
 			struct page *page = sg_page(sg) +
 					((sg->offset + *offset) >>

PAGE_SHIFT); unsigned int poff =

drivers/usb/storage/protocol.c +1 -0

@@ -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)

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 <[email protected]>

Quoted 11 lines · 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 <[email protected]>

As this is a regression and hits 2.6.24, can you send the final version of this patch to the [email protected] address so we can get it into the 2.6.24.y tree?

thanks,

greg k-h
-

Quoted 15 lines

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

Quoted 3 lines

Thanks,

Mark

As this is a regression and hits 2.6.24, can you send the final version of this patch to the [email protected] address so we can get it into the 2.6.24.y tree?

thanks,

greg k-h

Greg KH <[email protected]> rote:

Quoted 7 lines

As this is a regression and hits 2.6.24, can you send the final version
of this patch to the [email protected] 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 <[email protected]>

---
 drivers/usb/storage/isd200.c   |    7 +++++--
 drivers/usb/storage/protocol.c |   13 +++++++++----
 2 files changed, 14 insertions(+), 6 deletions(-)

drivers/usb/storage/isd200.c +5 -2

index 49ba6c0..8186e93 100644
@@ -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)
 		{

drivers/usb/storage/protocol.c +9 -4

index 889622b..038a284 100644
@@ -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);
 }

Signature · 1 line

1.5.3.3

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

Quoted 1 line · Matthew Dharm

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)

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.

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 <[email protected]>

---
 drivers/usb/storage/isd200.c   |    7 +++++--
 drivers/usb/storage/protocol.c |   13 +++++++++----
 2 files changed, 14 insertions(+), 6 deletions(-)

(quoted diff) +0 -0

b/drivers/usb/storage/isd200.c index 49ba6c0..8186e93 100644

drivers/usb/storage/isd200.c +3 -1

@@ -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;

drivers/usb/storage/isd200.c +2 -1

 		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)

drivers/usb/storage/isd200.c +0 -0

 		{

(quoted diff) +0 -0

b/drivers/usb/storage/protocol.c index 889622b..038a284 100644

drivers/usb/storage/protocol.c +0 -0

@@ -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;

drivers/usb/storage/protocol.c +1 -1

-		while (cnt < buflen) {
+		while (cnt < buflen && sg) {
 			struct page *page = sg_page(sg) +
 					((sg->offset + *offset) >>

PAGE_SHIFT); unsigned int poff =

drivers/usb/storage/protocol.c +8 -3

@@ -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);
 }