This attempts to address CVE-2006-6058
http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2006-6058
first reported at http://projects.info-pull.com/mokb/MOKB-17-11-2006.html
Essentially a corrupted minix dir inode reporting a very large
i_size will loop for a very long time in minix_readdir, minix_find_entry,
etc, because on EIO they just move on to try the next page. This is
under the BKL, printk'ing as well. This can lock up the machine
for a very long time. A simple approach is to at least limit the nr. of
pages attempted to no more than s_max_size. (s_max_size is about 256MB for
V1, but 2GB for V2; this could still result in a lot of EIO reads in the V2
case, should the retry loops in minix_readdir & friends be short-circuited
somehow instead? A simple "break" rather than "continue" on error would
certainly resolve it, too...)
Signed-off-by: Eric Sandeen <[email protected]>
--- linux-2.6.22-rc4.orig/fs/minix/dir.c
+++ linux-2.6.22-rc4/fs/minix/dir.c
@@ -42,7 +42,15 @@ minix_last_byte(struct inode *inode, uns
static inline unsigned long dir_pages(struct inode *inode)
{
- return (inode->i_size+PAGE_CACHE_SIZE-1)>>PAGE_CACHE_SHIFT;
+ loff_t size = inode->i_size;
+
+ if (size > minix_sb(inode->i_sb)->s_max_size) {
+ printk("%s: inode %lld i_size > s_max_size\n",
+ __FUNCTION__, inode->i_size);
+ size = minix_sb(inode->i_sb)->s_max_size;
+ }
+
+ return (size+PAGE_CACHE_SIZE-1)>>PAGE_CACHE_SHIFT;
}
static int dir_commit_chunk(struct page *page, unsigned from, unsigned to)
Perhaps this is simpler, and preferable. Thanks to adilger for
reminding me about printk_ratelimit. :)
----
This attempts to address CVE-2006-6058
http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2006-6058
first reported at http://projects.info-pull.com/mokb/MOKB-17-11-2006.html
Essentially a corrupted minix dir inode reporting a very large
i_size will loop for a very long time in minix_readdir, minix_find_entry,
etc, because on EIO they just move on to try the next page. This is
under the BKL, printk-storming as well. This can lock up the machine
for a very long time. Simply ratelimiting the printks gets things back
under control.
Signed-off-by: Eric Sandeen <[email protected]>
Index: linux-2.6.22-rc4/fs/minix/itree_v1.c
===================================================================
--- linux-2.6.22-rc4.orig/fs/minix/itree_v1.c
+++ linux-2.6.22-rc4/fs/minix/itree_v1.c
@@ -27,7 +27,8 @@ static int block_to_path(struct inode *
if (block < 0) {
printk("minix_bmap: block<0\n");
} else if (block >= (minix_sb(inode->i_sb)->s_max_size/BLOCK_SIZE)) {
- printk("minix_bmap: block>big\n");
+ if (printk_ratelimit())
+ printk("minix_bmap: block>big\n");
} else if (block < 7) {
offsets[n++] = block;
} else if ((block -= 7) < 512) {
Index: linux-2.6.22-rc4/fs/minix/itree_v2.c
===================================================================
--- linux-2.6.22-rc4.orig/fs/minix/itree_v2.c
+++ linux-2.6.22-rc4/fs/minix/itree_v2.c
@@ -28,7 +28,8 @@ static int block_to_path(struct inode *
if (block < 0) {
printk("minix_bmap: block<0\n");
} else if (block >= (minix_sb(inode->i_sb)->s_max_size/sb->s_blocksize)) {
- printk("minix_bmap: block>big\n");
+ if (printk_ratelimit())
+ printk("minix_bmap: block>big\n");
} else if (block < 7) {
offsets[n++] = block;
} else if ((block -= 7) < 256) {
Eric Sandeen <[email protected]> wrote:
> This attempts to address CVE-2006-6058
> http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2006-6058
>
> first reported at http://projects.info-pull.com/mokb/MOKB-17-11-2006.html
>
> Essentially a corrupted minix dir inode reporting a very large
> i_size will loop for a very long time in minix_readdir, minix_find_entry,
> etc, because on EIO they just move on to try the next page. This is
> under the BKL, printk-storming as well. This can lock up the machine
> for a very long time. Simply ratelimiting the printks gets things back
> under control.
> Index: linux-2.6.22-rc4/fs/minix/itree_v1.c
> ===================================================================
> --- linux-2.6.22-rc4.orig/fs/minix/itree_v1.c
> +++ linux-2.6.22-rc4/fs/minix/itree_v1.c
> @@ -27,7 +27,8 @@ static int block_to_path(struct inode *
> if (block < 0) {
> printk("minix_bmap: block<0\n");
> } else if (block >= (minix_sb(inode->i_sb)->s_max_size/BLOCK_SIZE)) {
> - printk("minix_bmap: block>big\n");
> + if (printk_ratelimit())
> + printk("minix_bmap: block>big\n");
Warning: I'm only looking at the patch.
You are supposed to print an error message for a user, not to write in a
chat window to a 1337 script kiddie. OK, you just matched the current style,
and your patch is IMHO OK for a quick security fix, but:
- Security fixes should be CCed to the security mailing list, shouldn't they?
(It might be security@ or stable@, I'll remember tomorrow, but then I'd
forget to comment)
- Imagine you have three mounts containing a minix fs, how can you tell which
one is the the defective one?
- The message says "minix_bmap", while the patch suggests it's in
block_to_path. Therefore I asume "minix_bmap" to have only random
informational value.
- Does block < 0 or block > $size make a difference?
- the printk lacks the loglevel.
- Asuming minix supports error handling, shouldn't it do something?
I'd suggest a message saying something like "minix: Bad block address on
device 08:15, needs fsck".
--
Oops. My brain just hit a bad sector.
Fri?, Spammer: [email protected] [email protected]
[email protected] [email protected]
Bodo Eggert wrote:
> Warning: I'm only looking at the patch.
>
> You are supposed to print an error message for a user, not to write in a
> chat window to a 1337 script kiddie. OK, you just matched the current style,
> and your patch is IMHO OK for a quick security fix, but:
>
> - Security fixes should be CCed to the security mailing list, shouldn't they?
> (It might be security@ or stable@, I'll remember tomorrow, but then I'd
> forget to comment)
ok.
> - Imagine you have three mounts containing a minix fs, how can you tell which
> one is the the defective one?
good point.
> - The message says "minix_bmap", while the patch suggests it's in
> block_to_path. Therefore I asume "minix_bmap" to have only random
> informational value.
Yup, you're right.
> - Does block < 0 or block > $size make a difference?
well, block > size is likely to arrive from a corrupt i_size, and the
insistence upon going ahead and checking the next page after
encountering an error on the last one... I don't have any scenario in
mind where we'd be repeatedly trying to check blocks < 0.
> - the printk lacks the loglevel.
As do all other printk's in minixfs... (hm and 11,619 other printk's in
the kernel :) )
> - Asuming minix supports error handling, shouldn't it do something?
>
> I'd suggest a message saying something like "minix: Bad block address on
> device 08:15, needs fsck".
Fair enough, as you said I was just fixing up the issue, not rewriting
the code around it. But yes, I should probably have considered at least
a better message here. I can fix this up & resend. But I'm not
promising to audit all other printk's in minixfs this time around. ;-)
-Eric
Bodo Eggert wrote:
> Warning: I'm only looking at the patch.
>
> You are supposed to print an error message for a user, not to write in a
> chat window to a 1337 script kiddie. OK, you just matched the current style,
> and your patch is IMHO OK for a quick security fix, but:
>
> - Security fixes should be CCed to the security mailing list, shouldn't they?
> (It might be security@ or stable@, I'll remember tomorrow, but then I'd
> forget to comment)
> - Imagine you have three mounts containing a minix fs, how can you tell which
> one is the the defective one?
> - The message says "minix_bmap", while the patch suggests it's in
> block_to_path. Therefore I asume "minix_bmap" to have only random
> informational value.
> - Does block < 0 or block > $size make a difference?
> - the printk lacks the loglevel.
> - Asuming minix supports error handling, shouldn't it do something?
>
> I'd suggest a message saying something like "minix: Bad block address on
> device 08:15, needs fsck".
>
Ok, do you like this slightly better? It states the subsystem, the
function with the error, the block nr. in the case of a too-large block,
and the block device on which the error occurred. Honestly minix.fsck
doesn't handle the situation well either, so at this point I hesitate
to recommend it in the print. :)
--------------------------
This attempts to address CVE-2006-6058
http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2006-6058
first reported at http://projects.info-pull.com/mokb/MOKB-17-11-2006.html
Essentially a corrupted minix dir inode reporting a very large
i_size will loop for a very long time in minix_readdir, minix_find_entry,
etc, because on EIO they just move on to try the next page. This is
under the BKL, printk-storming as well. This can lock up the machine
for a very long time. Simply ratelimiting the printks gets things back
under control. Make the message a bit more informative while we're here.
Signed-off-by: Eric Sandeen <[email protected]>
Index: linux-2.6.22-rc4/fs/minix/itree_v1.c
===================================================================
--- linux-2.6.22-rc4.orig/fs/minix/itree_v1.c
+++ linux-2.6.22-rc4/fs/minix/itree_v1.c
@@ -23,11 +23,16 @@ static inline block_t *i_data(struct ino
static int block_to_path(struct inode * inode, long block, int offsets[DEPTH])
{
int n = 0;
+ char b[BDEVNAME_SIZE];
if (block < 0) {
- printk("minix_bmap: block<0\n");
+ printk("MINIX-fs: block_to_path: block %ld < 0 on dev %s\n",
+ block, bdevname(inode->i_sb->s_bdev, b));
} else if (block >= (minix_sb(inode->i_sb)->s_max_size/BLOCK_SIZE)) {
- printk("minix_bmap: block>big\n");
+ if (printk_ratelimit())
+ printk("MINIX-fs: block_to_path: "
+ "block %ld too big on dev %s\n",
+ block, bdevname(inode->i_sb->s_bdev, b));
} else if (block < 7) {
offsets[n++] = block;
} else if ((block -= 7) < 512) {
Index: linux-2.6.22-rc4/fs/minix/itree_v2.c
===================================================================
--- linux-2.6.22-rc4.orig/fs/minix/itree_v2.c
+++ linux-2.6.22-rc4/fs/minix/itree_v2.c
@@ -23,12 +23,17 @@ static inline block_t *i_data(struct ino
static int block_to_path(struct inode * inode, long block, int offsets[DEPTH])
{
int n = 0;
+ char b[BDEVNAME_SIZE];
struct super_block *sb = inode->i_sb;
if (block < 0) {
- printk("minix_bmap: block<0\n");
+ printk("MINIX-fs: block_to_path: block %ld < 0 on dev %s\n",
+ block, bdevname(sb->s_bdev, b));
} else if (block >= (minix_sb(inode->i_sb)->s_max_size/sb->s_blocksize)) {
- printk("minix_bmap: block>big\n");
+ if (printk_ratelimit())
+ printk("MINIX-fs: block_to_path: "
+ "block %ld too big on dev %s\n",
+ block, bdevname(sb->s_bdev, b));
} else if (block < 7) {
offsets[n++] = block;
} else if ((block -= 7) < 256) {
On Mon, 13 Aug 2007, Eric Sandeen wrote:
> Bodo Eggert wrote:
> > Warning: I'm only looking at the patch.
> >
> > You are supposed to print an error message for a user, not to write in a
> > chat window to a 1337 script kiddie. OK, you just matched the current style,
> > and your patch is IMHO OK for a quick security fix, but:
> >
> > - Security fixes should be CCed to the security mailing list, shouldn't they?
> > (It might be security@ or stable@, I'll remember tomorrow, but then I'd
> > forget to comment)
> > - Imagine you have three mounts containing a minix fs, how can you tell which
> > one is the the defective one?
> > - The message says "minix_bmap", while the patch suggests it's in
> > block_to_path. Therefore I asume "minix_bmap" to have only random
> > informational value.
> > - Does block < 0 or block > $size make a difference?
> > - the printk lacks the loglevel.
> > - Asuming minix supports error handling, shouldn't it do something?
> >
> > I'd suggest a message saying something like "minix: Bad block address on
> > device 08:15, needs fsck".
> >
> Ok, do you like this slightly better? It states the subsystem, the
> function with the error, the block nr. in the case of a too-large block,
> and the block device on which the error occurred.
- how long is BDEVNAME_SIZE? Will it fit on the stack?
- Does it include thespace for \0?
I asume you copied other users, and the other users will do it right (or
at least not terribly wrong:), but I can't dig the code right now.
> Honestly minix.fsck
> doesn't handle the situation well either, so at this point I hesitate
> to recommend it in the print. :)
*g*
--
Top 100 things you don't want the sysadmin to say:
79. What's this "any" key I'm supposed to press?
Bodo Eggert wrote:
>> Ok, do you like this slightly better? It states the subsystem, the
>> function with the error, the block nr. in the case of a too-large block,
>> and the block device on which the error occurred.
>
> - how long is BDEVNAME_SIZE? Will it fit on the stack?
#define BDEVNAME_SIZE 32 /* Largest string for a blockdev
identifier */
~60 other users in md, ext3, jbd, buffer.c, etc. place it on the stack...
> - Does it include thespace for \0?
bdevname calls disk_name which does snprintf(buf, BDEVNAME_SIZE, ...),
so yes.
-Eric
> I asume you copied other users, and the other users will do it right (or
> at least not terribly wrong:), but I can't dig the code right now.
>
>> Honestly minix.fsck
>> doesn't handle the situation well either, so at this point I hesitate
>> to recommend it in the print. :)
>
> *g*