Write protect on on

Quoted 7 lines · Alan Stern

Quoted 4 lines · Boaz Harrosh

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)

Quoted 3 lines

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)

Quoted 2 lines

Besides, everybody seems to be missing an important point.

No, you are missing the point.

Quoted 7 lines

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.

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.

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 6 lines · Alan Stern

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.

Quoted 5 lines

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

include/linux/blkdev.h +1 -0

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

drivers/scsi/scsi_lib.c +1 -0

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

2008/6/3 Alan Stern <[email protected]>:
[...]

Quoted 2 lines

What do you think of a patch like this?

Quoted 5 lines

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

2008/6/13 Alan Stern <[email protected]>:

[...]

Quoted 9 lines

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.

Quoted 14 lines · Maciej Rutecki

2008/6/13 Alan Stern <[email protected]>:

[...]

Quoted 9 lines

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

Quoted 3 lines

[...]

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.

Quoted 4 lines

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?

Quoted 9 lines · James Bottomley

Quoted 4 lines

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.

Quoted 7 lines

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?

Quoted 2 lines

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?

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?

Quoted 4 lines · James Bottomley

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

drivers/scsi/scsi_lib.c +2 -0

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

Quoted 24 lines · Alan Stern

Quoted 4 lines · James Bottomley

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

drivers/scsi/scsi_lib.c +2 -0

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

drivers/scsi/scsi_lib.c int scsi_execute(struct scsi_device *sde · line 221

Quoted 10 lines

drivers/scsi/scsi_lib.c +7 -0

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

This patch (as1108) fixes a problem that can occur with certain USB mass-storage devices: They return invalid data together with a residue indicating that the data should be ignored. Rather than leave the invalid data in a transfer buffer, where it can get misinterpreted, the patch clears the invalid portion of the buffer.

This solves a problem (wrong write-protect setting detected) reported by Maciej Rutecki and Peter Teoh.

Signed-off-by Alan Stern <[email protected]>

Tested-by Peter Teoh <[email protected]>

---

Index: usb-2.6/drivers/scsi/scsi_lib.c
===================================================================

drivers/scsi/scsi_lib.c +9 -1

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

To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

It looks like we're getting down to the last -rc; can this patch please make it into main-line before 2.6.26 goes final?

Quoted 3 lines · Cal Peake

It looks like we're getting down to the last -rc; can this patch please make it into main-line before 2.6.26 goes final?

James said he was pushing it out to his scsi-rc-fixes tree, but I haven't seen a pull request.

But you're right, final is near. Maybe I should just merge it directly.

It's one of the two patches in scsi-rc-fixes. I'm just seeing if any problems show up in linux-next. I'm playing the usual game of trying to judge how long I have left ... so I'm thinking on Monday ... is that about right?

This patch (as1108) fixes a problem that can occur with certain USB mass-storage devices: They return invalid data together with a residue indicating that the data should be ignored. Rather than leave the invalid data in a transfer buffer, where it can get misinterpreted, the patch clears the invalid portion of the buffer.

This solves a problem (wrong write-protect setting detected) reported by Maciej Rutecki and Peter Teoh.

Signed-off-by Alan Stern <[email protected]>

Tested-by Peter Teoh <[email protected]>

---

Index: usb-2.6/drivers/scsi/scsi_lib.c
===================================================================

drivers/scsi/scsi_lib.c +0 -0

@@ -207,6 +207,15 @@ int scsi_execute(struct scsi_device *sde
 	 */

Quoted 5 lines · Alan Stern

This patch (as1108) fixes a problem that can occur with certain USB mass-storage devices: They return invalid data together with a residue indicating that the data should be ignored. Rather than leave the invalid data in a transfer buffer, where it can get misinterpreted, the patch clears the invalid portion of the buffer.

I've only just stumbled upon this patch and I don't quite understand how it is supposed to work.

Quoted 15 lines

This solves a problem (wrong write-protect setting detected) reported by Maciej Rutecki and Peter Teoh.

Signed-off-by Alan Stern <[email protected]>

Tested-by Peter Teoh <[email protected]>

---

Index: usb-2.6/drivers/scsi/scsi_lib.c
===================================================================

drivers/scsi/scsi_lib.c +0 -0

@@ -207,6 +207,15 @@ int scsi_execute(struct scsi_device *sde
 	 */

drivers/scsi/scsi_lib.c int scsi_execute(struct scsi_device *sde · line 217

Quoted 10 lines

drivers/scsi/scsi_lib.c +8 -0

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

Sorry, I don't understand that line at all. Surely, we want to zero out either the excess data, i.e. buffer -> buffer + req->data_len, or the residue, i.e. buffer + req->data_len -> buffer + bufflen. Your patch implies that there are bufflen - req->data_len bytes of valid data at the beginning of buffer. If this is intentional, please bear with me and explain. Otherwise, what about the following patch to 2.6.26? On the other hand, the same could probably be achieved by setting req->data_len to 0. Oh dear, it would appear that I'm completely lost here.

Quoted 1 line · Elias Oltmanns

Alan Stern <[email protected]> wrote:

Quoted 10 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+		memset(buffer + (bufflen - req->data_len), 0, req->data_len);

Sorry, I don't understand that line at all. Surely, we want to zero out either the excess data, i.e. buffer -> buffer + req->data_len, or the residue, i.e. buffer + req->data_len -> buffer + bufflen. Your patch implies that there are bufflen - req->data_len bytes of valid data at the beginning of buffer. If this is intentional, please bear with me and explain. Otherwise, what about the following patch to 2.6.26? On the other hand, the same could probably be achieved by setting req->data_len to 0. Oh dear, it would appear that I'm completely lost here.

I think all you don't understand is simply that for a REQ_BLOCK_PC, the residue (that's the amount of untransferred, or at least bogus, data) is returned in req->data_len. Thus, after such a request completes, you have bufflen-req->data_len good bytes.

Oh, I see. The name fooled me there, I suppose. So, the meaning of req->data_len is inverted, as it were, when the LLDD performs the data transfer, right?

Thanks for explaining,

Quoted 3 lines · Elias Oltmanns

Oh, I see. The name fooled me there, I suppose. So, the meaning of req->data_len is inverted, as it were, when the LLDD performs the data transfer, right?

The inversion takes place in the midlayer, not in the LLDD. Take a look inside scsi_io_completion():

	if (blk_pc_request(req)) { /* SG_IO ioctl from block level */
		...
		req->data_len = scsi_get_resid(cmd);
	}

Quoted 5 lines · Alan Stern

This patch (as1108) fixes a problem that can occur with certain USB mass-storage devices: They return invalid data together with a residue indicating that the data should be ignored. Rather than leave the invalid data in a transfer buffer, where it can get misinterpreted, the patch clears the invalid portion of the buffer.

I've only just stumbled upon this patch and I don't quite understand how it is supposed to work.

Quoted 14 lines

This solves a problem (wrong write-protect setting detected) reported by Maciej Rutecki and Peter Teoh.

Signed-off-by Alan Stern <[email protected]>

Tested-by Peter Teoh <[email protected]>

---

Index: usb-2.6/drivers/scsi/scsi_lib.c
===================================================================

drivers/scsi/scsi_lib.c +0 -0

@@ -207,6 +207,15 @@ int scsi_execute(struct scsi_device *sde
 	 */

drivers/scsi/scsi_lib.c int scsi_execute(struct scsi_device *sde · line 217

Quoted 10 lines

drivers/scsi/scsi_lib.c +8 -0

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

Sorry, I don't understand that line at all. Surely, we want to zero out either the excess data, i.e. buffer -> buffer + req->data_len, or the residue, i.e. buffer + req->data_len -> buffer + bufflen. Your patch implies that there are bufflen - req->data_len bytes of valid data at the beginning of buffer. If this is intentional, please bear with me and explain. Otherwise, what about the following patch to 2.6.26? On the other hand, the same could probably be achieved by setting req->data_len to 0. Oh dear, it would appear that I'm completely lost here.

Elias

drivers/scsi/scsi_lib.c +0 -0

index cbf55d5..977f22b 100644

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.