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 <[email protected]>.
http://marc.theaimsgroup.com/?l=linux-scsi&m=114055884611429
Signed-off-by: Stefan Richter <[email protected]>
---
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;
* Stefan Richter ([email protected]) 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
Chris Wright wrote:
> * Stefan Richter ([email protected]) 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/
On Sun, Feb 26, 2006 at 12:07:55AM +0100, Stefan Richter wrote:
> Chris Wright wrote:
> >* Stefan Richter ([email protected]) 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?
On Sun, 26 Feb 2006, Stefan Richter wrote:
> Chris Wright wrote:
> > * Stefan Richter ([email protected]) 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
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 ([email protected]) 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 <[email protected]>
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...
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
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
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.
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/
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...
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/
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
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?
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/
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
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.
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 <[email protected]>
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 <[email protected]>
Also removed the buffer size magic number (512) and added DPOFUA of
zero to the defaults
Signed-off-by: James Bottomley <[email protected]>
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");
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.
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/
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 <[email protected]>
Also removed the buffer size magic number (512) and added DPOFUA of
zero to the defaults
Signed-off-by: James Bottomley <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>
rediff for 2.6.15.x without DPOFUA bit, taken from commit
489708007785389941a89fa06aedc5ec53303c96
Signed-off-by: Stefan Richter <[email protected]>
---
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");
* Stefan Richter ([email protected]) wrote:
> sd: fix memory corruption with broken mode page headers
Thanks, queued this one for -stable.
-chris