(unknown)

PATCH New Eric Sesterhenn 2 participants 11 messages Jan 22, 2009

Share & tools

Eric Sesterhenn

author

2009-01-22 08:56 UTC #0 lore raw

As suggested by pavel i tested how e2fsck handles corrupted images, I used the fuzzer bunny (http://code.google.com/p/bunny-the-fuzzer/)
At http://www.cccmz.de/~snakebyte/e2fsck_err.tar.bz2 you can find a bunch of images crashing e2fsck or keeping it in an endless loop. I tested with e2fsck 1.41.0 which was the one i had at hand.

The crashes are either in ext2fs_inode_alloc_stats2() or ext2fs_read_inode_full(), looks like those are always the same faults.

000 endless loop
000 endless loop
053 ext2fs_inode_alloc_stats2
054 ext2fs_inode_alloc_stats2
073 different endless loop?
086 ext2fs_inode_alloc_stats2
112 ext2fs_read_inode_full
139 ext2fs_inode_alloc_stats2
143 ext2fs_inode_alloc_stats2
161 ext2fs_inode_alloc_stats2
192 ext2fs_inode_alloc_stats2
209 ext2fs_inode_alloc_stats2
214 endless loop
216 ext2fs_read_inode_full
241 endless loop
266 endless loop
303 ext2fs_inode_alloc_stats2
389 ext2fs_inode_alloc_stats2
438 ext2fs_inode_alloc_stats2
440 endless loop
446 ext2fs_inode_alloc_stats2
449 ext2fs_read_inode_full
451 ext2fs_read_inode_full
455 ext2fs_read_inode_full
518 ext2fs_inode_alloc_stats2
530 ext2fs_inode_alloc_stats2
534 ext2fs_inode_alloc_stats2

Greetings, Eric

Eric Sesterhenn

author

2009-01-22 11:13 UTC #1 lore raw

Re: e2fsck faults with corrupted images

Quoted 9 lines · * Eric Sesterhenn ([email protected]) wrote:

As suggested by pavel i tested how e2fsck handles corrupted images, I used the fuzzer bunny (http://code.google.com/p/bunny-the-fuzzer/)
At http://www.cccmz.de/~snakebyte/e2fsck_err.tar.bz2 you can find a bunch of images crashing e2fsck or keeping it in an endless loop. I tested with e2fsck 1.41.0 which was the one i had at hand.
The crashes are either in ext2fs_inode_alloc_stats2() or ext2fs_read_inode_full(), looks like those are always the same faults.

I just tried again with e2sck 1.41.3 and I can still produce the errors.

Greetings, Eric

Theodore Tso

2009-01-22 21:12 UTC #2 lore raw

Re: e2fsck faults with corrupted images

Quoted 10 lines · Eric Sesterhenn

Quoted 9 lines · * Eric Sesterhenn ([email protected]) wrote:
As suggested by pavel i tested how e2fsck handles corrupted images, I used the fuzzer bunny (http://code.google.com/p/bunny-the-fuzzer/)
At http://www.cccmz.de/~snakebyte/e2fsck_err.tar.bz2 you can find a bunch of images crashing e2fsck or keeping it in an endless loop. I tested with e2fsck 1.41.0 which was the one i had at hand.
The crashes are either in ext2fs_inode_alloc_stats2() or ext2fs_read_inode_full(), looks like those are always the same faults.

Thanks, they were all traced to the superblock parameter s_first_ino being extremely large --- much larger than s_inodes_count. I've committed the following patches to address the problem at multiple levels.

- Ted

Theodore Ts'o

2009-01-22 21:32 UTC #3 lore raw

PATCH 1/1 e2fsck: Add superblock check to make sure s_first_ino is valid

An deliberately corrupted filesystem with an insanely large s_first_ino field could cause e2fsck to crash with a seg fault.

Thanks to Eric Sesterhenn for supplying test cases which demonstrated this issue.

Signed-off-by "Theodore Ts'o" <[email protected]>

---
 e2fsck/super.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

e2fsck/super.c +4 -0

index cd2b9f0..24ec7a8 100644
@@ -513,6 +513,10 @@ void check_super_block(e2fsck_t ctx)
 	check_super_value(ctx, "reserved_gdt_blocks",
 			  sb->s_reserved_gdt_blocks, MAX_CHECK, 0,
 			  fs->blocksize/4);
+	if (sb->s_rev_level > EXT2_GOOD_OLD_REV)
+		check_super_value(ctx, "first_ino", sb->s_first_ino,
+				  MIN_CHECK | MAX_CHECK,
+				  EXT2_GOOD_OLD_FIRST_INO, sb->s_inodes_count);
 	inode_size = EXT2_INODE_SIZE(sb);
 	check_super_value(ctx, "inode_size",
 			  inode_size, MIN_CHECK | MAX_CHECK,

Signature · 1 line

1.6.0.4.8.g36f27.dirty

Theodore Ts'o

2009-01-22 21:32 UTC #4 lore raw

PATCH 1/1 ext2fs_new_inode(): Add sanity check to assure a valid inode number

Add a sanity check to makesure that even if the superblock field s_first_inode is insane, that we won't return an invalid inode number. (The function will return the error EXT2_ET_INODE_ALLOC_FAIL in that case.)

Signed-off-by "Theodore Ts'o" <[email protected]>

---
 lib/ext2fs/alloc.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

lib/ext2fs/alloc.c +2 -0

index be2b56b..ade5149 100644
@@ -120,6 +120,8 @@ errcode_t ext2fs_new_inode(ext2_filsys fs, ext2_ino_t dir,
 	start_inode = (dir_group * EXT2_INODES_PER_GROUP(fs->super)) + 1;
 	if (start_inode < EXT2_FIRST_INODE(fs->super))
 		start_inode = EXT2_FIRST_INODE(fs->super);
+	if (start_inode > fs->super->s_inodes_count)
+		return EXT2_ET_INODE_ALLOC_FAIL;
 	i = start_inode;
 
 	do {

Signature · 1 line

1.6.0.4.8.g36f27.dirty

Theodore Ts'o

2009-01-22 21:32 UTC #5 lore raw

PATCH 1/1 libext2fs: Add sanity checks to ext2fs_{block,inode}_alloc_stats

If ext2fs_inode_alloc_stats2() or ext2fs_block_alloc_stats() is passed an insanely large inode or block number, it's possible for these functions to overrun an array boundary and cause the calling program to crash with a memory error.

Detect this case, and since these functions don't return an error code, print a warning message, much like we do in ext2fs_warn_bitmap2().

Signed-off-by "Theodore Ts'o" <[email protected]>

---
 lib/ext2fs/alloc_stats.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

lib/ext2fs/alloc_stats.c +14 -0

index 97661dc..d523b43 100644
@@ -20,6 +20,13 @@ void ext2fs_inode_alloc_stats2(ext2_filsys fs, ext2_ino_t ino,
 {
 	int	group = ext2fs_group_of_ino(fs, ino);
 
+#ifndef OMIT_COM_ERR
+	if (ino > fs->super->s_inodes_count) {
+		com_err("ext2fs_inode_alloc_stats2", 0,
+			"Illegal inode number: %lu", ino);
+		return;
+	}
+#endif
 	if (inuse > 0)
 		ext2fs_mark_inode_bitmap(fs->inode_map, ino);
 	else

@@ -58,6 +65,13 @@ void ext2fs_block_alloc_stats(ext2_filsys fs, blk_t blk, int inuse)
 {
 	int	group = ext2fs_group_of_blk(fs, blk);
 
+#ifndef OMIT_COM_ERR
+	if (blk >= fs->super->s_blocks_count) {
+		com_err("ext2fs_block_alloc_stats2", 0,
+			"Illegal block number: %lu", blk);
+		return;
+	}
+#endif
 	if (inuse > 0)
 		ext2fs_mark_block_bitmap(fs->block_map, blk);
 	else

Signature · 1 line

1.6.0.4.8.g36f27.dirty

Theodore Ts'o

2009-01-22 21:32 UTC #6 lore raw

PATCH 1/1 e2fsck: Change PR_3_CREATE_LPF_ERROR to be a non-fatal problem

The other problem codes associated with failing to create the lost+found directory are non-fatal, and this one should be non-fatal as well. The two places which call e2fsck_get_lost_and_found() already deal with a failure to create the directory, so there's no point making this be a fatal error.

Signed-off-by "Theodore Ts'o" <[email protected]>

---
 e2fsck/problem.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

e2fsck/problem.c +1 -1

index 9d193c3..3f53350 100644
@@ -1386,7 +1386,7 @@ static struct e2fsck_problem problem_table[] = {
 	/* Error creating lost and found directory */
 	{ PR_3_CREATE_LPF_ERROR,
 	  N_("Error creating /@l @d (%s): %m\n"),
-	  PROMPT_NONE, PR_FATAL },
+	  PROMPT_NONE, 0 },
 
 	/* Root inode is not directory; aborting */
 	{ PR_3_ROOT_NOT_DIR_ABORT,

Signature · 1 line

1.6.0.4.8.g36f27.dirty

Theodore Ts'o

2009-01-22 21:33 UTC #7 lore raw

PATCH 1/1 e2fsck: Add superblock check to make sure s_first_ino is valid

In reply to Theodore Tso

An deliberately corrupted filesystem with an insanely large s_first_ino field could cause e2fsck to crash with a seg fault.

Thanks to Eric Sesterhenn for supplying test cases which demonstrated this issue.

Signed-off-by "Theodore Ts'o" <[email protected]>

---
 e2fsck/super.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

e2fsck/super.c +4 -0

index cd2b9f0..24ec7a8 100644
@@ -513,6 +513,10 @@ void check_super_block(e2fsck_t ctx)
 	check_super_value(ctx, "reserved_gdt_blocks",
 			  sb->s_reserved_gdt_blocks, MAX_CHECK, 0,
 			  fs->blocksize/4);
+	if (sb->s_rev_level > EXT2_GOOD_OLD_REV)
+		check_super_value(ctx, "first_ino", sb->s_first_ino,
+				  MIN_CHECK | MAX_CHECK,
+				  EXT2_GOOD_OLD_FIRST_INO, sb->s_inodes_count);
 	inode_size = EXT2_INODE_SIZE(sb);
 	check_super_value(ctx, "inode_size",
 			  inode_size, MIN_CHECK | MAX_CHECK,

Signature · 1 line

1.6.0.4.8.g36f27.dirty

Theodore Ts'o

2009-01-22 21:33 UTC #8 lore raw

PATCH 1/1 ext2fs_new_inode(): Add sanity check to assure a valid inode number

Signed-off-by "Theodore Ts'o" <[email protected]>

---
 lib/ext2fs/alloc.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

lib/ext2fs/alloc.c +2 -0

index be2b56b..ade5149 100644
@@ -120,6 +120,8 @@ errcode_t ext2fs_new_inode(ext2_filsys fs, ext2_ino_t dir,
 	start_inode = (dir_group * EXT2_INODES_PER_GROUP(fs->super)) + 1;
 	if (start_inode < EXT2_FIRST_INODE(fs->super))
 		start_inode = EXT2_FIRST_INODE(fs->super);
+	if (start_inode > fs->super->s_inodes_count)
+		return EXT2_ET_INODE_ALLOC_FAIL;
 	i = start_inode;
 
 	do {

Signature · 1 line

1.6.0.4.8.g36f27.dirty

Theodore Ts'o

2009-01-22 21:33 UTC #9 lore raw

PATCH 1/1 libext2fs: Add sanity checks to ext2fs_{block,inode}_alloc_stats

Detect this case, and since these functions don't return an error code, print a warning message, much like we do in ext2fs_warn_bitmap2().

Signed-off-by "Theodore Ts'o" <[email protected]>

---
 lib/ext2fs/alloc_stats.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

lib/ext2fs/alloc_stats.c +14 -0

index 97661dc..d523b43 100644
@@ -20,6 +20,13 @@ void ext2fs_inode_alloc_stats2(ext2_filsys fs, ext2_ino_t ino,
 {
 	int	group = ext2fs_group_of_ino(fs, ino);
 
+#ifndef OMIT_COM_ERR
+	if (ino > fs->super->s_inodes_count) {
+		com_err("ext2fs_inode_alloc_stats2", 0,
+			"Illegal inode number: %lu", ino);
+		return;
+	}
+#endif
 	if (inuse > 0)
 		ext2fs_mark_inode_bitmap(fs->inode_map, ino);
 	else

@@ -58,6 +65,13 @@ void ext2fs_block_alloc_stats(ext2_filsys fs, blk_t blk, int inuse)
 {
 	int	group = ext2fs_group_of_blk(fs, blk);
 
+#ifndef OMIT_COM_ERR
+	if (blk >= fs->super->s_blocks_count) {
+		com_err("ext2fs_block_alloc_stats2", 0,
+			"Illegal block number: %lu", blk);
+		return;
+	}
+#endif
 	if (inuse > 0)
 		ext2fs_mark_block_bitmap(fs->block_map, blk);
 	else

Signature · 1 line

1.6.0.4.8.g36f27.dirty

Theodore Ts'o

2009-01-22 21:33 UTC #10 lore raw

PATCH 1/1 e2fsck: Change PR_3_CREATE_LPF_ERROR to be a non-fatal problem

Signed-off-by "Theodore Ts'o" <[email protected]>

---
 e2fsck/problem.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

e2fsck/problem.c +1 -1

index 9d193c3..3f53350 100644
@@ -1386,7 +1386,7 @@ static struct e2fsck_problem problem_table[] = {
 	/* Error creating lost and found directory */
 	{ PR_3_CREATE_LPF_ERROR,
 	  N_("Error creating /@l @d (%s): %m\n"),
-	  PROMPT_NONE, PR_FATAL },
+	  PROMPT_NONE, 0 },
 
 	/* Root inode is not directory; aborting */
 	{ PR_3_ROOT_NOT_DIR_ABORT,

Signature · 1 line

1.6.0.4.8.g36f27.dirty