limit minixfs printks on corrupted dir i_size, CVE-2006-6058

PATCH In review Bodo Eggert 3 participants 5 messages Aug 9, 2007 – Aug 15, 2007 2 messages not in the archive VFS

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Bodo Eggert

author

2007-08-09 21:47 UTC #2 lore raw

Quoted 17 lines · Eric Sandeen

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
===================================================================

fs/minix/itree_v1.c static int block_to_path(struct inode * · line 31

Quoted 6 lines

fs/minix/itree_v1.c +2 -1

@@ -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".

Signature · 8 lines

Oops. My brain just hit a bad sector. 

Friß, Spammer: [email protected] [email protected]
 [email protected] [email protected]
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eric Sandeen

2007-08-09 22:08 UTC #3 lore raw

Quoted 9 lines · Bodo Eggert

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.

Quoted 2 lines

- Imagine you have three mounts containing a minix fs, how can you tell which one is the the defective one?

good point.

Quoted 3 lines

- 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.

Quoted 1 line

- 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.

Quoted 1 line

- the printk lacks the loglevel.

As do all other printk's in minixfs... (hm and 11,619 other printk's in the kernel :) )

Quoted 4 lines

- 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

Eric Sandeen

2007-08-13 17:56 UTC #4 lore raw

PATCH v3 1/1 limit minixfs printks on corrupted dir i_size, CVE-2006-6058

In reply to Bodo Eggert

Quoted 18 lines · Bodo Eggert

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?

Quoted 3 lines

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]>

fs/minix/itree_v1.c +7 -2

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

fs/minix/itree_v2.c +7 -2

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

Bodo Eggert

author

2007-08-15 11:51 UTC #5 lore raw

Quoted 20 lines · Eric Sandeen

Quoted 18 lines · Bodo Eggert
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?

Quoted 6 lines

Quoted 3 lines
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.

Quoted 3 lines

Honestly minix.fsck
doesn't handle the situation well either, so at this point I hesitate to recommend it in the print. :)

*g*

Signature · 2 lines

Top 100 things you don't want the sysadmin to say:
79. What's this "any" key I'm supposed to press?

Eric Sandeen

2007-08-15 14:59 UTC #6 lore raw

Quoted 5 lines · Bodo Eggert

Quoted 3 lines
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...

Quoted 1 line

- Does it include thespace for \0?

bdevname calls disk_name which does snprintf(buf, BDEVNAME_SIZE, ...), so yes.

-Eric

Quoted 8 lines

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.
Quoted 3 lines
Honestly minix.fsck
doesn't handle the situation well either, so at this point I hesitate to recommend it in the print. :)
*g*