Re: [PATCH V2] limit minixfs printks on corrupted dir i_size, CVE-2006-6058

Re: [PATCH V2] limit minixfs printks on corrupted dir i_size, CVE-2006-6058

[Bodo Eggert]

Eric Sandeen <sandeen@sandeen.net> 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: ei@lyUAkvOE.7eggert.dyndns.org wod@2lvjUzk.7eggert.dyndns.org
 P2DmchzHNa@7eggert.dyndns.org eOnB@utcRck.7eggert.dyndns.org
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2] limit minixfs printks on corrupted dir i_size, CVE-2006-6058

[Eric Sandeen]

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

[PATCH V3] limit minixfs printks on corrupted dir i_size, CVE-2006-6058

[Eric Sandeen]

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 <sandeen@redhat.com>

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) {

Re: [PATCH V3] limit minixfs printks on corrupted dir i_size, CVE-2006-6058

[Bodo Eggert]

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?

Re: [PATCH V3] limit minixfs printks on corrupted dir i_size, CVE-2006-6058

[Eric Sandeen]

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*