* [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type @ 2006-02-23 1:02 Stefan Richter 2006-02-25 2:10 ` [stable] " Chris Wright 0 siblings, 1 reply; 22+ messages in thread From: Stefan Richter @ 2006-02-23 1:02 UTC (permalink / raw) To: stable, Linus Torvalds; +Cc: linux-kernel, linux-scsi, Al Viro, Jody McIntyre 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; ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type 2006-02-23 1:02 [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type Stefan Richter @ 2006-02-25 2:10 ` Chris Wright 2006-02-25 23:07 ` Stefan Richter 0 siblings, 1 reply; 22+ messages in thread From: Chris Wright @ 2006-02-25 2:10 UTC (permalink / raw) To: Stefan Richter Cc: stable, Linus Torvalds, Jody McIntyre, linux-kernel, linux-scsi, Al Viro * 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type 2006-02-25 2:10 ` [stable] " Chris Wright @ 2006-02-25 23:07 ` Stefan Richter 2006-02-25 23:22 ` Al Viro ` (2 more replies) 0 siblings, 3 replies; 22+ messages in thread From: Stefan Richter @ 2006-02-25 23:07 UTC (permalink / raw) To: Chris Wright Cc: stable, Linus Torvalds, Jody McIntyre, linux-kernel, linux-scsi, Al Viro 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/ ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type 2006-02-25 23:07 ` Stefan Richter @ 2006-02-25 23:22 ` Al Viro 2006-02-26 8:11 ` Stefan Richter 2006-02-26 0:01 ` Linus Torvalds 2006-02-26 23:16 ` [PATCH 2.6.15.4 update] sd: fix memory corruption with broken mode page headers Stefan Richter 2 siblings, 1 reply; 22+ messages in thread From: Al Viro @ 2006-02-25 23:22 UTC (permalink / raw) To: Stefan Richter Cc: Chris Wright, stable, Linus Torvalds, Jody McIntyre, linux-kernel, linux-scsi 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? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type 2006-02-25 23:22 ` Al Viro @ 2006-02-26 8:11 ` Stefan Richter 2006-02-26 8:22 ` Al Viro 0 siblings, 1 reply; 22+ messages in thread From: Stefan Richter @ 2006-02-26 8:11 UTC (permalink / raw) To: Al Viro Cc: Chris Wright, stable, Linus Torvalds, Jody McIntyre, linux-kernel, linux-scsi 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/ ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type 2006-02-26 8:11 ` Stefan Richter @ 2006-02-26 8:22 ` Al Viro 2006-02-26 9:11 ` Stefan Richter 0 siblings, 1 reply; 22+ messages in thread From: Al Viro @ 2006-02-26 8:22 UTC (permalink / raw) To: Stefan Richter Cc: Chris Wright, stable, Linus Torvalds, Jody McIntyre, linux-kernel, linux-scsi 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... ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type 2006-02-26 8:22 ` Al Viro @ 2006-02-26 9:11 ` Stefan Richter 0 siblings, 0 replies; 22+ messages in thread From: Stefan Richter @ 2006-02-26 9:11 UTC (permalink / raw) To: Al Viro Cc: Chris Wright, stable, Linus Torvalds, Jody McIntyre, linux-kernel, linux-scsi 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/ ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type 2006-02-25 23:07 ` Stefan Richter 2006-02-25 23:22 ` Al Viro @ 2006-02-26 0:01 ` Linus Torvalds 2006-02-26 0:17 ` Al Viro 2006-02-26 5:14 ` James Bottomley 2006-02-26 23:16 ` [PATCH 2.6.15.4 update] sd: fix memory corruption with broken mode page headers Stefan Richter 2 siblings, 2 replies; 22+ messages in thread From: Linus Torvalds @ 2006-02-26 0:01 UTC (permalink / raw) To: Stefan Richter Cc: Chris Wright, stable, Jody McIntyre, linux-kernel, linux-scsi, Al Viro 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type 2006-02-26 0:01 ` Linus Torvalds @ 2006-02-26 0:17 ` Al Viro 2006-02-26 0:39 ` Linus Torvalds 2006-02-26 5:14 ` James Bottomley 1 sibling, 1 reply; 22+ messages in thread From: Al Viro @ 2006-02-26 0:17 UTC (permalink / raw) To: Linus Torvalds Cc: Stefan Richter, Chris Wright, stable, Jody McIntyre, linux-kernel, linux-scsi 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... ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type 2006-02-26 0:17 ` Al Viro @ 2006-02-26 0:39 ` Linus Torvalds 2006-02-26 8:39 ` Jeff Garzik 0 siblings, 1 reply; 22+ messages in thread From: Linus Torvalds @ 2006-02-26 0:39 UTC (permalink / raw) To: Al Viro Cc: Stefan Richter, Chris Wright, stable, Jody McIntyre, linux-kernel, linux-scsi 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type 2006-02-26 0:39 ` Linus Torvalds @ 2006-02-26 8:39 ` Jeff Garzik 2006-02-26 9:00 ` Al Viro 0 siblings, 1 reply; 22+ messages in thread From: Jeff Garzik @ 2006-02-26 8:39 UTC (permalink / raw) To: Linus Torvalds Cc: Al Viro, Stefan Richter, Chris Wright, stable, Jody McIntyre, linux-kernel, linux-scsi 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type 2006-02-26 8:39 ` Jeff Garzik @ 2006-02-26 9:00 ` Al Viro 2006-02-26 10:45 ` Jeff Garzik 0 siblings, 1 reply; 22+ messages in thread From: Al Viro @ 2006-02-26 9:00 UTC (permalink / raw) To: Jeff Garzik Cc: Linus Torvalds, Stefan Richter, Chris Wright, stable, Jody McIntyre, linux-kernel, linux-scsi, Junio C Hamano 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? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type 2006-02-26 9:00 ` Al Viro @ 2006-02-26 10:45 ` Jeff Garzik 2006-02-26 11:47 ` Al Viro 0 siblings, 1 reply; 22+ messages in thread From: Jeff Garzik @ 2006-02-26 10:45 UTC (permalink / raw) To: Al Viro Cc: Linus Torvalds, Stefan Richter, Chris Wright, stable, Jody McIntyre, linux-kernel, linux-scsi, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type 2006-02-26 10:45 ` Jeff Garzik @ 2006-02-26 11:47 ` Al Viro 0 siblings, 0 replies; 22+ messages in thread From: Al Viro @ 2006-02-26 11:47 UTC (permalink / raw) To: Jeff Garzik Cc: Linus Torvalds, Stefan Richter, Chris Wright, stable, Jody McIntyre, linux-kernel, linux-scsi, Junio C Hamano 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type 2006-02-26 0:01 ` Linus Torvalds 2006-02-26 0:17 ` Al Viro @ 2006-02-26 5:14 ` James Bottomley 2006-02-26 5:31 ` Al Viro 1 sibling, 1 reply; 22+ messages in thread From: James Bottomley @ 2006-02-26 5:14 UTC (permalink / raw) To: Linus Torvalds Cc: Stefan Richter, Chris Wright, stable, Jody McIntyre, linux-kernel, linux-scsi, Al Viro 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type 2006-02-26 5:14 ` James Bottomley @ 2006-02-26 5:31 ` Al Viro 2006-02-26 8:29 ` Stefan Richter 2006-02-26 14:34 ` James Bottomley 0 siblings, 2 replies; 22+ messages in thread From: Al Viro @ 2006-02-26 5:31 UTC (permalink / raw) To: James Bottomley Cc: Linus Torvalds, Stefan Richter, Chris Wright, stable, Jody McIntyre, linux-kernel, linux-scsi 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type 2006-02-26 5:31 ` Al Viro @ 2006-02-26 8:29 ` Stefan Richter 2006-02-26 14:34 ` James Bottomley 1 sibling, 0 replies; 22+ messages in thread From: Stefan Richter @ 2006-02-26 8:29 UTC (permalink / raw) To: James Bottomley Cc: Al Viro, Linus Torvalds, Chris Wright, stable, Jody McIntyre, linux-kernel, linux-scsi, Andrew Morton 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/ ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type 2006-02-26 5:31 ` Al Viro 2006-02-26 8:29 ` Stefan Richter @ 2006-02-26 14:34 ` James Bottomley 2006-02-26 14:57 ` Al Viro 2006-02-26 16:21 ` Stefan Richter 1 sibling, 2 replies; 22+ messages in thread From: James Bottomley @ 2006-02-26 14:34 UTC (permalink / raw) To: Al Viro Cc: Linus Torvalds, Stefan Richter, Chris Wright, stable, Jody McIntyre, linux-kernel, linux-scsi 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"); ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type 2006-02-26 14:34 ` James Bottomley @ 2006-02-26 14:57 ` Al Viro 2006-02-26 16:21 ` Stefan Richter 1 sibling, 0 replies; 22+ messages in thread From: Al Viro @ 2006-02-26 14:57 UTC (permalink / raw) To: James Bottomley Cc: Linus Torvalds, Stefan Richter, Chris Wright, stable, Jody McIntyre, linux-kernel, linux-scsi 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [stable] [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type 2006-02-26 14:34 ` James Bottomley 2006-02-26 14:57 ` Al Viro @ 2006-02-26 16:21 ` Stefan Richter 1 sibling, 0 replies; 22+ messages in thread From: Stefan Richter @ 2006-02-26 16:21 UTC (permalink / raw) To: James Bottomley Cc: Al Viro, Linus Torvalds, Chris Wright, stable, Jody McIntyre, linux-kernel, linux-scsi 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/ ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2.6.15.4 update] sd: fix memory corruption with broken mode page headers 2006-02-25 23:07 ` Stefan Richter 2006-02-25 23:22 ` Al Viro 2006-02-26 0:01 ` Linus Torvalds @ 2006-02-26 23:16 ` Stefan Richter 2006-02-27 20:25 ` [stable] " Chris Wright 2 siblings, 1 reply; 22+ messages in thread From: Stefan Richter @ 2006-02-26 23:16 UTC (permalink / raw) To: stable; +Cc: linux-kernel, Al Viro, James Bottomley 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"); ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [stable] [PATCH 2.6.15.4 update] sd: fix memory corruption with broken mode page headers 2006-02-26 23:16 ` [PATCH 2.6.15.4 update] sd: fix memory corruption with broken mode page headers Stefan Richter @ 2006-02-27 20:25 ` Chris Wright 0 siblings, 0 replies; 22+ messages in thread From: Chris Wright @ 2006-02-27 20:25 UTC (permalink / raw) To: Stefan Richter; +Cc: stable, James Bottomley, linux-kernel, Al Viro * Stefan Richter (stefanr@s5r6.in-berlin.de) wrote: > sd: fix memory corruption with broken mode page headers Thanks, queued this one for -stable. -chris ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2006-02-27 20:25 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2006-02-23 1:02 [PATCH 1/2] sd: fix memory corruption by sd_read_cache_type Stefan Richter 2006-02-25 2:10 ` [stable] " Chris Wright 2006-02-25 23:07 ` Stefan Richter 2006-02-25 23:22 ` Al Viro 2006-02-26 8:11 ` Stefan Richter 2006-02-26 8:22 ` Al Viro 2006-02-26 9:11 ` Stefan Richter 2006-02-26 0:01 ` Linus Torvalds 2006-02-26 0:17 ` Al Viro 2006-02-26 0:39 ` Linus Torvalds 2006-02-26 8:39 ` Jeff Garzik 2006-02-26 9:00 ` Al Viro 2006-02-26 10:45 ` Jeff Garzik 2006-02-26 11:47 ` Al Viro 2006-02-26 5:14 ` James Bottomley 2006-02-26 5:31 ` Al Viro 2006-02-26 8:29 ` Stefan Richter 2006-02-26 14:34 ` James Bottomley 2006-02-26 14:57 ` Al Viro 2006-02-26 16:21 ` Stefan Richter 2006-02-26 23:16 ` [PATCH 2.6.15.4 update] sd: fix memory corruption with broken mode page headers Stefan Richter 2006-02-27 20:25 ` [stable] " Chris Wright
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).