Re: [PATCH] sbp2: better check of transfer direction (protects from panic or oops)

Re: [PATCH] sbp2: better check of transfer direction (protects from panic or oops)

[Stefan Richter]

(Adding linux-scsi@vger.kernel.org again since there are more question 
marks than I thought...)

Jody McIntyre wrote:
> On Thu, Dec 08, 2005 at 10:42:02PM +0100, Stefan Richter wrote:
> 
>>sbp2: better check of transfer direction (protects from panic or oops)
>>
>>Move transfer direction check in sbp2_create_command_orb() up a bit to catch
>>more error cases.  Now we log an error note and proceed happily instead of a
>>kernel panic or oops.  Debugging and original variant of the patch by Andrew
>>de Quincey.
> 
> 
> Do you still want this if Jens' patch is applied to the SCSI subsystem?
> Extra checks are nice, but extra code is not :)  It's up to you...
> 
> Cheers,
> Jody

scsi_prep_fn() is not the only path which may set the transfer 
direction. Unless *all* paths are guaranteed to send us a proper 
direction, we still need the patch for sbp2_create_command_orb().

If all *known* paths are fixed, we could consider to override a bad 
transfer direction silently like e.g. usb_storage does. But we should 
not leave sbp2_create_command_orb() as fragile as it currently is.

I wonder if I know all paths where the direction could be set.

I also wonder if there could ever be something else execpt DMA_NONE, 
DMA_TO_DEVICE, or DMA_FROM_DEVICE being passed to sbp2. If it is 
guaranteed that scsi_cmnd.sc_data_direction is one of these three, we 
should remove ieee1394/sbp2.h::sbp2scsi_direction_table[].

> 
> 
> 
>>Problem became apparent when iPods were to be ejected:
>>http://marc.theaimsgroup.com/?l=linux1394-devel&m=113399994920181
>>http://marc.theaimsgroup.com/?l=linux1394-user&m=112152701817435
>>
>>Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
>>Cc: Ben Collins <bcollins@debian.org>
>>Cc: Andrew de Quincey <adq@lidskialf.net>
>>
>>---
>> drivers/ieee1394/sbp2.c |   24 ++++++++++++++----------
>> 1 files changed, 14 insertions(+), 10 deletions(-)
>>
>>--- linux/drivers/ieee1394.orig/sbp2.c	2005-11-24 23:10:21.000000000 +0100
>>+++ linux/drivers/ieee1394/sbp2.c	2005-12-08 22:06:49.000000000 +0100
>>@@ -1785,6 +1785,20 @@ static int sbp2_create_command_orb(struc
>> 	}
>> 
>> 	/*
>>+	 * Sanity check, in case we got a bogus direction from upper layers
>>+	 * or if sbp2scsi_direction_table is inadequate.
>>+	 */
>>+	if (orb_direction != ORB_DIRECTION_NO_DATA_TRANSFER &&
>>+	    scsi_request_bufflen == 0) {
>>+		SBP2_ERR("Transfer direction is \"%s\" but request buffer "
>>+			 "length is 0. This is a bug! Enforcing transfer "
>>+			 "direction DMA_NONE",
>>+			 orb_direction == ORB_DIRECTION_WRITE_TO_MEDIA ?
>>+			 "out" : "in");
>>+		orb_direction = ORB_DIRECTION_NO_DATA_TRANSFER;
>>+	}
>>+
>>+	/*
>> 	 * Set-up our pagetable stuff... unfortunately, this has become
>> 	 * messier than I'd like. Need to clean this up a bit.   ;-)
>> 	 */
>>@@ -1900,16 +1914,6 @@ static int sbp2_create_command_orb(struc
>> 			command_orb->misc |= ORB_SET_DATA_SIZE(scsi_request_bufflen);
>> 			command_orb->misc |= ORB_SET_DIRECTION(orb_direction);
>> 
>>-			/*
>>-			 * Sanity, in case our direction table is not
>>-			 * up-to-date
>>-			 */
>>-			if (!scsi_request_bufflen) {
>>-				command_orb->data_descriptor_hi = 0x0;
>>-				command_orb->data_descriptor_lo = 0x0;
>>-				command_orb->misc |= ORB_SET_DIRECTION(1);
>>-			}
>>-
>> 		} else {
>> 			/*
>> 			 * Need to turn this into page tables, since the
>>


-- 
Stefan Richter
-=====-=-=-= ==-- -=--=
http://arcgraph.de/sr/


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

Re: [PATCH] sbp2: better check of transfer direction (protects from panic or oops)

[Jody McIntyre]

On Fri, Dec 09, 2005 at 08:01:11PM +0100, Stefan Richter wrote:

> scsi_prep_fn() is not the only path which may set the transfer 
> direction. Unless *all* paths are guaranteed to send us a proper 
> direction, we still need the patch for sbp2_create_command_orb().

I realize that.. The question is "should we rely on the SCSI subsystem
to set a valid direction, or should we be checking it ourselves?"

> If all *known* paths are fixed, we could consider to override a bad 
> transfer direction silently like e.g. usb_storage does. But we should 
> not leave sbp2_create_command_orb() as fragile as it currently is.

I didn't know USB did this.. the fact that USB checks itself suggests
that we also need a check, and if we're checking we may as well warn.

Cheers,
Jody

> I wonder if I know all paths where the direction could be set.
> 
> I also wonder if there could ever be something else execpt DMA_NONE, 
> DMA_TO_DEVICE, or DMA_FROM_DEVICE being passed to sbp2. If it is 
> guaranteed that scsi_cmnd.sc_data_direction is one of these three, we 
> should remove ieee1394/sbp2.h::sbp2scsi_direction_table[].
> 
> >
> >
> >
> >>Problem became apparent when iPods were to be ejected:
> >>http://marc.theaimsgroup.com/?l=linux1394-devel&m=113399994920181
> >>http://marc.theaimsgroup.com/?l=linux1394-user&m=112152701817435
> >>
> >>Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
> >>Cc: Ben Collins <bcollins@debian.org>
> >>Cc: Andrew de Quincey <adq@lidskialf.net>
> >>
> >>---
> >>drivers/ieee1394/sbp2.c |   24 ++++++++++++++----------
> >>1 files changed, 14 insertions(+), 10 deletions(-)
> >>
> >>--- linux/drivers/ieee1394.orig/sbp2.c	2005-11-24 
> >>23:10:21.000000000 +0100
> >>+++ linux/drivers/ieee1394/sbp2.c	2005-12-08 22:06:49.000000000 +0100
> >>@@ -1785,6 +1785,20 @@ static int sbp2_create_command_orb(struc
> >>	}
> >>
> >>	/*
> >>+	 * Sanity check, in case we got a bogus direction from upper layers
> >>+	 * or if sbp2scsi_direction_table is inadequate.
> >>+	 */
> >>+	if (orb_direction != ORB_DIRECTION_NO_DATA_TRANSFER &&
> >>+	    scsi_request_bufflen == 0) {
> >>+		SBP2_ERR("Transfer direction is \"%s\" but request buffer "
> >>+			 "length is 0. This is a bug! Enforcing transfer "
> >>+			 "direction DMA_NONE",
> >>+			 orb_direction == ORB_DIRECTION_WRITE_TO_MEDIA ?
> >>+			 "out" : "in");
> >>+		orb_direction = ORB_DIRECTION_NO_DATA_TRANSFER;
> >>+	}
> >>+
> >>+	/*
> >>	 * Set-up our pagetable stuff... unfortunately, this has become
> >>	 * messier than I'd like. Need to clean this up a bit.   ;-)
> >>	 */
> >>@@ -1900,16 +1914,6 @@ static int sbp2_create_command_orb(struc
> >>			command_orb->misc |= 
> >>			ORB_SET_DATA_SIZE(scsi_request_bufflen);
> >>			command_orb->misc |= 
> >>			ORB_SET_DIRECTION(orb_direction);
> >>
> >>-			/*
> >>-			 * Sanity, in case our direction table is not
> >>-			 * up-to-date
> >>-			 */
> >>-			if (!scsi_request_bufflen) {
> >>-				command_orb->data_descriptor_hi = 0x0;
> >>-				command_orb->data_descriptor_lo = 0x0;
> >>-				command_orb->misc |= ORB_SET_DIRECTION(1);
> >>-			}
> >>-
> >>		} else {
> >>			/*
> >>			 * Need to turn this into page tables, since the
> >>
> 
> 
> -- 
> Stefan Richter
> -=====-=-=-= ==-- -=--=
> http://arcgraph.de/sr/

-- 


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

Re: [PATCH] sbp2: better check of transfer direction (protects from panic or oops)

[Stefan Richter]

Jody McIntyre wrote:
> On Fri, Dec 09, 2005 at 08:01:11PM +0100, Stefan Richter wrote:
>>If all *known* paths are fixed, we could consider to override a bad 
>>transfer direction silently like e.g. usb_storage does. But we should 
>>not leave sbp2_create_command_orb() as fragile as it currently is.
> 
> I didn't know USB did this.. the fact that USB checks itself suggests
> that we also need a check, and if we're checking we may as well warn.

Well, usb_storage actually does not override it explicitly. It just acts 
only on DMA_FROM_DEVICE or DMA_TO_DEVICE after having first checked for 
transfer buffer length AFAIU. However this is just one example. It 
differs between SCSI low level drivers. I don't know if any of them logs 
warnings about suspect transfer directions.
-- 
Stefan Richter
-=====-=-=-= ==-- -=--=
http://arcgraph.de/sr/

Re: [PATCH] sbp2: better check of transfer direction (protects from panic or oops)

[James Bottomley]

On Fri, 2005-12-09 at 14:34 -0500, Jody McIntyre wrote:
> I realize that.. The question is "should we rely on the SCSI subsystem
> to set a valid direction, or should we be checking it ourselves?"

You should rely on the SCSI subsystem.  If everyone has their own work
around, a) we get bloated drivers and b) the original bug is never
fixed.

James




-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

Re: [PATCH] sbp2: better check of transfer direction (protects from panic or oops)

[Stefan Richter]

I wrote on 2005-12-09:
> Jody McIntyre wrote:
...
>> Do you still want this if Jens' patch is applied to the SCSI subsystem?
>> Extra checks are nice, but extra code is not :)  It's up to you...
...
> scsi_prep_fn() is not the only path which may set the transfer 
> direction. Unless *all* paths are guaranteed to send us a proper 
> direction, we still need the patch for sbp2_create_command_orb().
> 
> If all *known* paths are fixed, we could consider to override a bad 
> transfer direction silently like e.g. usb_storage does. But we should 
> not leave sbp2_create_command_orb() as fragile as it currently is.
> 
> I wonder if I know all paths where the direction could be set.

I learned now of many more points which set the transfer direction. It 
can apparently even come from userspace.

> I also wonder if there could ever be something else execpt DMA_NONE, 
> DMA_TO_DEVICE, or DMA_FROM_DEVICE being passed to sbp2. If it is 
> guaranteed that scsi_cmnd.sc_data_direction is one of these three, we 
> should remove ieee1394/sbp2.h::sbp2scsi_direction_table[].

AFAIU it is still possibly that DMA_BIDIRECTIONAL is passed down. 
However I think now that we should delete sbp2scsi_direction_table 
anyway and simply reject DMA_BIDIRECTIONAL. This is what a few other 
SCSI low-level drivers do, and I think it is appropriate for a low-level 
driver to fail such commands instead of converting them to a "known" 
direction.

Or am I missing something?
-- 
Stefan Richter
-=====-=-=-= ==-- -=-=-
http://arcgraph.de/sr/


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

Re: [PATCH] sbp2: better check of transfer direction (protects from panic or oops)

[Christoph Hellwig]

On Sat, Dec 10, 2005 at 03:05:41PM +0100, Stefan Richter wrote:
> I learned now of many more points which set the transfer direction. It 
> can apparently even come from userspace.

It can be specified with sg.

> >I also wonder if there could ever be something else execpt DMA_NONE, 
> >DMA_TO_DEVICE, or DMA_FROM_DEVICE being passed to sbp2. If it is 
> >guaranteed that scsi_cmnd.sc_data_direction is one of these three, we 
> >should remove ieee1394/sbp2.h::sbp2scsi_direction_table[].
> 
> AFAIU it is still possibly that DMA_BIDIRECTIONAL is passed down. 
> However I think now that we should delete sbp2scsi_direction_table 
> anyway and simply reject DMA_BIDIRECTIONAL. This is what a few other 
> SCSI low-level drivers do, and I think it is appropriate for a low-level 
> driver to fail such commands instead of converting them to a "known" 
> direction.

Yes, please.


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

Re: [PATCH] sbp2: better check of transfer direction (protects from panic or oops)

[Douglas Gilbert]

Stefan Richter wrote:
> I wrote on 2005-12-09:
> 
>> Jody McIntyre wrote:
> 
> ...
> 
>>> Do you still want this if Jens' patch is applied to the SCSI subsystem?
>>> Extra checks are nice, but extra code is not :)  It's up to you...
> 
> ...
> 
>> scsi_prep_fn() is not the only path which may set the transfer
>> direction. Unless *all* paths are guaranteed to send us a proper
>> direction, we still need the patch for sbp2_create_command_orb().
>>
>> If all *known* paths are fixed, we could consider to override a bad
>> transfer direction silently like e.g. usb_storage does. But we should
>> not leave sbp2_create_command_orb() as fragile as it currently is.
>>
>> I wonder if I know all paths where the direction could be set.
> 
> 
> I learned now of many more points which set the transfer direction. It
> can apparently even come from userspace.
> 
>> I also wonder if there could ever be something else execpt DMA_NONE,
>> DMA_TO_DEVICE, or DMA_FROM_DEVICE being passed to sbp2. If it is
>> guaranteed that scsi_cmnd.sc_data_direction is one of these three, we
>> should remove ieee1394/sbp2.h::sbp2scsi_direction_table[].
> 
> 
> AFAIU it is still possibly that DMA_BIDIRECTIONAL is passed down.
> However I think now that we should delete sbp2scsi_direction_table
> anyway and simply reject DMA_BIDIRECTIONAL. This is what a few other
> SCSI low-level drivers do, and I think it is appropriate for a low-level
> driver to fail such commands instead of converting them to a "known"
> direction.
> 
> Or am I missing something?

SCSI does define various bidirectional commands, mostly
in OSD (Object Storage) and a couple in SBC (for disks,
RAID related). Linux does support not them yet.

About the time when Linux supports command lengths
greater than 16 bytes, it will also need to support
bidirectional data transfers. Perhaps bidirectional
data transfers would be implemented by two scatter
gather lists.

Doug Gilbert


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

Re: [PATCH] sbp2: better check of transfer direction (protects from panic or oops)

[Christoph Hellwig]

On Sun, Dec 11, 2005 at 07:46:52AM +1000, Douglas Gilbert wrote:
> SCSI does define various bidirectional commands, mostly
> in OSD (Object Storage) and a couple in SBC (for disks,
> RAID related). Linux does support not them yet.
> 
> About the time when Linux supports command lengths
> greater than 16 bytes, it will also need to support
> bidirectional data transfers. Perhaps bidirectional
> data transfers would be implemented by two scatter
> gather lists.

Yes.  but we can't just allow them for existing LLDDs because that would
open a _huge_ can of worms.  We'll need a flag similar to how we handle
e.g. 16 byte cdb support.


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

[PATCH] sbp2: fix panic when ejecting an ipod

[Stefan Richter]

sbp2: fix panic when ejecting an ipod

Sbp2 did not catch some bogus transfer directions in requests from upper
layers.  Problem became apparent when iPods were to be ejected:
http://marc.theaimsgroup.com/?l=linux1394-devel&m=113399994920181
http://marc.theaimsgroup.com/?l=linux1394-user&m=112152701817435
Debugging and original variant of the patch by Andrew de Quincey.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: Andrew de Quincey <adq@lidskialf.net>

---

Corresponding fix of the places where transfer direction is actually set and
how to clean sbp2_create_command_orb() up after this fix is being discussed.
A patch for SCSI mid and high level has been submitted.
http://marc.theaimsgroup.com/?t=113400010000001
Please apply the following patch to prevent kernel panics as the first step.

 drivers/ieee1394/sbp2.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

--- linux/drivers/ieee1394.orig/sbp2.c	2005-11-24 23:10:21.000000000 +0100
+++ linux/drivers/ieee1394/sbp2.c	2005-12-10 11:57:41.000000000 +0100
@@ -1784,6 +1784,12 @@ static int sbp2_create_command_orb(struc
 			break;
 	}
 
+	if (orb_direction != ORB_DIRECTION_NO_DATA_TRANSFER &&
+	    scsi_request_bufflen == 0) {
+		SBP2_WARN("Enforcing transfer direction DMA_NONE");
+		orb_direction = ORB_DIRECTION_NO_DATA_TRANSFER;
+	}
+
 	/*
 	 * Set-up our pagetable stuff... unfortunately, this has become
 	 * messier than I'd like. Need to clean this up a bit.   ;-)
@@ -1900,16 +1906,6 @@ static int sbp2_create_command_orb(struc
 			command_orb->misc |= ORB_SET_DATA_SIZE(scsi_request_bufflen);
 			command_orb->misc |= ORB_SET_DIRECTION(orb_direction);
 
-			/*
-			 * Sanity, in case our direction table is not
-			 * up-to-date
-			 */
-			if (!scsi_request_bufflen) {
-				command_orb->data_descriptor_hi = 0x0;
-				command_orb->data_descriptor_lo = 0x0;
-				command_orb->misc |= ORB_SET_DIRECTION(1);
-			}
-
 		} else {
 			/*
 			 * Need to turn this into page tables, since the

Re: [stable] [PATCH] sbp2: fix panic when ejecting an ipod

[Greg KH]

On Sat, Dec 10, 2005 at 12:24:59PM +0100, Stefan Richter wrote:
> sbp2: fix panic when ejecting an ipod
> 
> Sbp2 did not catch some bogus transfer directions in requests from upper
> layers.  Problem became apparent when iPods were to be ejected:
> http://marc.theaimsgroup.com/?l=linux1394-devel&m=113399994920181
> http://marc.theaimsgroup.com/?l=linux1394-user&m=112152701817435
> Debugging and original variant of the patch by Andrew de Quincey.
> 
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
> Cc: Andrew de Quincey <adq@lidskialf.net>

Is this in linus's tree yet?  Do the 1394 maintainers accept it as a
valid fix?

thanks,

greg k-h

Re: [stable] [PATCH] sbp2: fix panic when ejecting an ipod

[Stefan Richter]

Greg KH wrote:
> On Sat, Dec 10, 2005 at 12:24:59PM +0100, Stefan Richter wrote:
> 
>>sbp2: fix panic when ejecting an ipod
>>
>>Sbp2 did not catch some bogus transfer directions in requests from upper
>>layers.  Problem became apparent when iPods were to be ejected:
>>http://marc.theaimsgroup.com/?l=linux1394-devel&m=113399994920181
>>http://marc.theaimsgroup.com/?l=linux1394-user&m=112152701817435
>>Debugging and original variant of the patch by Andrew de Quincey.
>>
>>Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
>>Cc: Andrew de Quincey <adq@lidskialf.net>
> 
> 
> Is this in linus's tree yet? 

My idea was that it goes in parallel to Linus and to -stable, hence the 
selection of recipients of my posting.

I will submit two related cleanup patches for sbp2 to linux1394-devel on 
Sunday morning. They will remove obsolete code and reformat code for 
readability. IMO they will *not* be suitable for Linus' tree before the 
next subsystem merge window.

> Do the 1394 maintainers accept it as a valid fix?

Ben Collins and I are *sbp2* maintainers. I consider it a valid fix (but 
see below.) Jody McIntyre and Ben are *1394* maintainers. Jody posted a 
NAK a few hours ago:
|| NAK.  James has a patch to fix this in the SCSI layer, which is his
|| preference.

I agree with Jody and the SCSI people that Jens' and James' patches are 
the actual fixes. What I want to accomplish is twofold:
  - Don't let tiny mistakes lead to catastrophic failure (panic) if it
    can be avoided without additional code.
  - Get the panic fixed in -stable in one way or another ASAP.

James, I assume Jens' and your patch will be in Linus' tree soon. 
Therefore and because my pending sbp2 cleanups will land in Linus' tree 
eventually, this sbp2 patch here is not vital for the current kernel. 
But do you consider to submit the SCSI fixes or a derivative to -stable 
too? If not, I recommend my patch to be included in -stable.

Jody, I very much respect and appreciate your opinion. Please continue 
to step in the way when I'm doing goofy things. :-)
-- 
Stefan Richter
-=====-=-=-= ==-- -=-==
http://arcgraph.de/sr/

Re: [stable] [PATCH] sbp2: fix panic when ejecting an ipod

[Stefan Richter]

I wrote on 2005-12-11:
> Greg KH wrote:
>> On Sat, Dec 10, 2005 at 12:24:59PM +0100, Stefan Richter wrote:
>>> Sbp2 did not catch some bogus transfer directions in requests from upper
>>> layers.
...
>> Is this in linus's tree yet? 
...
>> Do the 1394 maintainers accept it as a valid fix?
...
> Jody posted a NAK a few hours ago:
> || NAK.  James has a patch to fix this in the SCSI layer, which is his
> || preference.

FYI, James' fix for sd_init_command etc. is this one:
http://www.kernel.org/git/?p=linux/kernel/git/jejb/scsi-rc-fixes-2.6.git;a=commit;h=c9526497cf03ee775c3a6f8ba62335735f98de7a

It depends on the following patch to apply cleanly (but does not 
actually require it to work and fix the iPod related panic):
http://www.kernel.org/git/?p=linux/kernel/git/jejb/scsi-rc-fixes-2.6.git;a=commit;h=a8c730e85e80734412f4f73ab28496a0e8b04a7b

Sorry for any confusion I might have created,
-- 
Stefan Richter
-=====-=-=-= ==-- -===-
http://arcgraph.de/sr/

Re: [stable] [PATCH] sbp2: fix panic when ejecting an ipod

[Greg KH]

On Wed, Dec 14, 2005 at 09:09:41PM +0100, Stefan Richter wrote:
> I wrote on 2005-12-11:
> >Greg KH wrote:
> >>On Sat, Dec 10, 2005 at 12:24:59PM +0100, Stefan Richter wrote:
> >>>Sbp2 did not catch some bogus transfer directions in requests from upper
> >>>layers.
> ...
> >>Is this in linus's tree yet? 
> ...
> >>Do the 1394 maintainers accept it as a valid fix?
> ...
> >Jody posted a NAK a few hours ago:
> >|| NAK.  James has a patch to fix this in the SCSI layer, which is his
> >|| preference.
> 
> FYI, James' fix for sd_init_command etc. is this one:
> http://www.kernel.org/git/?p=linux/kernel/git/jejb/scsi-rc-fixes-2.6.git;a=commit;h=c9526497cf03ee775c3a6f8ba62335735f98de7a
> 
> It depends on the following patch to apply cleanly (but does not 
> actually require it to work and fix the iPod related panic):
> http://www.kernel.org/git/?p=linux/kernel/git/jejb/scsi-rc-fixes-2.6.git;a=commit;h=a8c730e85e80734412f4f73ab28496a0e8b04a7b
> 
> Sorry for any confusion I might have created,

Ok, care to submit something to stable@ to get the fix into the next
2.6.14.y queue?

thanks,

greg k-h

[PATCH 2.6.14.3 1/2] SCSI: fix transfer direction in sd (kernel panic when ejecting iPod)

[Stefan Richter]

SCSI: fix transfer direction in sd (kernel panic when ejecting iPod)

sd_init_command could issue WRITE requests with zero buffer length.
This may lead to kernel panic or oops with some SCSI low-level drivers.
Seen with the command "eject /dev/sdX" when disconnecting an iPod:
http://marc.theaimsgroup.com/?l=linux1394-devel&m=113399994920181
http://marc.theaimsgroup.com/?l=linux1394-user&m=112152701817435

Derived from -rc patches from Jens Axboe and James Bottomley.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---

Patch is reassembled for -stable from patches:
[SCSI] fix panic when ejecting ieee1394 ipod
[SCSI] Consolidate REQ_BLOCK_PC handling path (fix ipod panic)

 drivers/scsi/scsi_lib.c  |   20 ++++++++++++++++++++
 drivers/scsi/sd.c        |   16 +---------------
 include/scsi/scsi_cmnd.h |    1 +
 3 files changed, 22 insertions(+), 15 deletions(-)

--- linux-2.6.14.3/drivers/scsi.orig/scsi_lib.c	2005-11-24 23:10:21.000000000 +0100
+++ linux-2.6.14.3/drivers/scsi/scsi_lib.c	2005-12-14 22:23:13.000000000 +0100
@@ -1129,6 +1129,26 @@ static void scsi_generic_done(struct scs
 	scsi_io_completion(cmd, cmd->result == 0 ? cmd->bufflen : 0, 0);
 }
 
+void scsi_setup_blk_pc_cmnd(struct scsi_cmnd *cmd, int retries)
+{
+	struct request *req = cmd->request;
+
+	BUG_ON(sizeof(req->cmd) > sizeof(cmd->cmnd));
+	memcpy(cmd->cmnd, req->cmd, sizeof(cmd->cmnd));
+	cmd->cmd_len = req->cmd_len;
+	if (!req->data_len)
+		cmd->sc_data_direction = DMA_NONE;
+	else if (rq_data_dir(req) == WRITE)
+		cmd->sc_data_direction = DMA_TO_DEVICE;
+	else
+		cmd->sc_data_direction = DMA_FROM_DEVICE;
+	
+	cmd->transfersize = req->data_len;
+	cmd->allowed = retries;
+	cmd->timeout_per_command = req->timeout;
+}
+EXPORT_SYMBOL_GPL(scsi_setup_blk_pc_cmnd);
+
 static int scsi_prep_fn(struct request_queue *q, struct request *req)
 {
 	struct scsi_device *sdev = q->queuedata;
--- linux-2.6.14.3/drivers/scsi.orig/sd.c	2005-11-24 23:10:21.000000000 +0100
+++ linux-2.6.14.3/drivers/scsi/sd.c	2005-12-14 22:23:13.000000000 +0100
@@ -231,24 +231,10 @@ static int sd_init_command(struct scsi_c
 	 * SG_IO from block layer already setup, just copy cdb basically
 	 */
 	if (blk_pc_request(rq)) {
-		if (sizeof(rq->cmd) > sizeof(SCpnt->cmnd))
-			return 0;
-
-		memcpy(SCpnt->cmnd, rq->cmd, sizeof(SCpnt->cmnd));
-		SCpnt->cmd_len = rq->cmd_len;
-		if (rq_data_dir(rq) == WRITE)
-			SCpnt->sc_data_direction = DMA_TO_DEVICE;
-		else if (rq->data_len)
-			SCpnt->sc_data_direction = DMA_FROM_DEVICE;
-		else
-			SCpnt->sc_data_direction = DMA_NONE;
-
-		this_count = rq->data_len;
+		scsi_setup_blk_pc_cmnd(SCpnt, SD_PASSTHROUGH_RETRIES);
 		if (rq->timeout)
 			timeout = rq->timeout;
 
-		SCpnt->transfersize = rq->data_len;
-		SCpnt->allowed = SD_PASSTHROUGH_RETRIES;
 		goto queue;
 	}
 
--- linux-2.6.14.3/include/scsi.orig/scsi_cmnd.h	2005-11-24 23:10:21.000000000 +0100
+++ linux-2.6.14.3/include/scsi/scsi_cmnd.h	2005-12-14 22:23:14.000000000 +0100
@@ -150,5 +150,6 @@ extern struct scsi_cmnd *scsi_get_comman
 extern void scsi_put_command(struct scsi_cmnd *);
 extern void scsi_io_completion(struct scsi_cmnd *, unsigned int, unsigned int);
 extern void scsi_finish_command(struct scsi_cmnd *cmd);
+extern void scsi_setup_blk_pc_cmnd(struct scsi_cmnd *cmd, int retries);
 
 #endif /* _SCSI_SCSI_CMND_H */

[PATCH 2.6.14.3] SCSI: fix transfer direction in scsi_lib and st

[Stefan Richter]

SCSI: fix transfer direction in scsi_lib and st

scsi_prep_fn and st_init_command could issue WRITE requests with zero
buffer length. This may lead to kernel panic or oops with some SCSI
low-level drivers.

Derived from -rc patches from Jens Axboe and James Bottomley.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---

Patch is reassembled for -stable from patches:
[SCSI] fix panic when ejecting ieee1394 ipod
[SCSI] Consolidate REQ_BLOCK_PC handling path (fix ipod panic)

Depends on patch "SCSI: fix transfer direction in sd (kernel panic when
ejecting iPod)". Also modifies the already correct sr_init_command to
fully match the corresponding -rc patch.

 drivers/scsi/scsi_lib.c |   13 +------------
 drivers/scsi/sr.c       |   20 +++-----------------
 drivers/scsi/st.c       |   19 +------------------
 3 files changed, 5 insertions(+), 47 deletions(-)

--- linux-2.6.14.3/drivers/scsi.orig/scsi_lib.c	2005-11-24 23:10:21.000000000 +0100
+++ linux-2.6.14.3/drivers/scsi/scsi_lib.c	2005-12-14 22:23:13.000000000 +0100
@@ -1284,18 +1304,7 @@ static int scsi_prep_fn(struct request_q
 				goto kill;
 			}
 		} else {
-			memcpy(cmd->cmnd, req->cmd, sizeof(cmd->cmnd));
-			cmd->cmd_len = req->cmd_len;
-			if (rq_data_dir(req) == WRITE)
-				cmd->sc_data_direction = DMA_TO_DEVICE;
-			else if (req->data_len)
-				cmd->sc_data_direction = DMA_FROM_DEVICE;
-			else
-				cmd->sc_data_direction = DMA_NONE;
-			
-			cmd->transfersize = req->data_len;
-			cmd->allowed = 3;
-			cmd->timeout_per_command = req->timeout;
+			scsi_setup_blk_pc_cmnd(cmd, 3);
 			cmd->done = scsi_generic_done;
 		}
 	}
--- linux-2.6.14.3/drivers/scsi.orig/sr.c	2005-11-24 23:10:21.000000000 +0100
+++ linux-2.6.14.3/drivers/scsi/sr.c	2005-12-14 22:23:13.000000000 +0100
@@ -320,25 +320,11 @@ static int sr_init_command(struct scsi_c
 	 * these are already setup, just copy cdb basically
 	 */
 	if (SCpnt->request->flags & REQ_BLOCK_PC) {
-		struct request *rq = SCpnt->request;
+		scsi_setup_blk_pc_cmnd(SCpnt, MAX_RETRIES);
 
-		if (sizeof(rq->cmd) > sizeof(SCpnt->cmnd))
-			return 0;
-
-		memcpy(SCpnt->cmnd, rq->cmd, sizeof(SCpnt->cmnd));
-		SCpnt->cmd_len = rq->cmd_len;
-		if (!rq->data_len)
-			SCpnt->sc_data_direction = DMA_NONE;
-		else if (rq_data_dir(rq) == WRITE)
-			SCpnt->sc_data_direction = DMA_TO_DEVICE;
-		else
-			SCpnt->sc_data_direction = DMA_FROM_DEVICE;
-
-		this_count = rq->data_len;
-		if (rq->timeout)
-			timeout = rq->timeout;
+		if (SCpnt->timeout_per_command)
+			timeout = SCpnt->timeout_per_command;
 
-		SCpnt->transfersize = rq->data_len;
 		goto queue;
 	}
 
--- linux-2.6.14.3/drivers/scsi.orig/st.c	2005-11-24 23:10:21.000000000 +0100
+++ linux-2.6.14.3/drivers/scsi/st.c	2005-12-14 22:23:13.000000000 +0100
@@ -4196,27 +4196,10 @@ static void st_intr(struct scsi_cmnd *SC
  */
 static int st_init_command(struct scsi_cmnd *SCpnt)
 {
-	struct request *rq;
-
 	if (!(SCpnt->request->flags & REQ_BLOCK_PC))
 		return 0;
 
-	rq = SCpnt->request;
-	if (sizeof(rq->cmd) > sizeof(SCpnt->cmnd))
-		return 0;
-
-	memcpy(SCpnt->cmnd, rq->cmd, sizeof(SCpnt->cmnd));
-	SCpnt->cmd_len = rq->cmd_len;
-
-	if (rq_data_dir(rq) == WRITE)
-		SCpnt->sc_data_direction = DMA_TO_DEVICE;
-	else if (rq->data_len)
-		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
-	else
-		SCpnt->sc_data_direction = DMA_NONE;
-
-	SCpnt->timeout_per_command = rq->timeout;
-	SCpnt->transfersize = rq->data_len;
+	scsi_setup_blk_pc_cmnd(SCpnt, 0);
 	SCpnt->done = st_intr;
 	return 1;
 }

Re: [PATCH 2.6.14.3] SCSI: fix transfer direction in scsi_lib and st

[Stefan Richter]

PS: This was of course "[PATCH 2.6.14.3 2/2] ...".
-- 
Stefan Richter
-=====-=-=-= ==-- -===-
http://arcgraph.de/sr/

Re: [stable] Re: [PATCH 2.6.14.3] SCSI: fix transfer direction in scsi_lib and st

[Greg KH]

On Wed, Dec 14, 2005 at 11:43:35PM +0100, Stefan Richter wrote:
> PS: This was of course "[PATCH 2.6.14.3 2/2] ...".

Both of them have been applied to the queue, thanks.

greg k-h