Re: [Re: Linux 2.6.26-rc2] Write protect on on

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Alan Stern]

Summary: 2.6.26-rc2 doesn't detect a USB drive's write-protect setting 
correctly.

On Sat, 17 May 2008, Maciej Rutecki wrote:

> 2.6.25.4 (works fine):
> http://unixy.pl/maciek/download/kernel/2.6.25.4/syslog_debug.txt
> http://unixy.pl/maciek/download/kernel/2.6.25.4/usbmon.txt
> 
> 2.6.26-rc2 ("write protect is on" problem; can't mount device):
> http://unixy.pl/maciek/download/kernel/2.6.26-rc2/syslog_debug.txt
> http://unixy.pl/maciek/download/kernel/2.6.26-rc2/usbmon.txt

I'm not sure exactly what changed to cause this regression, but the 
problem lies in the SCSI layer, not the USB layer.

The logs show that in response to the 192-byte MODE SENSE command (used
to read the write-protect status), the device sends back no data, good
status, and Residue = 192.  The SCSI core ignores the Residue and
thinks that the old left-over data in the buffer (in this case left
over from the READ CAPACITY command) actually indicates the
write-protect status -- which it obviously doesn't.

Boaz, is scsi_mode_sense() the right place to check for this sort of 
thing?  It probably should be treated the same as an Illegal Request 
error.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Boaz Harrosh]

Alan Stern wrote:
> Summary: 2.6.26-rc2 doesn't detect a USB drive's write-protect setting 
> correctly.
> 
> On Sat, 17 May 2008, Maciej Rutecki wrote:
> 
>> 2.6.25.4 (works fine):
>> http://unixy.pl/maciek/download/kernel/2.6.25.4/syslog_debug.txt
>> http://unixy.pl/maciek/download/kernel/2.6.25.4/usbmon.txt
>>
>> 2.6.26-rc2 ("write protect is on" problem; can't mount device):
>> http://unixy.pl/maciek/download/kernel/2.6.26-rc2/syslog_debug.txt
>> http://unixy.pl/maciek/download/kernel/2.6.26-rc2/usbmon.txt
> 
> I'm not sure exactly what changed to cause this regression, but the 
> problem lies in the SCSI layer, not the USB layer.
> 
> The logs show that in response to the 192-byte MODE SENSE command (used
> to read the write-protect status), the device sends back no data, good
> status, and Residue = 192.  The SCSI core ignores the Residue and
> thinks that the old left-over data in the buffer (in this case left
> over from the READ CAPACITY command) actually indicates the
> write-protect status -- which it obviously doesn't.
> 
> Boaz, is scsi_mode_sense() the right place to check for this sort of 
> thing?  It probably should be treated the same as an Illegal Request 
> error.
> 
> Alan Stern
> 

Do you mean this diff below:

@@ -796,133 +789,133 @@ kernel: usb-storage: *** thread awakened
 kernel: usb-storage: Command MODE_SENSE (6 bytes)
 kernel: usb-storage:  1a 00 3f 00 c0 00
 kernel: usb-storage: Bulk Command S 0x43425355 T 0x4 L 192 F 128 Trg 0 LUN 0 CL 6
 kernel: usb-storage: usb_stor_bulk_transfer_buf: xfer 31 bytes
 kernel: usb-storage: Status code 0; transferred 31/31
 kernel: usb-storage: -- transfer complete
 kernel: usb-storage: Bulk command transfer result=0
 kernel: usb-storage: usb_stor_bulk_transfer_sglist: xfer 192 bytes, 1 entries
 kernel: usb-storage: Status code -32; transferred 0/192
 kernel: usb-storage: clearing endpoint halt for pipe 0xc0008480
 kernel: usb-storage: usb_stor_control_msg: rq=01 rqtype=02 value=0000 index=81 len=0
 kernel: usb-storage: usb_stor_clear_halt: result = 0
 kernel: usb-storage: Bulk data transfer result 0x2
 kernel: usb-storage: Attempting to get CSW...
 kernel: usb-storage: usb_stor_bulk_transfer_buf: xfer 13 bytes
 kernel: usb-storage: Status code 0; transferred 13/13
 kernel: usb-storage: -- transfer complete
 kernel: usb-storage: Bulk status result = 0
 kernel: usb-storage: Bulk Status S 0x53425355 T 0x4 R 192 Stat 0x0
 kernel: usb-storage: scsi cmd done, result=0x0
 kernel: usb-storage: *** thread sleeping.
-kernel: sd 2:0:0:0: [sda] Write Protect is off
-kernel: sd 2:0:0:0: [sda] Mode Sense: 00 00 00 00
+kernel: sd 2:0:0:0: [sda] Write Protect is on
+kernel: sd 2:0:0:0: [sda] Mode Sense: 09 50 f8 af
 kernel: sd 2:0:0:0: [sda] Assuming drive cache: write through
 kernel: usb-storage: queuecommand called

("+" is the new kernel and "-" the older one)

It looks like it used to be the same exact return and everything only that at
old kernel the 4 bytes used to be zero and now they are not.

So It looks to me that it never used to work (Data was never actually read
from device) but by luck, the garbage data used to be a better default 
"Write Protect is off"

I do not think it is legal in scsi to return "Nothing was read" with no
error condition. You are probably right that we do not at all check resid
if status is 0, even though short reads are allowed with out error status
in some cases, as per command. But this is not the case here here nothing
was read at all, status must be returned. Or even worse if this command 
is mandatory by scsi but not supported by some USB devices then it will
have to be emulated by usb_storage.

My $0.017
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Alan Stern]

On Sun, 18 May 2008, Boaz Harrosh wrote:

> Do you mean this diff below:
> 
> @@ -796,133 +789,133 @@ kernel: usb-storage: *** thread awakened
>  kernel: usb-storage: Command MODE_SENSE (6 bytes)
>  kernel: usb-storage:  1a 00 3f 00 c0 00
>  kernel: usb-storage: Bulk Command S 0x43425355 T 0x4 L 192 F 128 Trg 0 LUN 0 CL 6
>  kernel: usb-storage: usb_stor_bulk_transfer_buf: xfer 31 bytes
>  kernel: usb-storage: Status code 0; transferred 31/31
>  kernel: usb-storage: -- transfer complete
>  kernel: usb-storage: Bulk command transfer result=0
>  kernel: usb-storage: usb_stor_bulk_transfer_sglist: xfer 192 bytes, 1 entries
>  kernel: usb-storage: Status code -32; transferred 0/192
>  kernel: usb-storage: clearing endpoint halt for pipe 0xc0008480
>  kernel: usb-storage: usb_stor_control_msg: rq=01 rqtype=02 value=0000 index=81 len=0
>  kernel: usb-storage: usb_stor_clear_halt: result = 0
>  kernel: usb-storage: Bulk data transfer result 0x2
>  kernel: usb-storage: Attempting to get CSW...
>  kernel: usb-storage: usb_stor_bulk_transfer_buf: xfer 13 bytes
>  kernel: usb-storage: Status code 0; transferred 13/13
>  kernel: usb-storage: -- transfer complete
>  kernel: usb-storage: Bulk status result = 0
>  kernel: usb-storage: Bulk Status S 0x53425355 T 0x4 R 192 Stat 0x0
>  kernel: usb-storage: scsi cmd done, result=0x0
>  kernel: usb-storage: *** thread sleeping.
> -kernel: sd 2:0:0:0: [sda] Write Protect is off
> -kernel: sd 2:0:0:0: [sda] Mode Sense: 00 00 00 00
> +kernel: sd 2:0:0:0: [sda] Write Protect is on
> +kernel: sd 2:0:0:0: [sda] Mode Sense: 09 50 f8 af
>  kernel: sd 2:0:0:0: [sda] Assuming drive cache: write through
>  kernel: usb-storage: queuecommand called
> 
> ("+" is the new kernel and "-" the older one)

That's right.

> It looks like it used to be the same exact return and everything only that at
> old kernel the 4 bytes used to be zero and now they are not.
> 
> So It looks to me that it never used to work (Data was never actually read
> from device) but by luck, the garbage data used to be a better default 
> "Write Protect is off"

Yes, it never worked properly.  But now it fails in a bad way whereas 
before it failed in a benign way.

> I do not think it is legal in scsi to return "Nothing was read" with no
> error condition.

I'm not aware of any part of the spec where it says that.  In any case 
it doesn't matter what the spec says; we ought to be able to drive this 
device even if it isn't compliant with the spec.

> You are probably right that we do not at all check resid
> if status is 0, even though short reads are allowed with out error status
> in some cases, as per command. But this is not the case here here nothing
> was read at all, status must be returned. Or even worse if this command 
> is mandatory by scsi but not supported by some USB devices then it will
> have to be emulated by usb_storage.

The real question is how should we fix the problem.  For the sake of 
argument, let's say that we fix it by changing scsi_mode_sense() -- 
make the routine return 0 if the residue is so large that there isn't 
even a valid header.

But how can this be done?  Should we modify struct scsi_sense_hdr, by 
adding a "residue" field?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Boaz Harrosh]

Alan Stern wrote:
> On Sun, 18 May 2008, Boaz Harrosh wrote:
> 
>> Do you mean this diff below:
>>
>> @@ -796,133 +789,133 @@ kernel: usb-storage: *** thread awakened
>>  kernel: usb-storage: Command MODE_SENSE (6 bytes)
>>  kernel: usb-storage:  1a 00 3f 00 c0 00
>>  kernel: usb-storage: Bulk Command S 0x43425355 T 0x4 L 192 F 128 Trg 0 LUN 0 CL 6
>>  kernel: usb-storage: usb_stor_bulk_transfer_buf: xfer 31 bytes
>>  kernel: usb-storage: Status code 0; transferred 31/31
>>  kernel: usb-storage: -- transfer complete
>>  kernel: usb-storage: Bulk command transfer result=0
>>  kernel: usb-storage: usb_stor_bulk_transfer_sglist: xfer 192 bytes, 1 entries
>>  kernel: usb-storage: Status code -32; transferred 0/192
>>  kernel: usb-storage: clearing endpoint halt for pipe 0xc0008480
>>  kernel: usb-storage: usb_stor_control_msg: rq=01 rqtype=02 value=0000 index=81 len=0
>>  kernel: usb-storage: usb_stor_clear_halt: result = 0
>>  kernel: usb-storage: Bulk data transfer result 0x2
>>  kernel: usb-storage: Attempting to get CSW...
>>  kernel: usb-storage: usb_stor_bulk_transfer_buf: xfer 13 bytes
>>  kernel: usb-storage: Status code 0; transferred 13/13
>>  kernel: usb-storage: -- transfer complete
>>  kernel: usb-storage: Bulk status result = 0
>>  kernel: usb-storage: Bulk Status S 0x53425355 T 0x4 R 192 Stat 0x0
>>  kernel: usb-storage: scsi cmd done, result=0x0
>>  kernel: usb-storage: *** thread sleeping.
>> -kernel: sd 2:0:0:0: [sda] Write Protect is off
>> -kernel: sd 2:0:0:0: [sda] Mode Sense: 00 00 00 00
>> +kernel: sd 2:0:0:0: [sda] Write Protect is on
>> +kernel: sd 2:0:0:0: [sda] Mode Sense: 09 50 f8 af
>>  kernel: sd 2:0:0:0: [sda] Assuming drive cache: write through
>>  kernel: usb-storage: queuecommand called
>>
>> ("+" is the new kernel and "-" the older one)
> 
> That's right.
> 
>> It looks like it used to be the same exact return and everything only that at
>> old kernel the 4 bytes used to be zero and now they are not.
>>
>> So It looks to me that it never used to work (Data was never actually read
>> from device) but by luck, the garbage data used to be a better default 
>> "Write Protect is off"
> 
> Yes, it never worked properly.  But now it fails in a bad way whereas 
> before it failed in a benign way.
> 

You do realize that, that was pure lock to have a zero'ed buffer.

>> I do not think it is legal in scsi to return "Nothing was read" with no
>> error condition.
> 
> I'm not aware of any part of the spec where it says that.  In any case 
> it doesn't matter what the spec says; we ought to be able to drive this 
> device even if it isn't compliant with the spec.
> 

Yes it is so. In read_x command a short read is an error status (or short
writes for write_x). The only commands that are allowed short reads 
(with no error) are those commands that have optional sizes like inquiry
and stuff, so if the device does not have the extra info it will return
a short read, of exact number of bytes as permitted by the shorter version
of the command. The initiator will understand that the command return is of
the shorter version. And also REQUEST_SENSE can be short. But all other 
commands must return the number of bytes requested or set a status. Here
the device does not return a shorter MODE_SENSE, it does not return it
at all. A status must be set/emulated.

>> You are probably right that we do not at all check resid
>> if status is 0, even though short reads are allowed with out error status
>> in some cases, as per command. But this is not the case here here nothing
>> was read at all, status must be returned. Or even worse if this command 
>> is mandatory by scsi but not supported by some USB devices then it will
>> have to be emulated by usb_storage.
> 
> The real question is how should we fix the problem.  For the sake of 
> argument, let's say that we fix it by changing scsi_mode_sense() -- 
> make the routine return 0 if the residue is so large that there isn't 
> even a valid header.
> 
> But how can this be done?  Should we modify struct scsi_sense_hdr, by 
> adding a "residue" field?
> 

I would not do that, because:
- This is a per command filter and will not be easy to do right.
- All other drivers will need to be inspected, because mid-layer
  behavior change. Where before the gate was status, then if set
  look at resid. What you propose enables another gate for errors.
  I'm much more lazy then you, and for my peace of mine will not do
  such a subtle change, that I can never fully test for.

If I where you I would fix it at the usb level, where if *no* data
was read/written I would atleast set the DID_NOT_CONNECT status.
(OK I'm not sure what to do here it should be carefully considered)

Also be ware that I think the MODE_SENSE command is mandatory and
you must emulate it if the device does not supports it. (That is
filter for MODE_SENSE command and if returns error, set a sensible
returned buffer with info gathered by other means. Like good'ol 
isd200 does it)

> Alan Stern
> 

Boaz

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Linus Torvalds]

On Mon, 19 May 2008, Boaz Harrosh wrote:

> Alan Stern wrote:
> > 
> > Yes, it never worked properly.  But now it fails in a bad way whereas 
> > before it failed in a benign way.
> 
> You do realize that, that was pure lock to have a zero'ed buffer.

Umm. Maybe it SHOULD NOT HAVE BEEN!

The thing is, if we can get partial results back, we really *should* 
either error out, or we should have at least cleared the buffer (either 
beforehand or when seeing the partial result). Returning a buffer with the 
old random contents is a bug.

And if clearing the buffer not only avoids any security holes and possible 
undefined behavior, but _also_ ends up fixing the write protect sense 
issue, all the better!

		Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Boaz Harrosh]

Linus Torvalds wrote:
> 
> On Mon, 19 May 2008, Boaz Harrosh wrote:
> 
>> Alan Stern wrote:
>>> Yes, it never worked properly.  But now it fails in a bad way whereas 
>>> before it failed in a benign way.
>> You do realize that, that was pure lock to have a zero'ed buffer.
> 
> Umm. Maybe it SHOULD NOT HAVE BEEN!
> 
> The thing is, if we can get partial results back, we really *should* 
> either error out, or we should have at least cleared the buffer (either 
> beforehand or when seeing the partial result). Returning a buffer with the 
> old random contents is a bug.
> 
> And if clearing the buffer not only avoids any security holes and possible 
> undefined behavior, but _also_ ends up fixing the write protect sense 
> issue, all the better!
> 
> 		Linus
> --

Sure, inspecting other places that emulate MODE_SENSE, (And inspecting the scsi
spec) all zeros is a very good scsi response. Alan do you want to send a fix for all
places that initiate a MODE_SENSE command, specifically at
scsi_scan.c::scsi_unlock_floptical() ? (Some other places do)

Boaz

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Linus Torvalds]

On Mon, 19 May 2008, Boaz Harrosh wrote:
> 
> Sure, inspecting other places that emulate MODE_SENSE, (And inspecting the scsi
> spec) all zeros is a very good scsi response. Alan do you want to send a fix for all
> places that initiate a MODE_SENSE command, specifically at
> scsi_scan.c::scsi_unlock_floptical() ? (Some other places do)

I was actualyl more thinking that the safest thing to do would be to just 
pre-clear the sense buffer. Then, if some driver doesn't fill it 
correctly, big deal..

It's not like pre-clearing the bugger is a performance issue.

		Linus

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Boaz Harrosh]

Linus Torvalds wrote:
> 
> On Mon, 19 May 2008, Boaz Harrosh wrote:
>> Sure, inspecting other places that emulate MODE_SENSE, (And inspecting the scsi
>> spec) all zeros is a very good scsi response. Alan do you want to send a fix for all
>> places that initiate a MODE_SENSE command, specifically at
>> scsi_scan.c::scsi_unlock_floptical() ? (Some other places do)
> 
> I was actualyl more thinking that the safest thing to do would be to just 
> pre-clear the sense buffer. Then, if some driver doesn't fill it 
> correctly, big deal..

The sense buffer *is* always cleared and it is mandated by scsi spec.
But the problem above is that the actual data buffer for read, had 
garbage data, and *no* read was actually preformed do to none-standard 
device response.

> 
> It's not like pre-clearing the bugger is a performance issue.
> 
> 		Linus
> --

Boaz

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Matthew Wilcox]

On Mon, May 19, 2008 at 10:27:17AM -0700, Linus Torvalds wrote:
> On Mon, 19 May 2008, Boaz Harrosh wrote:
> > 
> > Sure, inspecting other places that emulate MODE_SENSE, (And inspecting the scsi
> > spec) all zeros is a very good scsi response. Alan do you want to send a fix for all
> > places that initiate a MODE_SENSE command, specifically at
> > scsi_scan.c::scsi_unlock_floptical() ? (Some other places do)
> 
> I was actualyl more thinking that the safest thing to do would be to just 
> pre-clear the sense buffer. Then, if some driver doesn't fill it 
> correctly, big deal..
> 
> It's not like pre-clearing the bugger is a performance issue.

Actually, I think it might be.  I don't have any hard evidence for this
right now, but I'll let you know if I do.  There's certainly a lot of L2
cache misses in the scsi command submission path.

I'm not arguing that speed should trump performance, just we might
want to not clear the sense buffer if we know we have a non-USB device.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Alan Stern]

On Mon, 19 May 2008, Boaz Harrosh wrote:

> Sure, inspecting other places that emulate MODE_SENSE, (And inspecting the scsi
> spec) all zeros is a very good scsi response. Alan do you want to send a fix for all
> places that initiate a MODE_SENSE command, specifically at
> scsi_scan.c::scsi_unlock_floptical() ? (Some other places do)

I can't send such a fix because at these places the residue information 
has already been lost.  The fix needs to be made lower down.


Besides, everybody seems to be missing an important point.

MODE SENSE is just one example of a command for which a device might
return less data than the host expected.  In principle the same thing
could happen with _any_ command.  The host should be prepared for this
and should be able to handle it correctly.  And the host shouldn't
blindly assume that devices will slavishly follow the letter of the
SCSI spec.

We need something much more thorough than just fiddling with
scsi_mode_sense().  One possibility would be to pass a
minimum-response-length argument to scsi_execute_req().  But even that
wouldn't catch all the code paths where this sort of thing could  
happen (although it probably would catch most of them).

This needs someone who is more familiar than I am with the operation of
the SCSI core.
  
Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Alan Stern]

On Tue, 20 May 2008, Boaz Harrosh wrote:

> Alan Stern wrote:
> > On Mon, 19 May 2008, Boaz Harrosh wrote:
> > 
> >> Sure, inspecting other places that emulate MODE_SENSE, (And inspecting the scsi
> >> spec) all zeros is a very good scsi response. Alan do you want to send a fix for all
> >> places that initiate a MODE_SENSE command, specifically at
> >> scsi_scan.c::scsi_unlock_floptical() ? (Some other places do)
> > 
> > I can't send such a fix because at these places the residue information 
> > has already been lost.  The fix needs to be made lower down.
> > 
> 
> I was talking about zero-set the data buffer before issue of the 
> MODE_SENSE command. (As it accidentally happen to be before)

The data buffer _is_ set to zero bufore the MODE SENSE command is
issued.  It doesn't help, because the device sends garbage data along
with a Residue value saying that none of the data it sent was valid.

> > Besides, everybody seems to be missing an important point.
> > 
> 
> No, you are missing the point.
> 
> > MODE SENSE is just one example of a command for which a device might
> > return less data than the host expected.  In principle the same thing
> > could happen with _any_ command.  The host should be prepared for this
> > and should be able to handle it correctly.  And the host shouldn't
> > blindly assume that devices will slavishly follow the letter of the
> > SCSI spec.
> > 
> 
> LLD's do a good job upto now. If *NO* data was written to a target they
> return some error status.

We are talking about data being _read_ from a device, not _written_ to 
a device.  MODE SENSE does a read, not a write.

> You keep returning to, "what if we wrote less and the target
> did not signal an error status". Well, up to now we did not see such 
> thing, let's cross that when we see it with black lists or something.

Why use a blacklist?  This sort of thing is allowed by the USB mass 
storage spec, and probably also by the SCSI spec.  The core should be 
able to handle it.

> > We need something much more thorough than just fiddling with
> > scsi_mode_sense().  One possibility would be to pass a
> > minimum-response-length argument to scsi_execute_req().  But even that
> > wouldn't catch all the code paths where this sort of thing could  
> > happen (although it probably would catch most of them).
> > 
> It never happens! LLD's set proper status when something gone wrong.

What do you mean by "gone wrong"?  The device didn't send an error 
status, so in that sense nothing is wrong.

It also didn't send the data we want.  But how is the LLD supposed to 
know how much data is truly needed?  The only mechanism for that is 
scmd->underflow.  usb-storage does indeed test that, as it should.

Apparently the SCSI core should start setting scmd->underflow in cases
where it currently doesn't.

> > This needs someone who is more familiar than I am with the operation of
> > the SCSI core.
> >
> 
> The SCSI core assumes the LLD will return some status condition. 
> If an LLD trusts it's device it will blindly return what the device returned,
> if not it will do some checks of it's own. You can see examples of both in 
> the source tree.

Your description bears no relation to what usb-storage does.  It does 
_not_ blindly trust the device; it _does_ check status conditions, and 
it _does_ do checks of its own.

It's not usb-storage's fault that the SCSI core failed to tell it how
much data was required to be transferred.

Alan Stern

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[James Bottomley]

On Mon, 2008-05-19 at 10:27 -0700, Linus Torvalds wrote:
> 
> On Mon, 19 May 2008, Boaz Harrosh wrote:
> > 
> > Sure, inspecting other places that emulate MODE_SENSE, (And inspecting the scsi
> > spec) all zeros is a very good scsi response. Alan do you want to send a fix for all
> > places that initiate a MODE_SENSE command, specifically at
> > scsi_scan.c::scsi_unlock_floptical() ? (Some other places do)
> 
> I was actualyl more thinking that the safest thing to do would be to just 
> pre-clear the sense buffer. Then, if some driver doesn't fill it 
> correctly, big deal..
> 
> It's not like pre-clearing the bugger is a performance issue.

It already is pre cleared, for precisely the reason that some devices
lie about returning sense, so we use the zero return to mean no sense
was collected.

James

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Alan Stern]

On Tue, 20 May 2008, Boaz Harrosh wrote:

> Alan Stern wrote:
> 
> > MODE SENSE is just one example of a command for which a device might
> > return less data than the host expected.  In principle the same thing
> > could happen with _any_ command.  The host should be prepared for this
> > and should be able to handle it correctly.  And the host shouldn't
> > blindly assume that devices will slavishly follow the letter of the
> > SCSI spec.

...

> > We need something much more thorough than just fiddling with
> > scsi_mode_sense().  One possibility would be to pass a
> > minimum-response-length argument to scsi_execute_req().  But even that
> > wouldn't catch all the code paths where this sort of thing could  
> > happen (although it probably would catch most of them).

What do you think of a patch like this?

Index: usb-2.6/include/linux/blkdev.h
===================================================================
--- usb-2.6.orig/include/linux/blkdev.h
+++ usb-2.6/include/linux/blkdev.h
@@ -221,6 +221,7 @@ struct request {
 
 	unsigned int data_len;
 	unsigned int extra_len;	/* length of alignment and padding */
+	unsigned int min_data_len;
 	unsigned int sense_len;
 	void *data;
 	void *sense;
Index: usb-2.6/drivers/scsi/scsi_lib.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_lib.c
+++ usb-2.6/drivers/scsi/scsi_lib.c
@@ -1125,6 +1125,7 @@ int scsi_setup_blk_pc_cmnd(struct scsi_d
 		ret = scsi_init_io(cmd, GFP_ATOMIC);
 		if (unlikely(ret))
 			return ret;
+		cmd->underflow = req->min_data_len;
 	} else {
 		BUG_ON(req->data_len);
 		BUG_ON(req->data);

Combined with an extra minimum-data-length argument to scsi_execute()
and scsi_execute_req(), this ought to solve the problem.

(To refresh your memory: The problem is that a weird device responds to 
MODE SENSE with Residue equal to the data length -- so none of the 
returned data is valid -- and Okay status.)

Alan Stern

P.S.: Maybe a safer approach would be to add a new flag bit in struct
request.  Normally the flag would be clear, indicating that for
BLOCK_PC requests, scmd->underflow should be set to 0.  But when the
flag is set, scmd->underflow would be set to req->data_len.

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Maciej Rutecki]

2008/6/3 Alan Stern <stern@rowland.harvard.edu>:
[...]
>
> What do you think of a patch like this?
>
> Index: usb-2.6/include/linux/blkdev.h
> ===================================================================
> --- usb-2.6.orig/include/linux/blkdev.h
> +++ usb-2.6/include/linux/blkdev.h
> @@ -221,6 +221,7 @@ struct request {
>
>        unsigned int data_len;
>        unsigned int extra_len; /* length of alignment and padding */
> +       unsigned int min_data_len;
>        unsigned int sense_len;
>        void *data;
>        void *sense;
> Index: usb-2.6/drivers/scsi/scsi_lib.c
> ===================================================================
> --- usb-2.6.orig/drivers/scsi/scsi_lib.c
> +++ usb-2.6/drivers/scsi/scsi_lib.c
> @@ -1125,6 +1125,7 @@ int scsi_setup_blk_pc_cmnd(struct scsi_d
>                ret = scsi_init_io(cmd, GFP_ATOMIC);
>                if (unlikely(ret))
>                        return ret;
> +               cmd->underflow = req->min_data_len;
>        } else {
>                BUG_ON(req->data_len);
>                BUG_ON(req->data);
>
> Combined with an extra minimum-data-length argument to scsi_execute()
> and scsi_execute_req(), this ought to solve the problem.
>
> (To refresh your memory: The problem is that a weird device responds to
> MODE SENSE with Residue equal to the data length -- so none of the
> returned data is valid -- and Okay status.)
>
> Alan Stern
>
> P.S.: Maybe a safer approach would be to add a new flag bit in struct
> request.  Normally the flag would be clear, indicating that for
> BLOCK_PC requests, scmd->underflow should be set to 0.  But when the
> flag is set, scmd->underflow would be set to req->data_len.
>
>

It does'n help. I still have "write protect is on" message. Also I see
"usb-storage: queuecommand called" message.

Log (2.6.26-rc2):
http://www.unixy.pl/maciek/download/kernel/2.6.26-rc2/20080613/syslog

-- 
Maciej Rutecki
http://www.maciek.unixy.pl

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Alan Stern]

On Fri, 13 Jun 2008, Maciej Rutecki wrote:

> 2008/6/3 Alan Stern <stern@rowland.harvard.edu>:
> [...]
> >
> > What do you think of a patch like this?

...

> > (To refresh your memory: The problem is that a weird device responds to
> > MODE SENSE with Residue equal to the data length -- so none of the
> > returned data is valid -- and Okay status.)
> >
> > Alan Stern

> It does'n help. I still have "write protect is on" message. Also I see
> "usb-storage: queuecommand called" message.

That patch wasn't meant to help; it wasn't complete.  It was meant to 
stimulate conversation -- and clearly it failed.

Below is a complete patch.  It's very ugly and isn't likely to get
accepted, but maybe it will convince people to start talking about the
problem.  Maybe it will offend people's sensibilities so that they will
just _have_ to chime in, if only to complain about how bad the patch
is...

Alan Stern



Index: 2.6.26-rc5/include/linux/blkdev.h
===================================================================
--- 2.6.26-rc5.orig/include/linux/blkdev.h
+++ 2.6.26-rc5/include/linux/blkdev.h
@@ -221,6 +221,7 @@ struct request {
 
 	unsigned int data_len;
 	unsigned int extra_len;	/* length of alignment and padding */
+	unsigned int min_data_len;
 	unsigned int sense_len;
 	void *data;
 	void *sense;
Index: 2.6.26-rc5/include/scsi/scsi_device.h
===================================================================
--- 2.6.26-rc5.orig/include/scsi/scsi_device.h
+++ 2.6.26-rc5/include/scsi/scsi_device.h
@@ -324,9 +324,17 @@ extern int scsi_execute(struct scsi_devi
 			int data_direction, void *buffer, unsigned bufflen,
 			unsigned char *sense, int timeout, int retries,
 			int flag);
+extern int scsi_execute_min(struct scsi_device *sdev, const unsigned char *cmd,
+			int data_direction, void *buffer, unsigned bufflen,
+			unsigned char *sense, int timeout, int retries,
+			int flag, unsigned min_data);
 extern int scsi_execute_req(struct scsi_device *sdev, const unsigned char *cmd,
 			    int data_direction, void *buffer, unsigned bufflen,
 			    struct scsi_sense_hdr *, int timeout, int retries);
+extern int scsi_execute_req_min(struct scsi_device *sdev, const unsigned char *cmd,
+			    int data_direction, void *buffer, unsigned bufflen,
+			    struct scsi_sense_hdr *, int timeout, int retries,
+			    unsigned min_data);
 extern int scsi_execute_async(struct scsi_device *sdev,
 			      const unsigned char *cmd, int cmd_len, int data_direction,
 			      void *buffer, unsigned bufflen, int use_sg,
Index: 2.6.26-rc5/drivers/scsi/scsi_lib.c
===================================================================
--- 2.6.26-rc5.orig/drivers/scsi/scsi_lib.c
+++ 2.6.26-rc5/drivers/scsi/scsi_lib.c
@@ -215,6 +215,44 @@ int scsi_execute(struct scsi_device *sde
 }
 EXPORT_SYMBOL(scsi_execute);
 
+int scsi_execute_min(struct scsi_device *sdev, const unsigned char *cmd,
+		 int data_direction, void *buffer, unsigned bufflen,
+		 unsigned char *sense, int timeout, int retries, int flags,
+		 unsigned min_data)
+{
+	struct request *req;
+	int write = (data_direction == DMA_TO_DEVICE);
+	int ret = DRIVER_ERROR << 24;
+
+	req = blk_get_request(sdev->request_queue, write, __GFP_WAIT);
+
+	if (bufflen &&	blk_rq_map_kern(sdev->request_queue, req,
+					buffer, bufflen, __GFP_WAIT))
+		goto out;
+
+	req->cmd_len = COMMAND_SIZE(cmd[0]);
+	memcpy(req->cmd, cmd, req->cmd_len);
+	req->sense = sense;
+	req->sense_len = 0;
+	req->retries = retries;
+	req->timeout = timeout;
+	req->cmd_type = REQ_TYPE_BLOCK_PC;
+	req->cmd_flags |= flags | REQ_QUIET | REQ_PREEMPT;
+	req->min_data_len = min_data;
+
+	/*
+	 * head injection *required* here otherwise quiesce won't work
+	 */
+	blk_execute_rq(req->q, NULL, req, 1);
+
+	ret = req->errors;
+ out:
+	blk_put_request(req);
+
+	return ret;
+}
+EXPORT_SYMBOL(scsi_execute_min);
+
 
 int scsi_execute_req(struct scsi_device *sdev, const unsigned char *cmd,
 		     int data_direction, void *buffer, unsigned bufflen,
@@ -238,6 +276,29 @@ int scsi_execute_req(struct scsi_device 
 }
 EXPORT_SYMBOL(scsi_execute_req);
 
+int scsi_execute_req_min(struct scsi_device *sdev, const unsigned char *cmd,
+		     int data_direction, void *buffer, unsigned bufflen,
+		     struct scsi_sense_hdr *sshdr, int timeout, int retries,
+		     unsigned min_data)
+{
+	char *sense = NULL;
+	int result;
+
+	if (sshdr) {
+		sense = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO);
+		if (!sense)
+			return DRIVER_ERROR << 24;
+	}
+	result = scsi_execute_min(sdev, cmd, data_direction, buffer, bufflen,
+			      sense, timeout, retries, 0, min_data);
+	if (sshdr)
+		scsi_normalize_sense(sense, SCSI_SENSE_BUFFERSIZE, sshdr);
+
+	kfree(sense);
+	return result;
+}
+EXPORT_SYMBOL(scsi_execute_req_min);
+
 struct scsi_io_context {
 	void *data;
 	void (*done)(void *data, char *sense, int result, int resid);
@@ -1125,6 +1186,7 @@ int scsi_setup_blk_pc_cmnd(struct scsi_d
 		ret = scsi_init_io(cmd, GFP_ATOMIC);
 		if (unlikely(ret))
 			return ret;
+		cmd->underflow = req->min_data_len;
 	} else {
 		BUG_ON(req->data_len);
 		BUG_ON(req->data);
@@ -1879,8 +1941,8 @@ scsi_mode_sense(struct scsi_device *sdev
 
 	memset(buffer, 0, len);
 
-	result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer, len,
-				  sshdr, timeout, retries);
+	result = scsi_execute_req_min(sdev, cmd, DMA_FROM_DEVICE, buffer, len,
+				  sshdr, timeout, retries, header_length);
 
 	/* This code looks awful: what it's doing is making sure an
 	 * ILLEGAL REQUEST sense return identifies the actual command

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Maciej Rutecki]

2008/6/13 Alan Stern <stern@rowland.harvard.edu>:

[...]

>
> That patch wasn't meant to help; it wasn't complete.  It was meant to
> stimulate conversation -- and clearly it failed.
>
> Below is a complete patch.  It's very ugly and isn't likely to get
> accepted, but maybe it will convince people to start talking about the
> problem.  Maybe it will offend people's sensibilities so that they will
> just _have_ to chime in, if only to complain about how bad the patch
> is...

[...]

Tested with 2.6.26-rc2 and works fine. Thanks.

-- 
Maciej Rutecki
http://www.maciek.unixy.pl

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Alan Stern]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 916 bytes --]

On Sat, 14 Jun 2008, Maciej Rutecki wrote:

> 2008/6/13 Alan Stern <stern@rowland.harvard.edu>:
> 
> [...]
> 
> >
> > That patch wasn't meant to help; it wasn't complete.  It was meant to
> > stimulate conversation -- and clearly it failed.
> >
> > Below is a complete patch.  It's very ugly and isn't likely to get
> > accepted, but maybe it will convince people to start talking about the
> > problem.  Maybe it will offend people's sensibilities so that they will
> > just _have_ to chime in, if only to complain about how bad the patch
> > is...
> 
> [...]
> 
> Tested with 2.6.26-rc2 and works fine. Thanks.

Not having received any comments on that earlier patch, I wrote a new
version.  Actually it's a pair of patches, and they have to be applied
in order.  They don't look as ugly as the old one and they have a
decent chance of being accepted.

If they also fix your problem, I'll submit them.

Alan Stern

[-- Attachment #2: Type: TEXT/PLAIN, Size: 11839 bytes --]

Index: usb-2.6/include/scsi/scsi_device.h
===================================================================
--- usb-2.6.orig/include/scsi/scsi_device.h
+++ usb-2.6/include/scsi/scsi_device.h
@@ -323,10 +323,11 @@ extern int scsi_is_target_device(const s
 extern int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 			int data_direction, void *buffer, unsigned bufflen,
 			unsigned char *sense, int timeout, int retries,
-			int flag);
+			int flag, unsigned *residue);
 extern int scsi_execute_req(struct scsi_device *sdev, const unsigned char *cmd,
 			    int data_direction, void *buffer, unsigned bufflen,
-			    struct scsi_sense_hdr *, int timeout, int retries);
+			    struct scsi_sense_hdr *, int timeout, int retries,
+			    unsigned *residue);
 extern int scsi_execute_async(struct scsi_device *sdev,
 			      const unsigned char *cmd, int cmd_len, int data_direction,
 			      void *buffer, unsigned bufflen, int use_sg,
Index: usb-2.6/drivers/ata/libata-scsi.c
===================================================================
--- usb-2.6.orig/drivers/ata/libata-scsi.c
+++ usb-2.6/drivers/ata/libata-scsi.c
@@ -332,7 +332,7 @@ int ata_cmd_ioctl(struct scsi_device *sc
 	/* Good values for timeout and retries?  Values below
 	   from scsi_ioctl_send_command() for default case... */
 	cmd_result = scsi_execute(scsidev, scsi_cmd, data_dir, argbuf, argsize,
-				  sensebuf, (10*HZ), 5, 0);
+				  sensebuf, (10*HZ), 5, 0, NULL);
 
 	if (driver_byte(cmd_result) == DRIVER_SENSE) {/* sense data available */
 		u8 *desc = sensebuf + 8;
@@ -418,7 +418,7 @@ int ata_task_ioctl(struct scsi_device *s
 	/* Good values for timeout and retries?  Values below
 	   from scsi_ioctl_send_command() for default case... */
 	cmd_result = scsi_execute(scsidev, scsi_cmd, DMA_NONE, NULL, 0,
-				sensebuf, (10*HZ), 5, 0);
+				sensebuf, (10*HZ), 5, 0, NULL);
 
 	if (driver_byte(cmd_result) == DRIVER_SENSE) {/* sense data available */
 		u8 *desc = sensebuf + 8;
Index: usb-2.6/drivers/scsi/ch.c
===================================================================
--- usb-2.6.orig/drivers/scsi/ch.c
+++ usb-2.6/drivers/scsi/ch.c
@@ -189,7 +189,7 @@ ch_do_scsi(scsi_changer *ch, unsigned ch
 
         result = scsi_execute_req(ch->device, cmd, direction, buffer,
 				  buflength, &sshdr, timeout * HZ,
-				  MAX_RETRIES);
+				  MAX_RETRIES, NULL);
 
 	dprintk("result: 0x%x\n",result);
 	if (driver_byte(result) & DRIVER_SENSE) {
Index: usb-2.6/drivers/scsi/scsi_ioctl.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_ioctl.c
+++ usb-2.6/drivers/scsi/scsi_ioctl.c
@@ -94,7 +94,7 @@ static int ioctl_internal_command(struct
 	SCSI_LOG_IOCTL(1, printk("Trying ioctl with scsi command %d\n", *cmd));
 
 	result = scsi_execute_req(sdev, cmd, DMA_NONE, NULL, 0,
-				  &sshdr, timeout, retries);
+				  &sshdr, timeout, retries, NULL);
 
 	SCSI_LOG_IOCTL(2, printk("Ioctl returned  0x%x\n", result));
 
Index: usb-2.6/drivers/scsi/scsi_lib.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_lib.c
+++ usb-2.6/drivers/scsi/scsi_lib.c
@@ -175,13 +175,15 @@ int scsi_queue_insert(struct scsi_cmnd *
  * @timeout:	request timeout in seconds
  * @retries:	number of times to retry request
  * @flags:	or into request flags;
+ * @residue:	number of bytes not transferred correctly
  *
  * returns the req->errors value which is the scsi_cmnd result
  * field.
  */
 int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 		 int data_direction, void *buffer, unsigned bufflen,
-		 unsigned char *sense, int timeout, int retries, int flags)
+		 unsigned char *sense, int timeout, int retries, int flags,
+		 unsigned *residue)
 {
 	struct request *req;
 	int write = (data_direction == DMA_TO_DEVICE);
@@ -207,6 +209,8 @@ int scsi_execute(struct scsi_device *sde
 	 */
 	blk_execute_rq(req->q, NULL, req, 1);
 
+	if (residue)
+		*residue = req->data_len;
 	ret = req->errors;
  out:
 	blk_put_request(req);
@@ -218,7 +222,8 @@ EXPORT_SYMBOL(scsi_execute);
 
 int scsi_execute_req(struct scsi_device *sdev, const unsigned char *cmd,
 		     int data_direction, void *buffer, unsigned bufflen,
-		     struct scsi_sense_hdr *sshdr, int timeout, int retries)
+		     struct scsi_sense_hdr *sshdr, int timeout, int retries,
+		     unsigned *residue)
 {
 	char *sense = NULL;
 	int result;
@@ -229,7 +234,7 @@ int scsi_execute_req(struct scsi_device 
 			return DRIVER_ERROR << 24;
 	}
 	result = scsi_execute(sdev, cmd, data_direction, buffer, bufflen,
-			      sense, timeout, retries, 0);
+			      sense, timeout, retries, 0, residue);
 	if (sshdr)
 		scsi_normalize_sense(sense, SCSI_SENSE_BUFFERSIZE, sshdr);
 
@@ -1815,7 +1820,7 @@ scsi_mode_select(struct scsi_device *sde
 	}
 
 	ret = scsi_execute_req(sdev, cmd, DMA_TO_DEVICE, real_buffer, len,
-			       sshdr, timeout, retries);
+			       sshdr, timeout, retries, NULL);
 	kfree(real_buffer);
 	return ret;
 }
@@ -1880,7 +1885,7 @@ scsi_mode_sense(struct scsi_device *sdev
 	memset(buffer, 0, len);
 
 	result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer, len,
-				  sshdr, timeout, retries);
+				  sshdr, timeout, retries, NULL);
 
 	/* This code looks awful: what it's doing is making sure an
 	 * ILLEGAL REQUEST sense return identifies the actual command
@@ -1962,7 +1967,7 @@ scsi_test_unit_ready(struct scsi_device 
 	/* try to eat the UNIT_ATTENTION if there are enough retries */
 	do {
 		result = scsi_execute_req(sdev, cmd, DMA_NONE, NULL, 0, sshdr,
-					  timeout, retries);
+					  timeout, retries, NULL);
 	} while ((driver_byte(result) & DRIVER_SENSE) &&
 		 sshdr && sshdr->sense_key == UNIT_ATTENTION &&
 		 --retries);
Index: usb-2.6/drivers/scsi/scsi_scan.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_scan.c
+++ usb-2.6/drivers/scsi/scsi_scan.c
@@ -216,7 +216,7 @@ static void scsi_unlock_floptical(struct
 	scsi_cmd[4] = 0x2a;     /* size */
 	scsi_cmd[5] = 0;
 	scsi_execute_req(sdev, scsi_cmd, DMA_FROM_DEVICE, result, 0x2a, NULL,
-			 SCSI_TIMEOUT, 3);
+			 SCSI_TIMEOUT, 3, NULL);
 }
 
 /**
@@ -580,7 +580,8 @@ static int scsi_probe_lun(struct scsi_de
 
 		result = scsi_execute_req(sdev,  scsi_cmd, DMA_FROM_DEVICE,
 					  inq_result, try_inquiry_len, &sshdr,
-					  HZ / 2 + HZ * scsi_inq_timeout, 3);
+					  HZ / 2 + HZ * scsi_inq_timeout, 3,
+					  NULL);
 
 		SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO "scsi scan: INQUIRY %s "
 				"with code 0x%x\n",
@@ -1376,7 +1377,7 @@ static int scsi_report_lun_scan(struct s
 
 		result = scsi_execute_req(sdev, scsi_cmd, DMA_FROM_DEVICE,
 					  lun_data, length, &sshdr,
-					  SCSI_TIMEOUT + 4 * HZ, 3);
+					  SCSI_TIMEOUT + 4 * HZ, 3, NULL);
 
 		SCSI_LOG_SCAN_BUS(3, printk (KERN_INFO "scsi scan: REPORT LUNS"
 				" %s (try %d) result 0x%x\n", result
Index: usb-2.6/drivers/scsi/scsi_transport_spi.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_transport_spi.c
+++ usb-2.6/drivers/scsi/scsi_transport_spi.c
@@ -109,7 +109,7 @@ static int spi_execute(struct scsi_devic
 	for(i = 0; i < DV_RETRIES; i++) {
 		result = scsi_execute(sdev, cmd, dir, buffer, bufflen,
 				      sense, DV_TIMEOUT, /* retries */ 1,
-				      REQ_FAILFAST);
+				      REQ_FAILFAST, NULL);
 		if (result & DRIVER_SENSE) {
 			struct scsi_sense_hdr sshdr_tmp;
 			if (!sshdr)
Index: usb-2.6/drivers/scsi/sd.c
===================================================================
--- usb-2.6.orig/drivers/scsi/sd.c
+++ usb-2.6/drivers/scsi/sd.c
@@ -842,7 +842,7 @@ static int sd_sync_cache(struct scsi_dis
 		 * flush everything.
 		 */
 		res = scsi_execute_req(sdp, cmd, DMA_NONE, NULL, 0, &sshdr,
-				       SD_TIMEOUT, SD_MAX_RETRIES);
+				       SD_TIMEOUT, SD_MAX_RETRIES, NULL);
 		if (res == 0)
 			break;
 	}
@@ -1070,7 +1070,7 @@ sd_spinup_disk(struct scsi_disk *sdkp)
 			the_result = scsi_execute_req(sdkp->device, cmd,
 						      DMA_NONE, NULL, 0,
 						      &sshdr, SD_TIMEOUT,
-						      SD_MAX_RETRIES);
+						      SD_MAX_RETRIES, NULL);
 
 			/*
 			 * If the drive has indicated to us that it
@@ -1126,7 +1126,8 @@ sd_spinup_disk(struct scsi_disk *sdkp)
 				cmd[4] = 1;	/* Start spin cycle */
 				scsi_execute_req(sdkp->device, cmd, DMA_NONE,
 						 NULL, 0, &sshdr,
-						 SD_TIMEOUT, SD_MAX_RETRIES);
+						 SD_TIMEOUT, SD_MAX_RETRIES,
+						 NULL);
 				spintime_expire = jiffies + 100 * HZ;
 				spintime = 1;
 			}
@@ -1199,7 +1200,8 @@ repeat:
 		
 		the_result = scsi_execute_req(sdp, cmd, DMA_FROM_DEVICE,
 					      buffer, longrc ? 12 : 8, &sshdr,
-					      SD_TIMEOUT, SD_MAX_RETRIES);
+					      SD_TIMEOUT, SD_MAX_RETRIES,
+					      NULL);
 
 		if (media_not_present(sdkp, &sshdr))
 			return;
@@ -1794,7 +1796,7 @@ static int sd_start_stop_device(struct s
 		return -ENODEV;
 
 	res = scsi_execute_req(sdp, cmd, DMA_NONE, NULL, 0, &sshdr,
-			       SD_TIMEOUT, SD_MAX_RETRIES);
+			       SD_TIMEOUT, SD_MAX_RETRIES, NULL);
 	if (res) {
 		sd_printk(KERN_WARNING, sdkp, "START_STOP FAILED\n");
 		sd_print_result(sdkp, res);
Index: usb-2.6/drivers/scsi/ses.c
===================================================================
--- usb-2.6.orig/drivers/scsi/ses.c
+++ usb-2.6/drivers/scsi/ses.c
@@ -77,7 +77,7 @@ static int ses_recv_diag(struct scsi_dev
 	};
 
 	return scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf, bufflen,
-				NULL, SES_TIMEOUT, SES_RETRIES);
+				NULL, SES_TIMEOUT, SES_RETRIES, NULL);
 }
 
 static int ses_send_diag(struct scsi_device *sdev, int page_code,
@@ -95,7 +95,7 @@ static int ses_send_diag(struct scsi_dev
 	};
 
 	result = scsi_execute_req(sdev, cmd, DMA_TO_DEVICE, buf, bufflen,
-				  NULL, SES_TIMEOUT, SES_RETRIES);
+				  NULL, SES_TIMEOUT, SES_RETRIES, NULL);
 	if (result)
 		sdev_printk(KERN_ERR, sdev, "SEND DIAGNOSTIC result: %8x\n",
 			    result);
@@ -369,7 +369,8 @@ static void ses_match_to_enclosure(struc
 		return;
 
 	if (scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf,
-			     VPD_INQUIRY_SIZE, NULL, SES_TIMEOUT, SES_RETRIES))
+			     VPD_INQUIRY_SIZE, NULL, SES_TIMEOUT, SES_RETRIES,
+			     NULL))
 		goto free;
 
 	len = (buf[2] << 8) + buf[3];
Index: usb-2.6/drivers/scsi/sr.c
===================================================================
--- usb-2.6.orig/drivers/scsi/sr.c
+++ usb-2.6/drivers/scsi/sr.c
@@ -177,7 +177,7 @@ int sr_test_unit_ready(struct scsi_devic
 	do {
 		the_result = scsi_execute_req(sdev, cmd, DMA_NONE, NULL,
 					      0, sshdr, SR_TIMEOUT,
-					      retries--);
+					      retries--, NULL);
 
 	} while (retries > 0 &&
 		 (!scsi_status_is_good(the_result) ||
@@ -687,7 +687,7 @@ static void get_sectorsize(struct scsi_c
 		/* Do the command and wait.. */
 		the_result = scsi_execute_req(cd->device, cmd, DMA_FROM_DEVICE,
 					      buffer, 8, NULL, SR_TIMEOUT,
-					      MAX_RETRIES);
+					      MAX_RETRIES, NULL);
 
 		retries--;
 
Index: usb-2.6/drivers/scsi/sr_ioctl.c
===================================================================
--- usb-2.6.orig/drivers/scsi/sr_ioctl.c
+++ usb-2.6/drivers/scsi/sr_ioctl.c
@@ -207,7 +207,7 @@ int sr_do_ioctl(Scsi_CD *cd, struct pack
 	memset(sense, 0, sizeof(*sense));
 	result = scsi_execute(SDev, cgc->cmd, cgc->data_direction,
 			      cgc->buffer, cgc->buflen, (char *)sense,
-			      cgc->timeout, IOCTL_RETRIES, 0);
+			      cgc->timeout, IOCTL_RETRIES, 0, NULL);
 
 	scsi_normalize_sense((char *)sense, sizeof(*sense), &sshdr);
 

[-- Attachment #3: Type: TEXT/PLAIN, Size: 1104 bytes --]

Index: usb-2.6/drivers/scsi/scsi_lib.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_lib.c
+++ usb-2.6/drivers/scsi/scsi_lib.c
@@ -1852,6 +1852,7 @@ scsi_mode_sense(struct scsi_device *sdev
 	int use_10_for_ms;
 	int header_length;
 	int result;
+	unsigned residue;
 	struct scsi_sense_hdr my_sshdr;
 
 	memset(data, 0, sizeof(*data));
@@ -1885,7 +1886,7 @@ scsi_mode_sense(struct scsi_device *sdev
 	memset(buffer, 0, len);
 
 	result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer, len,
-				  sshdr, timeout, retries, NULL);
+				  sshdr, timeout, retries, &residue);
 
 	/* This code looks awful: what it's doing is making sure an
 	 * ILLEGAL REQUEST sense return identifies the actual command
@@ -1907,6 +1908,8 @@ scsi_mode_sense(struct scsi_device *sdev
 	}
 
 	if(scsi_status_is_good(result)) {
+		if (residue > len - header_length)
+			return SAM_STAT_CHECK_CONDITION;
 		if (unlikely(buffer[0] == 0x86 && buffer[1] == 0x0b &&
 			     (modepage == 6 || modepage == 8))) {
 			/* Initio breakage? */

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[James Bottomley]

On Fri, 2008-06-20 at 16:22 -0400, Alan Stern wrote:
> On Sat, 14 Jun 2008, Maciej Rutecki wrote:
> 
> > 2008/6/13 Alan Stern <stern@rowland.harvard.edu>:
> > 
> > [...]
> > 
> > >
> > > That patch wasn't meant to help; it wasn't complete.  It was meant to
> > > stimulate conversation -- and clearly it failed.
> > >
> > > Below is a complete patch.  It's very ugly and isn't likely to get
> > > accepted, but maybe it will convince people to start talking about the
> > > problem.  Maybe it will offend people's sensibilities so that they will
> > > just _have_ to chime in, if only to complain about how bad the patch
> > > is...
> > 
> > [...]
> > 
> > Tested with 2.6.26-rc2 and works fine. Thanks.
> 
> Not having received any comments on that earlier patch, I wrote a new
> version.  Actually it's a pair of patches, and they have to be applied
> in order.  They don't look as ugly as the old one and they have a
> decent chance of being accepted.

Actually, just looking at this, what you're really trying to do is
enforce an underrun detection, which is a concept built in to the
command structure but not necessarily well implemented by all drivers.

However, I had a tick on this one to go back and look at it.

Your initial contention was that the garbage value was "left over data"
in the sense command.  However, I don't see how we're getting this into
the buffer; scsi_mode_sense() clears the buffer up to the length it's
expecting before it executes the request.  How is this getting garbage
data if nothing's returned ... surely it should still be all zeros?

James

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Alan Stern]

On Fri, 20 Jun 2008, James Bottomley wrote:

> > Not having received any comments on that earlier patch, I wrote a new
> > version.  Actually it's a pair of patches, and they have to be applied
> > in order.  They don't look as ugly as the old one and they have a
> > decent chance of being accepted.
> 
> Actually, just looking at this, what you're really trying to do is
> enforce an underrun detection, which is a concept built in to the
> command structure but not necessarily well implemented by all drivers.
> 
> However, I had a tick on this one to go back and look at it.
> 
> Your initial contention was that the garbage value was "left over data"
> in the sense command.  However, I don't see how we're getting this into
> the buffer; scsi_mode_sense() clears the buffer up to the length it's
> expecting before it executes the request.  How is this getting garbage
> data if nothing's returned ... surely it should still be all zeros?

It's not true that nothing's returned.  The device returns N bytes of
garbage data (I forget just now what N was) and sets the residue equal
to N -- meaning that none of the data was meaningful.

USB mass-storage is perhaps a little strange for people accustomed to 
regular SCSI.  Quoting from the relevant spec:

	For Data-In the device shall report in the dCSWDataResidue the 
	difference between the amount of data expected as stated in the
	dCBWDataTransferLength and the actual amount of relevant data
	sent by the device.

The key word here is "relevant".  The device is allowed to send 
non-relevant data and then tell the host to ignore it.  Later on the 
spec says:

    If the device intends to send less data than the host indicated, then: 
	The device shall send the intended data.
	    The device may send fill data to pad up to a total of dCBWDataTransferLength.
	    If the device actually transfers less data than the host indicated, then:
		The device may end the transfer with a short packet.
		The device shall STALL the Bulk-In pipe.
	The device shall set bCSWStatus to 00h or 01h.
	The device shall set dCSWDataResidue to the difference between dCBWDataTransferLength
	    and the actual amount of relevant data sent.

In this case the fill data is getting treated as real data.  Does this
clarify the situation?

Alan Stern

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[James Bottomley]

On Fri, 2008-06-20 at 17:46 -0400, Alan Stern wrote:
> On Fri, 20 Jun 2008, James Bottomley wrote:
> 
> > > Not having received any comments on that earlier patch, I wrote a new
> > > version.  Actually it's a pair of patches, and they have to be applied
> > > in order.  They don't look as ugly as the old one and they have a
> > > decent chance of being accepted.
> > 
> > Actually, just looking at this, what you're really trying to do is
> > enforce an underrun detection, which is a concept built in to the
> > command structure but not necessarily well implemented by all drivers.
> > 
> > However, I had a tick on this one to go back and look at it.
> > 
> > Your initial contention was that the garbage value was "left over data"
> > in the sense command.  However, I don't see how we're getting this into
> > the buffer; scsi_mode_sense() clears the buffer up to the length it's
> > expecting before it executes the request.  How is this getting garbage
> > data if nothing's returned ... surely it should still be all zeros?
> 
> It's not true that nothing's returned.  The device returns N bytes of
> garbage data (I forget just now what N was) and sets the residue equal
> to N -- meaning that none of the data was meaningful.
> 
> USB mass-storage is perhaps a little strange for people accustomed to 
> regular SCSI.  Quoting from the relevant spec:
> 
> 	For Data-In the device shall report in the dCSWDataResidue the 
> 	difference between the amount of data expected as stated in the
> 	dCBWDataTransferLength and the actual amount of relevant data
> 	sent by the device.
> 
> The key word here is "relevant".  The device is allowed to send 
> non-relevant data and then tell the host to ignore it.  Later on the 
> spec says:
> 
>     If the device intends to send less data than the host indicated, then: 
> 	The device shall send the intended data.
> 	    The device may send fill data to pad up to a total of dCBWDataTransferLength.
> 	    If the device actually transfers less data than the host indicated, then:
> 		The device may end the transfer with a short packet.
> 		The device shall STALL the Bulk-In pipe.
> 	The device shall set bCSWStatus to 00h or 01h.
> 	The device shall set dCSWDataResidue to the difference between dCBWDataTransferLength
> 	    and the actual amount of relevant data sent.
> 
> In this case the fill data is getting treated as real data.  Does this
> clarify the situation?

Yes, thanks.  It's a bit nasty from a security point of view, since the
leaking data apparently belonged to a different command.  Wouldn't a
better fix (and a more secure one) be to clear from the end of the valid
data to the end of the buffer?

James

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Alan Stern]

On Fri, 20 Jun 2008, James Bottomley wrote:

> > In this case the fill data is getting treated as real data.  Does this
> > clarify the situation?
> 
> Yes, thanks.  It's a bit nasty from a security point of view, since the
> leaking data apparently belonged to a different command.

Indeed; the data belonged to the previous command.  It definitely is a 
security hole.

>  Wouldn't a
> better fix (and a more secure one) be to clear from the end of the valid
> data to the end of the buffer?

It certainly would be easier and shorter.  I'll send in a patch to do
it next week.

Whether it would be _better_ is a question of taste.  I don't really
like the idea of telling a caller "Here's your data.  Some of it is
valid (but we're not going to tell you how much) and the rest is set to
0.  Do the best you can with it."  What if somebody had preset their
buffer to some value other than 0?

Alan Stern

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Alan Stern]

On Fri, 20 Jun 2008, James Bottomley wrote:

> Yes, thanks.  It's a bit nasty from a security point of view, since the
> leaking data apparently belonged to a different command.  Wouldn't a
> better fix (and a more secure one) be to clear from the end of the valid
> data to the end of the buffer?

Here's the promised patch.  Maciej and Peter, please try it out.

Alan Stern


Index: usb-2.6/drivers/scsi/scsi_lib.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_lib.c
+++ usb-2.6/drivers/scsi/scsi_lib.c
@@ -207,6 +207,15 @@ int scsi_execute(struct scsi_device *sde
 	 */
 	blk_execute_rq(req->q, NULL, req, 1);
 
+	/*
+	 * Some devices (USB mass-storage in particular) may transfer
+	 * garbage data together with a residue indicating that the data
+	 * is invalid.  Prevent the garbage from being misinterpreted
+	 * and prevent security leaks by zeroing out the excess data.
+	 */
+	if (unlikely(req->data_len > 0 && req->data_len <= bufflen))
+		memset(buffer + (bufflen - req->data_len), 0, req->data_len);
+
 	ret = req->errors;
  out:
 	blk_put_request(req);

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Peter Teoh]

On Mon, Jun 23, 2008 at 11:04 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Fri, 20 Jun 2008, James Bottomley wrote:
>
>> Yes, thanks.  It's a bit nasty from a security point of view, since the
>> leaking data apparently belonged to a different command.  Wouldn't a
>> better fix (and a more secure one) be to clear from the end of the valid
>> data to the end of the buffer?
>
> Here's the promised patch.  Maciej and Peter, please try it out.
>
> Alan Stern
>
>
> Index: usb-2.6/drivers/scsi/scsi_lib.c
> ===================================================================
> --- usb-2.6.orig/drivers/scsi/scsi_lib.c
> +++ usb-2.6/drivers/scsi/scsi_lib.c
> @@ -207,6 +207,15 @@ int scsi_execute(struct scsi_device *sde
>         */
>        blk_execute_rq(req->q, NULL, req, 1);
>
> +       /*
> +        * Some devices (USB mass-storage in particular) may transfer
> +        * garbage data together with a residue indicating that the data
> +        * is invalid.  Prevent the garbage from being misinterpreted
> +        * and prevent security leaks by zeroing out the excess data.
> +        */
> +       if (unlikely(req->data_len > 0 && req->data_len <= bufflen))
> +               memset(buffer + (bufflen - req->data_len), 0, req->data_len);
> +
>        ret = req->errors;
>  out:
>        blk_put_request(req);
>
>

Yes, it worked (based on latest linus git tree) the before and after
patch dmesg are as follows:

[  387.015562] scsi 3:0:0:0: Direct-Access     WD
1600BEAExternal  1.02 PQ: 0 ANSI: 0
[  387.018563] sd 3:0:0:0: [sde] 312581808 512-byte hardware sectors (160042 MB)
[  387.018573] sd 3:0:0:0: [sde] Write Protect is on
[  387.018583] sd 3:0:0:0: [sde] Mode Sense: 12 a1 9e af
[  387.018589] sd 3:0:0:0: [sde] Assuming drive cache: write through
[  387.020182] sd 3:0:0:0: [sde] 312581808 512-byte hardware sectors (160042 MB)
[  387.020602] sd 3:0:0:0: [sde] Write Protect is on
[  387.020602] sd 3:0:0:0: [sde] Mode Sense: 12 a1 9e af
[  387.020602] sd 3:0:0:0: [sde] Assuming drive cache: write through
[  387.020602]  sde: sde1 sde2 sde3 < sde5 > sde4
[  387.094903] sd 3:0:0:0: [sde] Attached SCSI disk
[  387.095304] sd 3:0:0:0: Attached scsi generic sg4 type 0

After patching and recompiling and reboot:

[  230.281708] sd 3:0:0:0: [sdd] 312581808 512-byte hardware sectors (160042 MB)
[  230.282708] sd 3:0:0:0: [sdd] Write Protect is off
[  230.282708] sd 3:0:0:0: [sdd] Mode Sense: 00 00 00 00
[  230.282708] sd 3:0:0:0: [sdd] Assuming drive cache: write through
[  230.283708] sd 3:0:0:0: [sdd] 312581808 512-byte hardware sectors (160042 MB)
[  230.284709] sd 3:0:0:0: [sdd] Write Protect is off
[  230.284709] sd 3:0:0:0: [sdd] Mode Sense: 00 00 00 00
[  230.284709] sd 3:0:0:0: [sdd] Assuming drive cache: write through
[  230.284709]  sdd: sdd1 sdd2 sdd3 < sdd5 > sdd4
[  230.364830] sd 3:0:0:0: [sdd] Attached SCSI disk

Thank you very much Alan.

-- 
Regards,
Peter Teoh

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Peter Teoh]

Sorry, missed this one - in case u need it:

Tested-by: Peter Teoh <htmldeveloper@gmail.com>

Thanks.

On Tue, Jun 24, 2008 at 11:25 AM, Peter Teoh <htmldeveloper@gmail.com> wrote:
> On Mon, Jun 23, 2008 at 11:04 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> On Fri, 20 Jun 2008, James Bottomley wrote:
>>
>>> Yes, thanks.  It's a bit nasty from a security point of view, since the
>>> leaking data apparently belonged to a different command.  Wouldn't a
>>> better fix (and a more secure one) be to clear from the end of the valid
>>> data to the end of the buffer?
>>
>> Here's the promised patch.  Maciej and Peter, please try it out.
>>
>> Alan Stern
>>
>>
>> Index: usb-2.6/drivers/scsi/scsi_lib.c
>> ===================================================================
>> --- usb-2.6.orig/drivers/scsi/scsi_lib.c
>> +++ usb-2.6/drivers/scsi/scsi_lib.c
>> @@ -207,6 +207,15 @@ int scsi_execute(struct scsi_device *sde
>>         */
>>        blk_execute_rq(req->q, NULL, req, 1);
>>
>> +       /*
>> +        * Some devices (USB mass-storage in particular) may transfer
>> +        * garbage data together with a residue indicating that the data
>> +        * is invalid.  Prevent the garbage from being misinterpreted
>> +        * and prevent security leaks by zeroing out the excess data.
>> +        */
>> +       if (unlikely(req->data_len > 0 && req->data_len <= bufflen))
>> +               memset(buffer + (bufflen - req->data_len), 0, req->data_len);
>> +
>>        ret = req->errors;
>>  out:
>>        blk_put_request(req);
>>
>>
>
> Yes, it worked (based on latest linus git tree) the before and after
> patch dmesg are as follows:
>
> [  387.015562] scsi 3:0:0:0: Direct-Access     WD
> 1600BEAExternal  1.02 PQ: 0 ANSI: 0
> [  387.018563] sd 3:0:0:0: [sde] 312581808 512-byte hardware sectors (160042 MB)
> [  387.018573] sd 3:0:0:0: [sde] Write Protect is on
> [  387.018583] sd 3:0:0:0: [sde] Mode Sense: 12 a1 9e af
> [  387.018589] sd 3:0:0:0: [sde] Assuming drive cache: write through
> [  387.020182] sd 3:0:0:0: [sde] 312581808 512-byte hardware sectors (160042 MB)
> [  387.020602] sd 3:0:0:0: [sde] Write Protect is on
> [  387.020602] sd 3:0:0:0: [sde] Mode Sense: 12 a1 9e af
> [  387.020602] sd 3:0:0:0: [sde] Assuming drive cache: write through
> [  387.020602]  sde: sde1 sde2 sde3 < sde5 > sde4
> [  387.094903] sd 3:0:0:0: [sde] Attached SCSI disk
> [  387.095304] sd 3:0:0:0: Attached scsi generic sg4 type 0
>
> After patching and recompiling and reboot:
>
> [  230.281708] sd 3:0:0:0: [sdd] 312581808 512-byte hardware sectors (160042 MB)
> [  230.282708] sd 3:0:0:0: [sdd] Write Protect is off
> [  230.282708] sd 3:0:0:0: [sdd] Mode Sense: 00 00 00 00
> [  230.282708] sd 3:0:0:0: [sdd] Assuming drive cache: write through
> [  230.283708] sd 3:0:0:0: [sdd] 312581808 512-byte hardware sectors (160042 MB)
> [  230.284709] sd 3:0:0:0: [sdd] Write Protect is off
> [  230.284709] sd 3:0:0:0: [sdd] Mode Sense: 00 00 00 00
> [  230.284709] sd 3:0:0:0: [sdd] Assuming drive cache: write through
> [  230.284709]  sdd: sdd1 sdd2 sdd3 < sdd5 > sdd4
> [  230.364830] sd 3:0:0:0: [sdd] Attached SCSI disk
>
> Thank you very much Alan.
>
> --
> Regards,
> Peter Teoh
>



-- 
Regards,
Peter Teoh

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Alan Stern]

On Tue, 24 Jun 2008, Peter Teoh wrote:

> Yes, it worked (based on latest linus git tree) the before and after
> patch dmesg are as follows:
...
> [  387.018573] sd 3:0:0:0: [sde] Write Protect is on

> After patching and recompiling and reboot:
...
> [  230.282708] sd 3:0:0:0: [sdd] Write Protect is off

How about you, Maciej?  I want to make sure it fixes your problem too 
before submitting the patch.

Alan Stern

Re: [Re: Linux 2.6.26-rc2] Write protect on on

[Maciej Rutecki]

2008/6/24 Alan Stern <stern@rowland.harvard.edu>:

>
> How about you, Maciej?  I want to make sure it fixes your problem too
> before submitting the patch.
>

Works great on 2.6.26-rc7:

usb 3-1: new high speed USB device using ehci_hcd and address 4
usb 3-1: configuration #1 chosen from 1 choice
Initializing USB Mass Storage driver...
scsi2 : SCSI emulation for USB Mass Storage devices
usbcore: registered new interface driver usb-storage
usb-storage: device found at 4
usb-storage: waiting for device to settle before scanning
USB Mass Storage support registered.
scsi 2:0:0:0: Direct-Access     Initio   MHV2080BH PL     3.01 PQ: 0 ANSI: 0
usb-storage: device scan complete
Driver 'sd' needs updating - please use bus_type methods
sd 2:0:0:0: [sda] 156301488 512-byte hardware sectors (80026 MB)
sd 2:0:0:0: [sda] Write Protect is off
sd 2:0:0:0: [sda] Mode Sense: 00 00 00 00
sd 2:0:0:0: [sda] Assuming drive cache: write through
sd 2:0:0:0: [sda] 156301488 512-byte hardware sectors (80026 MB)
sd 2:0:0:0: [sda] Write Protect is off
sd 2:0:0:0: [sda] Mode Sense: 00 00 00 00
sd 2:0:0:0: [sda] Assuming drive cache: write through
 sda: sda1
sd 2:0:0:0: [sda] Attached SCSI disk

maciek@zlom:~$ ntfsmount /dev/sda1 ~/mnt/ntfs/
maciek@zlom:~$ mount | grep sda1
/dev/sda1 on /home/maciek/mnt/ntfs type fuseblk
(rw,nosuid,nodev,default_permissions,allow_other,blksize=4096,user=maciek)

Thanks very much :-)

-- 
Maciej Rutecki
http://www.maciek.unixy.pl