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

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

[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

[PATCH] SCSI: erase invalid data returned by device

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

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

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Tested-by: Peter Teoh <htmldeveloper@gmail.com>

---

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: [PATCH] SCSI: erase invalid data returned by device

[Cal Peake]

Hi,

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?

Thanks!


On Tue, 24 Jun 2008, Alan Stern wrote:

> 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 <stern@rowland.harvard.edu>
> Tested-by: Peter Teoh <htmldeveloper@gmail.com>
> 
> ---
> 
> 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);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Cal Peake

Re: [PATCH] SCSI: erase invalid data returned by device

[Linus Torvalds]

On Thu, 10 Jul 2008, Cal Peake wrote:
> 
> 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.

		Linus

Re: [PATCH] SCSI: erase invalid data returned by device

[James Bottomley]

On Thu, 2008-07-10 at 16:23 -0700, Linus Torvalds wrote:
> 
> On Thu, 10 Jul 2008, Cal Peake wrote:
> > 
> > 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?

James

Re: [PATCH] SCSI: erase invalid data returned by device

[Linus Torvalds]

On Thu, 10 Jul 2008, James Bottomley wrote:
> 
> 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?

I'm not planning on a -rc10, there's not any real justification for it. 

Yes, we've had changes since -rc9, but apart from 1300+ lines of 
documentation, the rest is all really small fixes (some for problems that 
have been pending for a longish while - today has been a good day, for 
example).

Lacking a -rc10 due to major changes (which quite frankly do not look 
likely based on current regression lists or other activity), 2.6.26 would 
probably happen over the weekend. So it would be better to get any trivial 
changes in _now_, and at least give them a couple of days in -git.

		Linus

Re: [PATCH] SCSI: erase invalid data returned by device

[Elias Oltmanns]

Alan Stern <stern@rowland.harvard.edu> wrote:
> 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.

>
> This solves a problem (wrong write-protect setting detected) reported
> by Maciej Rutecki and Peter Teoh.
>
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Tested-by: Peter Teoh <htmldeveloper@gmail.com>
>
> ---
>
> 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);

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


diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index cbf55d5..977f22b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -214,7 +214,7 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 	 * 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);
+		memset(buffer + req->data_len, 0, bufflen - req->data_len);
 
 	ret = req->errors;
  out:

Re: [PATCH] SCSI: erase invalid data returned by device

[James Bottomley]

On Wed, 2008-07-16 at 15:41 +0200, Elias Oltmanns wrote:
> Alan Stern <stern@rowland.harvard.edu> wrote:
> > 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.
> 
> >
> > This solves a problem (wrong write-protect setting detected) reported
> > by Maciej Rutecki and Peter Teoh.
> >
> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > Tested-by: Peter Teoh <htmldeveloper@gmail.com>
> >
> > ---
> >
> > 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);
> 
> 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.

James

Re: [PATCH] SCSI: erase invalid data returned by device

[Elias Oltmanns]

James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> On Wed, 2008-07-16 at 15:41 +0200, Elias Oltmanns wrote:
>> Alan Stern <stern@rowland.harvard.edu> wrote:
[...]
>> > +		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,

Elias

Re: [PATCH] SCSI: erase invalid data returned by device

[Alan Stern]

On Wed, 16 Jul 2008, Elias Oltmanns wrote:

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

> Thanks for explaining,

Alan Stern

Re: [PATCH] SCSI: erase invalid data returned by device

[Elias Oltmanns]

Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 16 Jul 2008, Elias Oltmanns wrote:
>
>> 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);
> 	}

Right, thanks for the hint.

Regards,

Elias

Re: [PATCH] SCSI: erase invalid data returned by device

[Boaz Harrosh]

Elias Oltmanns wrote:
> Alan Stern <stern@rowland.harvard.edu> wrote:
>> 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.
> 
>> This solves a problem (wrong write-protect setting detected) reported
>> by Maciej Rutecki and Peter Teoh.
>>
>> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
>> Tested-by: Peter Teoh <htmldeveloper@gmail.com>
>>
>> ---
>>
>> 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);
> 
> 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
> 
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index cbf55d5..977f22b 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -214,7 +214,7 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
>  	 * 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);
> +		memset(buffer + req->data_len, 0, bufflen - req->data_len);
>  
>  	ret = req->errors;
>   out:

This is not your fault it is built confusing.
The req->data_len is used in to ways. At first it is used as bufflen, as input
to blk_execute but at the very last stage of execution it is set to be the residual
of the transfer, from the scsi_cmnd->resid member. So at this stage you see above,
req->data_len is whats left of @bufflen that was not written/read by blk_execute.

Boaz

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