[PATCH 1/2] sd: fix memory corruption by sd_read_cache_type

[PATCH 1/2] sd: fix memory corruption by sd_read_cache_type

[Stefan Richter]

sd: fix memory corruption by sd_read_cache_type

Let sd check more thoroughly before using mode_sense responses for data
length calculation. Fixes memory corruption ("slab error in
cache_free_debugcheck") or kernel panic when buggy disks are connected,
notably Initio SBP-2 bridges.
http://bugzilla.kernel.org/show_bug.cgi?id=6114
http://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=182005

Taken from a patch by Al Viro <viro@ftp.linux.org.uk>.
http://marc.theaimsgroup.com/?l=linux-scsi&m=114055884611429

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
Patch is applicable to 2.6.14, 2.6.15, 2.6.16.

Index: linux-2.6.16-rc4/drivers/scsi/sd.c
===================================================================
--- linux-2.6.16-rc4.orig/drivers/scsi/sd.c	2006-02-22 22:27:42.000000000 +0100
+++ linux-2.6.16-rc4/drivers/scsi/sd.c	2006-02-22 22:29:10.000000000 +0100
@@ -1342,6 +1342,8 @@ sd_read_cache_type(struct scsi_disk *sdk
 
 	/* Take headers and block descriptors into account */
 	len += data.header_length + data.block_descriptor_length;
+	if (len > 512)
+		goto bad_sense;
 
 	/* Get the data */
 	res = sd_do_mode_sense(sdp, dbd, modepage, buffer, len, &data, &sshdr);
@@ -1354,6 +1356,12 @@ sd_read_cache_type(struct scsi_disk *sdk
 		int ct = 0;
 		int offset = data.header_length + data.block_descriptor_length;
 
+		if (offset >= 512 - 2) {
+			printk(KERN_ERR "%s: malformed MODE SENSE response",
+				diskname);
+			goto defaults;
+		}
+
 		if ((buffer[offset] & 0x3f) != modepage) {
 			printk(KERN_ERR "%s: got wrong page\n", diskname);
 			goto defaults;

Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type

[Chris Wright]

* Stefan Richter (stefanr@s5r6.in-berlin.de) wrote:
> sd: fix memory corruption by sd_read_cache_type

Looks like these still aren't upstream.  Can you please resend to -stable
once they've been picked up by Linus?

thanks,
-chris

Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type

[Stefan Richter]

Chris Wright wrote:
> * Stefan Richter (stefanr@s5r6.in-berlin.de) wrote:
>>sd: fix memory corruption by sd_read_cache_type
> 
> Looks like these still aren't upstream.  Can you please resend to -stable
> once they've been picked up by Linus?

Yes, I will do so.
-- 
Stefan Richter
-=====-=-==- --=- ==-=-
http://arcgraph.de/sr/

Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type

[Al Viro]

On Sun, Feb 26, 2006 at 12:07:55AM +0100, Stefan Richter wrote:
> Chris Wright wrote:
> >* Stefan Richter (stefanr@s5r6.in-berlin.de) wrote:
> >>sd: fix memory corruption by sd_read_cache_type
> >
> >Looks like these still aren't upstream.  Can you please resend to -stable
> >once they've been picked up by Linus?
> 
> Yes, I will do so.

Speaking of sbp2 problems...  Why the _hell_ are we blacklisting on
firmware revision alone?  Especially with entries like "all firmware
with 2.<whatever> as version is broken"...

Case in point: Initio bridge, firmware revision 2.21.  Couldn't care
less about long INQUIRY, doesn't need skip_ms_page_8, *DOES* need
correctly detected cache type.

What kind of chipsets do affected devices really have?

Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type

[Linus Torvalds]

On Sun, 26 Feb 2006, Stefan Richter wrote:

> Chris Wright wrote:
> > * Stefan Richter (stefanr@s5r6.in-berlin.de) wrote:
> > > sd: fix memory corruption by sd_read_cache_type
> > 
> > Looks like these still aren't upstream.  Can you please resend to -stable
> > once they've been picked up by Linus?
> 
> Yes, I will do so.

Perhaps equally importantly, let's get them into mainline if they are so 
important. Which means that I want sign-offs and acks from the appropriate 
people (scsi and original author, which is apparently Al).

		Linus

Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type

[Al Viro]

On Sat, Feb 25, 2006 at 04:01:35PM -0800, Linus Torvalds wrote:
> 
> 
> On Sun, 26 Feb 2006, Stefan Richter wrote:
> 
> > Chris Wright wrote:
> > > * Stefan Richter (stefanr@s5r6.in-berlin.de) wrote:
> > > > sd: fix memory corruption by sd_read_cache_type
> > > 
> > > Looks like these still aren't upstream.  Can you please resend to -stable
> > > once they've been picked up by Linus?
> > 
> > Yes, I will do so.
> 
> Perhaps equally importantly, let's get them into mainline if they are so 
> important. Which means that I want sign-offs and acks from the appropriate 
> people (scsi and original author, which is apparently Al).

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

FWIW, there's a potential problem: original was not split in two, and AFAICS
it was picked by SCSI folks in that form.  I've no objections against
such split, just wondering if git might...

ObGit: is there any way to fetch _all_ branches from remote, creating local
branches with the same names if they didn't exist yet?  Ot, at least, get
the full list of branches existing in the remote repository...

Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type

[Linus Torvalds]

On Sun, 26 Feb 2006, Al Viro wrote:
> 
> ObGit: is there any way to fetch _all_ branches from remote, creating local
> branches with the same names if they didn't exist yet?  Ot, at least, get
> the full list of branches existing in the remote repository...

The magic is "git-ls-remote". In particular, the "--heads" flag limits it 
to just showing branch heads.

Then you can feed that into "git fetch", which takes the format 
"localname:remotename" to tell it how to fetch.

In other words, something like

	git fetch remote $(git ls-remote --heads remote | awk '{print $2":"$2}')

should do what you asked for.

		Linus

Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type

[James Bottomley]

On Sat, 2006-02-25 at 16:01 -0800, Linus Torvalds wrote:
> Perhaps equally importantly, let's get them into mainline if they are so 
> important. Which means that I want sign-offs and acks from the appropriate 
> people (scsi and original author, which is apparently Al).

Yes, I've been thinking about this.  The problem is that it's a change
to sd and a change to scsi_lib in a fairly critical routine.  While I'm
reasonably certain the change is safe, I'd prefer to make sure by
incubating in -mm for a while.

The title, by the way, is misleading; it's not a memory corruption in sd
at all really.  It's the initio bridge which produces a totally
standards non conformant return to a mode sense which produces the
problem.  And so, it's only the single initio bridge which is currently
affected; hence the caution.

James

Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type

[Al Viro]

On Sat, Feb 25, 2006 at 11:14:48PM -0600, James Bottomley wrote:
> On Sat, 2006-02-25 at 16:01 -0800, Linus Torvalds wrote:
> > Perhaps equally importantly, let's get them into mainline if they are so 
> > important. Which means that I want sign-offs and acks from the appropriate 
> > people (scsi and original author, which is apparently Al).
> 
> Yes, I've been thinking about this.  The problem is that it's a change
> to sd and a change to scsi_lib in a fairly critical routine.  While I'm
> reasonably certain the change is safe, I'd prefer to make sure by
> incubating in -mm for a while.
> 
> The title, by the way, is misleading; it's not a memory corruption in sd
> at all really.  It's the initio bridge which produces a totally
> standards non conformant return to a mode sense which produces the
> problem.  And so, it's only the single initio bridge which is currently
> affected; hence the caution.

No.  It's sd.c assuming that mode page header is sane, without any
checks.  And yes, it does give memory corruption if it's not.

Initio-related part is in scsi_lib.c (and in recovery part of sd.c
changes).  That one is about how we can handle gracefully a broken
device that gives no header at all.

Checks for ->block_descriptors_length are just making sure we won't try
to do any access past the end of buffer, no matter what crap we got from
device.

Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type

[Stefan Richter]

Al Viro wrote:
> Speaking of sbp2 problems...  Why the _hell_ are we blacklisting on
> firmware revision alone?  Especially with entries like "all firmware
> with 2.<whatever> as version is broken"...

The firmware_revision CSR key value has so far been a good method to 
guesstimate the bridge chip. I don't know a better one.

> Case in point: Initio bridge, firmware revision 2.21.  Couldn't care
> less about long INQUIRY, doesn't need skip_ms_page_8, *DOES* need
> correctly detected cache type.
...

I agree.

I posted an improved blacklisting patch a few days ago. Among other 
small cleanups, I removed skip_ms_page_8 from the Initio blacklist entry.
http://marc.theaimsgroup.com/?l=linux1394-devel&m=114065678722190
-- 
Stefan Richter
-=====-=-==- --=- ==-=-
http://arcgraph.de/sr/

Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type

[Al Viro]

On Sun, Feb 26, 2006 at 09:11:08AM +0100, Stefan Richter wrote:
> Al Viro wrote:
> >Speaking of sbp2 problems...  Why the _hell_ are we blacklisting on
> >firmware revision alone?  Especially with entries like "all firmware
> >with 2.<whatever> as version is broken"...
> 
> The firmware_revision CSR key value has so far been a good method to 
> guesstimate the bridge chip. I don't know a better one.

Umm...  What about ->vendor_name_kv (plus firmware_revision, obviously)?
 
> I posted an improved blacklisting patch a few days ago. Among other 
> small cleanups, I removed skip_ms_page_8 from the Initio blacklist entry.
> http://marc.theaimsgroup.com/?l=linux1394-devel&m=114065678722190

FWIW, that puppy appears to live just fine without forcing 36byte
inquiry here...

Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type

[Stefan Richter]

Al Viro wrote:
> On Sat, Feb 25, 2006 at 11:14:48PM -0600, James Bottomley wrote:
...
>>The problem is that it's a change
>>to sd and a change to scsi_lib in a fairly critical routine.  While I'm
>>reasonably certain the change is safe, I'd prefer to make sure by
>>incubating in -mm for a while.
>>
>>The title, by the way, is misleading; it's not a memory corruption in sd
>>at all really.  It's the initio bridge which produces a totally
>>standards non conformant return to a mode sense which produces the
>>problem.  And so, it's only the single initio bridge which is currently
>>affected; hence the caution.
> 
> No.  It's sd.c assuming that mode page header is sane, without any
> checks.  And yes, it does give memory corruption if it's not.
> 
> Initio-related part is in scsi_lib.c (and in recovery part of sd.c
> changes).  That one is about how we can handle gracefully a broken
> device that gives no header at all.
> 
> Checks for ->block_descriptors_length are just making sure we won't try
> to do any access past the end of buffer, no matter what crap we got from
> device.

That's why I split Al's patch. Part 1/2 prevents sd from using values 
from buggy firmwares to overwrite memory where sd has no business to 
write at. As Al explained, the culprit is sd which feeds a too big len 
to scsi_mode_sense()'s memset(buffer, 0, len). Patch 2/2 adds an Initio 
specific workaround. (The 2nd part is not necessarily material for 
-stable, although it assures correct cache handling for Initio based 
SBP-2 HDDs.)

Please merge the sd fix ASAP (part 1/2). Users _are_ seeing memory 
corruption or panic in interrupt context without this patch. Fully 
reproducable, probably with all Initio SBP-2 bridges which exist; and 
these are actually popular chips for 1394a S400 as well as 1394b S800 
products, both noname and branded products.

The 2nd part could perhaps go through -mm. If you wish I resend it to 
Andrew. Al, what do you think? I believe this patch to be safe for 
non-broken devices though.

BTW I missed a small whitespace mishap in pt 2/2 which is why I should 
repost it anyway:
-+		} else if(use_10_for_ms) {
++		} else if (use_10_for_ms) {
-- 
Stefan Richter
-=====-=-==- --=- ==-=-
http://arcgraph.de/sr/

Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Sun, 26 Feb 2006, Al Viro wrote:
> 
>>ObGit: is there any way to fetch _all_ branches from remote, creating local
>>branches with the same names if they didn't exist yet?  Ot, at least, get
>>the full list of branches existing in the remote repository...
> 
> 
> The magic is "git-ls-remote". In particular, the "--heads" flag limits it 
> to just showing branch heads.
> 
> Then you can feed that into "git fetch", which takes the format 
> "localname:remotename" to tell it how to fetch.
> 
> In other words, something like
> 
> 	git fetch remote $(git ls-remote --heads remote | awk '{print $2":"$2}')
> 
> should do what you asked for.

Any chance we could get 'git fetch --heads' ?

FWIW, I regularly blow away and create new heads, so the above is rather 
long for people who use my repos.  A lot of them use rsync because when 
you're tracking a repo with ever-changing branches, 'git pull' doesn't 
really approximate "make local X look like remote X".

	Jeff

Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type

[Al Viro]

On Sun, Feb 26, 2006 at 03:39:50AM -0500, Jeff Garzik wrote:
> Any chance we could get 'git fetch --heads' ?
> 
> FWIW, I regularly blow away and create new heads, so the above is rather 
> long for people who use my repos.  A lot of them use rsync because when 
> you're tracking a repo with ever-changing branches, 'git pull' doesn't 
> really approximate "make local X look like remote X".

Speaking of which...  Shouldn't git clone bring in .git/HEAD for rsync:// URLs?
As it is, we end up with HEAD pointing to refs/master, which might simply
not be there.  For git:// we get .git/HEAD same as in remote repository,
so behaviour for rsync:// probably should be the same...

Looks like a missing rsync in git-clone, around
        rsync://*)
                rsync $quiet -av --ignore-existing  \
                        --exclude info "$repo/objects/" "$GIT_DIR/objects/" &&
                rsync $quiet -av --ignore-existing  \
                        --exclude info "$repo/refs/" "$GIT_DIR/refs/" || exit

Comments?

Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type

[Stefan Richter]

Al Viro wrote:
> On Sun, Feb 26, 2006 at 09:11:08AM +0100, Stefan Richter wrote:
>>Al Viro wrote:
>>>Speaking of sbp2 problems...  Why the _hell_ are we blacklisting on
>>>firmware revision alone?  Especially with entries like "all firmware
>>>with 2.<whatever> as version is broken"...
>>
>>The firmware_revision CSR key value has so far been a good method to 
>>guesstimate the bridge chip. I don't know a better one.
> 
> Umm...  What about ->vendor_name_kv (plus firmware_revision, obviously)?

Not a single one of the devices in my collection features vendor_name_kv 
in the ROM's unit directory. The vendor_name_kv in the ROM's root 
directory more often reflects the vendor of the enclosure or bridge 
board than the vendor of the bridge chip. (Most vendors of enclosures or 
boards seem to put only their name into a firmware although they have 
the opportunity for market differentiation by an own firmware full of 
their very own bugs...)

>>I posted an improved blacklisting patch a few days ago. Among other 
>>small cleanups, I removed skip_ms_page_8 from the Initio blacklist entry.
>>http://marc.theaimsgroup.com/?l=linux1394-devel&m=114065678722190
> 
> FWIW, that puppy appears to live just fine without forcing 36byte
> inquiry here...

A few older Initio based enclosures needed it. Newer don't, including 
the one I have here. AFAIK the 36byte inquiry workaround does not break 
anything if forced onto non-broken devices.
-- 
Stefan Richter
-=====-=-==- --=- ==-=-
http://arcgraph.de/sr/

Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type

[Jeff Garzik]

Al Viro wrote:
> On Sun, Feb 26, 2006 at 03:39:50AM -0500, Jeff Garzik wrote:
> 
>>Any chance we could get 'git fetch --heads' ?
>>
>>FWIW, I regularly blow away and create new heads, so the above is rather 
>>long for people who use my repos.  A lot of them use rsync because when 
>>you're tracking a repo with ever-changing branches, 'git pull' doesn't 
>>really approximate "make local X look like remote X".
> 
> 
> Speaking of which...  Shouldn't git clone bring in .git/HEAD for rsync:// URLs?
> As it is, we end up with HEAD pointing to refs/master, which might simply
> not be there.  For git:// we get .git/HEAD same as in remote repository,
> so behaviour for rsync:// probably should be the same...
> 
> Looks like a missing rsync in git-clone, around
>         rsync://*)
>                 rsync $quiet -av --ignore-existing  \
>                         --exclude info "$repo/objects/" "$GIT_DIR/objects/" &&
>                 rsync $quiet -av --ignore-existing  \
>                         --exclude info "$repo/refs/" "$GIT_DIR/refs/" || exit
> 
> Comments?

AFAICS 'git clone $rsync_url' pulls down the heads and tags just fine... 
  I just tried it again to be certain.  refs/heads and refs/tags is 
fully populated, and HEAD links to refs/master as it should.

	Jeff

Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type

[Al Viro]

On Sun, Feb 26, 2006 at 05:45:02AM -0500, Jeff Garzik wrote:
> AFAICS 'git clone $rsync_url' pulls down the heads and tags just fine... 
>  I just tried it again to be certain.  refs/heads and refs/tags is 
> fully populated, and HEAD links to refs/master as it should.

Why should it?  Note that this is _not_ what happens with e.g. git://
URLs.  And while we are at it, who said that refs/master even exists?
AFAICS, it's a convention and no more - it doesn't have to be there.

Even if it does exist, I'd say that behaviour of git:// makes more sense -
it sets HEAD to the same thing we have in remote; if it's refs/master,
so be it, if it's set to a different branch, we start on the same branch.

Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type

[James Bottomley]

On Sun, 2006-02-26 at 05:31 +0000, Al Viro wrote:
> No.  It's sd.c assuming that mode page header is sane, without any
> checks.  And yes, it does give memory corruption if it's not.

Well, OK, I agree allowing us to request data longer than the actual
buffer is a problem.  However, I don't exactly see how this actually
causes corruption, since even the initio bridge only sends 12 bytes of
data, so we should stop with a data underrun at that point (however big
the buffer is)

> Initio-related part is in scsi_lib.c (and in recovery part of sd.c
> changes).  That one is about how we can handle gracefully a broken
> device that gives no header at all.
> 
> Checks for ->block_descriptors_length are just making sure we won't try
> to do any access past the end of buffer, no matter what crap we got from
> device.

I agree; how about this for the actual patch (for 2.6.16) ... I put two
more tidy ups into it ...

James

---

[SCSI] sd: Add sanity checking to mode sense return data

From: Al Viro <viro@ftp.linux.org.uk>

There's a problem in sd where we blindly believe the length of the
headers and block descriptors.  Some devices return insane values for
these and cause our length to end up greater than the actual buffer
size, so check to make sure.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Also removed the buffer size magic number (512) and added DPOFUA of
zero to the defaults

Signed-off-by: James Bottomley <James.Bottomley@SteelEye.com>
Index: linux-2.6/drivers/scsi/sd.c
===================================================================
--- linux-2.6.orig/drivers/scsi/sd.c
+++ linux-2.6/drivers/scsi/sd.c
@@ -89,6 +89,11 @@
 #define SD_MAX_RETRIES		5
 #define SD_PASSTHROUGH_RETRIES	1
 
+/*
+ * Size of the initial data buffer for mode and read capacity data
+ */
+#define SD_BUF_SIZE		512
+
 static void scsi_disk_release(struct kref *kref);
 
 struct scsi_disk {
@@ -1239,7 +1244,7 @@ sd_do_mode_sense(struct scsi_device *sdp
 
 /*
  * read write protect setting, if possible - called only in sd_revalidate_disk()
- * called with buffer of length 512
+ * called with buffer of length SD_BUF_SIZE
  */
 static void
 sd_read_write_protect_flag(struct scsi_disk *sdkp, char *diskname,
@@ -1297,7 +1302,7 @@ sd_read_write_protect_flag(struct scsi_d
 
 /*
  * sd_read_cache_type - called only from sd_revalidate_disk()
- * called with buffer of length 512
+ * called with buffer of length SD_BUF_SIZE
  */
 static void
 sd_read_cache_type(struct scsi_disk *sdkp, char *diskname,
@@ -1342,6 +1347,8 @@ sd_read_cache_type(struct scsi_disk *sdk
 
 	/* Take headers and block descriptors into account */
 	len += data.header_length + data.block_descriptor_length;
+	if (len > SD_BUF_SIZE)
+		goto bad_sense;
 
 	/* Get the data */
 	res = sd_do_mode_sense(sdp, dbd, modepage, buffer, len, &data, &sshdr);
@@ -1354,6 +1361,12 @@ sd_read_cache_type(struct scsi_disk *sdk
 		int ct = 0;
 		int offset = data.header_length + data.block_descriptor_length;
 
+		if (offset >= SD_BUF_SIZE - 2) {
+			printk(KERN_ERR "%s: malformed MODE SENSE response",
+				diskname);
+			goto defaults;
+		}
+
 		if ((buffer[offset] & 0x3f) != modepage) {
 			printk(KERN_ERR "%s: got wrong page\n", diskname);
 			goto defaults;
@@ -1398,6 +1411,7 @@ defaults:
 	       diskname);
 	sdkp->WCE = 0;
 	sdkp->RCD = 0;
+	sdkp->DPOFUA = 0;
 }
 
 /**
@@ -1421,7 +1435,7 @@ static int sd_revalidate_disk(struct gen
 	if (!scsi_device_online(sdp))
 		goto out;
 
-	buffer = kmalloc(512, GFP_KERNEL | __GFP_DMA);
+	buffer = kmalloc(SD_BUF_SIZE, GFP_KERNEL | __GFP_DMA);
 	if (!buffer) {
 		printk(KERN_WARNING "(sd_revalidate_disk:) Memory allocation "
 		       "failure.\n");

Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type

[Al Viro]

On Sun, Feb 26, 2006 at 08:34:10AM -0600, James Bottomley wrote:
> Well, OK, I agree allowing us to request data longer than the actual
> buffer is a problem.  However, I don't exactly see how this actually
> causes corruption, since even the initio bridge only sends 12 bytes of
> data, so we should stop with a data underrun at that point (however big
> the buffer is)

scsi_mode_sense() does memset(buffer, 0, len).  You don't need corrupting
data to come from device - 10Kb of zeroes into 512-byte kmalloc'ed buffer
will do the job just fine...
 
ACKed in that form.

Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type

[Stefan Richter]

James Bottomley wrote:
...
> how about this for the actual patch (for 2.6.16) ... I put two
> more tidy ups into it ...
...
> +#define SD_BUF_SIZE		512

Thanks.
Believe it or not, I was actually about to follow up with something like 
this. :-)
-- 
Stefan Richter
-=====-=-==- --=- ==-=-
http://arcgraph.de/sr/

[PATCH 2.6.15.4 update] sd: fix memory corruption with broken mode page headers

[Stefan Richter]

sd: fix memory corruption with broken mode page headers

There's a problem in sd where we blindly believe the length of the
headers and block descriptors.  Some devices return insane values for
these and cause our length to end up greater than the actual buffer
size, so check to make sure.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Also removed the buffer size magic number (512) and added DPOFUA of
zero to the defaults

Signed-off-by: James Bottomley <James.Bottomley@SteelEye.com>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>

rediff for 2.6.15.x without DPOFUA bit, taken from commit
489708007785389941a89fa06aedc5ec53303c96

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
fixes http://bugzilla.kernel.org/show_bug.cgi?id=6114 and
http://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=182005

Index: linux-2.6.15.4/drivers/scsi/sd.c
===================================================================
--- linux-2.6.15.4.orig/drivers/scsi/sd.c	2006-02-26 23:58:40.000000000 +0100
+++ linux-2.6.15.4/drivers/scsi/sd.c	2006-02-27 00:03:07.000000000 +0100
@@ -88,6 +88,11 @@
 #define SD_MAX_RETRIES		5
 #define SD_PASSTHROUGH_RETRIES	1
 
+/*
+ * Size of the initial data buffer for mode and read capacity data
+ */
+#define SD_BUF_SIZE		512
+
 static void scsi_disk_release(struct kref *kref);
 
 struct scsi_disk {
@@ -1299,7 +1304,7 @@ sd_do_mode_sense(struct scsi_device *sdp
 
 /*
  * read write protect setting, if possible - called only in sd_revalidate_disk()
- * called with buffer of length 512
+ * called with buffer of length SD_BUF_SIZE
  */
 static void
 sd_read_write_protect_flag(struct scsi_disk *sdkp, char *diskname,
@@ -1357,7 +1362,7 @@ sd_read_write_protect_flag(struct scsi_d
 
 /*
  * sd_read_cache_type - called only from sd_revalidate_disk()
- * called with buffer of length 512
+ * called with buffer of length SD_BUF_SIZE
  */
 static void
 sd_read_cache_type(struct scsi_disk *sdkp, char *diskname,
@@ -1402,6 +1407,8 @@ sd_read_cache_type(struct scsi_disk *sdk
 
 	/* Take headers and block descriptors into account */
 	len += data.header_length + data.block_descriptor_length;
+	if (len > SD_BUF_SIZE)
+		goto bad_sense;
 
 	/* Get the data */
 	res = sd_do_mode_sense(sdp, dbd, modepage, buffer, len, &data, &sshdr);
@@ -1414,6 +1421,12 @@ sd_read_cache_type(struct scsi_disk *sdk
 		int ct = 0;
 		int offset = data.header_length + data.block_descriptor_length;
 
+		if (offset >= SD_BUF_SIZE - 2) {
+			printk(KERN_ERR "%s: malformed MODE SENSE response",
+				diskname);
+			goto defaults;
+		}
+
 		if ((buffer[offset] & 0x3f) != modepage) {
 			printk(KERN_ERR "%s: got wrong page\n", diskname);
 			goto defaults;
@@ -1472,7 +1485,7 @@ static int sd_revalidate_disk(struct gen
 	if (!scsi_device_online(sdp))
 		goto out;
 
-	buffer = kmalloc(512, GFP_KERNEL | __GFP_DMA);
+	buffer = kmalloc(SD_BUF_SIZE, GFP_KERNEL | __GFP_DMA);
 	if (!buffer) {
 		printk(KERN_WARNING "(sd_revalidate_disk:) Memory allocation "
 		       "failure.\n");

Re: [stable] [PATCH 2.6.15.4 update] sd: fix memory corruption with broken mode page headers

[Chris Wright]

* Stefan Richter (stefanr@s5r6.in-berlin.de) wrote:
> sd: fix memory corruption with broken mode page headers

Thanks, queued this one for -stable.
-chris