[PATCH] libext2fs: fix inode cache overruns

[PATCH] libext2fs: fix inode cache overruns

[Eric Whitney]

An inode cache slot will be overrun if a caller to ext2fs_read_inode_full()
or ext2fs_write_inode_full() attempts to read or write a full sized 156
byte inode when the target filesystem contains 128 byte inodes.  Limit the
copied inode to the smaller of the target filesystem's or the caller's
requested inode size.

Signed-off-by: Eric Whitney <enwlinux@gmail.com>
---
 lib/ext2fs/inode.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/ext2fs/inode.c b/lib/ext2fs/inode.c
index 0ea210e..e47d664 100644
--- a/lib/ext2fs/inode.c
+++ b/lib/ext2fs/inode.c
@@ -582,7 +582,8 @@ errcode_t ext2fs_read_inode_full(ext2_filsys fs, ext2_ino_t ino,
 	/* Check to see if it's in the inode cache */
 	for (i = 0; i < fs->icache->cache_size; i++) {
 		if (fs->icache->cache[i].ino == ino) {
-			memcpy(inode, fs->icache->cache[i].inode, bufsize);
+			memcpy(inode, fs->icache->cache[i].inode,
+			       (bufsize > length) ? length : bufsize);
 			return 0;
 		}
 	}
@@ -649,7 +650,7 @@ errcode_t ext2fs_read_inode_full(ext2_filsys fs, ext2_ino_t ino,
 	/* Update the inode cache bookkeeping */
 	fs->icache->cache_last = cache_slot;
 	fs->icache->cache[cache_slot].ino = ino;
-	memcpy(inode, iptr, bufsize);
+	memcpy(inode, iptr, (bufsize > length) ? length : bufsize);
 
 	return 0;
 }
@@ -705,7 +706,7 @@ errcode_t ext2fs_write_inode_full(ext2_filsys fs, ext2_ino_t ino,
 		for (i=0; i < fs->icache->cache_size; i++) {
 			if (fs->icache->cache[i].ino == ino) {
 				memcpy(fs->icache->cache[i].inode, inode,
-				       bufsize);
+				       (bufsize > length) ? length : bufsize);
 				break;
 			}
 		}
-- 
1.7.10.4

Re: [PATCH] libext2fs: fix inode cache overruns

[Eric Whitney]

* Eric Whitney <enwlinux@gmail.com>:
> 
> An inode cache slot will be overrun if a caller to ext2fs_read_inode_full()
> or ext2fs_write_inode_full() attempts to read or write a full sized 156
> byte inode when the target filesystem contains 128 byte inodes.  Limit the
> copied inode to the smaller of the target filesystem's or the caller's
> requested inode size.
> 
> Signed-off-by: Eric Whitney <enwlinux@gmail.com>
> ---
>  lib/ext2fs/inode.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/ext2fs/inode.c b/lib/ext2fs/inode.c
> index 0ea210e..e47d664 100644
> --- a/lib/ext2fs/inode.c
> +++ b/lib/ext2fs/inode.c
> @@ -582,7 +582,8 @@ errcode_t ext2fs_read_inode_full(ext2_filsys fs, ext2_ino_t ino,
>  	/* Check to see if it's in the inode cache */
>  	for (i = 0; i < fs->icache->cache_size; i++) {
>  		if (fs->icache->cache[i].ino == ino) {
> -			memcpy(inode, fs->icache->cache[i].inode, bufsize);
> +			memcpy(inode, fs->icache->cache[i].inode,
> +			       (bufsize > length) ? length : bufsize);
>  			return 0;
>  		}
>  	}
> @@ -649,7 +650,7 @@ errcode_t ext2fs_read_inode_full(ext2_filsys fs, ext2_ino_t ino,
>  	/* Update the inode cache bookkeeping */
>  	fs->icache->cache_last = cache_slot;
>  	fs->icache->cache[cache_slot].ino = ino;
> -	memcpy(inode, iptr, bufsize);
> +	memcpy(inode, iptr, (bufsize > length) ? length : bufsize);
>  
>  	return 0;
>  }
> @@ -705,7 +706,7 @@ errcode_t ext2fs_write_inode_full(ext2_filsys fs, ext2_ino_t ino,
>  		for (i=0; i < fs->icache->cache_size; i++) {
>  			if (fs->icache->cache[i].ino == ino) {
>  				memcpy(fs->icache->cache[i].inode, inode,
> -				       bufsize);
> +				       (bufsize > length) ? length : bufsize);
>  				break;
>  			}
>  		}
> -- 
> 1.7.10.4
> 

Ted:

Here's some additional background for this patch for a regression
introduced in e2fsprogs commit 91db7e206d as found in 1.43-WIP
(master branch).

e2fsprog's f_dup4 test fails when run on a Pandaboard installed with
Ubuntu 12.10 Server and running custom 3.7-rc* kernels.  glibc detects
a heap problem and emits this message:

*** glibc detected *** ../debugfs/debugfs: double free or corruption (out):
 0x0004b6f8 ***

This happens at the beginning of the f_dup4 test in debugfs, which is
used to modify/prepare a test filesystem for subsequent fsck'ing (object
of the test) - the failure occurs after debugfs has made some but not all
of the mods, and then causes debugfs to abort.  The test script doesn't
check for trouble, so fsck then proceeds to run (successfully) on a
partially-modified filesystem.  The resulting fsck output does not match
the output expected for a fully-modified filesystem, and the test fails.

The root cause is the inode cache is being overrun.  The test filesystem
contains 128 byte inodes, so the inode cache is allocated with 128 byte
inodes.  However, debugfs's do_set_inode() passes a bufsize of
sizeof(ext2_inode_large) - 156 bytes - down to ext2fs_{read|write}_inode_full.
Since the bufsize is used to determine how much data to read from or write
to the inode cache, we get overruns.

f_dup4 does not fail when run on an x86_64 test system, even though the 
test filesystem still contains 128 byte inodes.  glibc does not detect an
error, debugfs runs to completion, and the fsck output is as expected.
However, if f_dup4 is run with --valgrind after modifying the f_dup4
script to capture the debugfs output rather than direct it to /dev/null,
valgrind reports inode cache overruns (same is true on the Pandaboard).

Interestingly, f_dup4 does fail when run on a 32 bit x86 system - glibc
detects and reports the heap problem and forces debugfs to abort in a
manner identical to that seen on the Pandaboard.  So, we see failures
on the 32 bit systems but not on the 64 bit.  It's not yet clear to me
why glibc isn't catching the overruns on x86_64, but that's what's behind
my reports of different f_dup4 test behavior in recent ext4 concalls.

Thanks,
Eric

Re: [PATCH] libext2fs: fix inode cache overruns

[Theodore Ts'o]

On Sat, Nov 17, 2012 at 01:37:45PM -0500, Eric Whitney wrote:
> An inode cache slot will be overrun if a caller to ext2fs_read_inode_full()
> or ext2fs_write_inode_full() attempts to read or write a full sized 156
> byte inode when the target filesystem contains 128 byte inodes.  Limit the
> copied inode to the smaller of the target filesystem's or the caller's
> requested inode size.
> 
> Signed-off-by: Eric Whitney <enwlinux@gmail.com>

Thanks, applied.

I investigated and discovered why running the ext4 regression tests
using valgrind didn't discover the problem.  The following commit
allowed the bug to be trivially surfaced by running the command:

	cd tests; ./test_script --valgrind

I've now verified that the with your patch and this one, plus another
fix which I've applied to the maint branch to fix various gcc -Wall
complaints in resize2fs, "./test_script --valgrind" now runs cleanly.

							- Ted

commit 603e5ebc8bb4b5e75c53ddf1461992f8861b35a1
Author: Theodore Ts'o <tytso@mit.edu>
Date:   Thu Nov 29 20:40:21 2012 -0500

    libext2fs: allocate separate memory regions for each inode in the cache
    
    The changes to support metadata checksum allocated a single large
    array for all of the inodes in the inode cache.  This is slightly more
    efficient, but given that the inode cache is small (only 4 inodes) it
    doesn't really have that much benefit.  The problem with doing things
    this way is that the memory overruns, such as the one fixed in commit
    43c4910371a, do not get detected by valgrind.
    
    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
index 9148d4e..7ec189e 100644
--- a/lib/ext2fs/ext2fs.h
+++ b/lib/ext2fs/ext2fs.h
@@ -1328,6 +1328,7 @@ extern errcode_t ext2fs_get_memalign(unsigned long size,
 				     unsigned long align, void *ptr);
 
 /* inode.c */
+extern void ext2fs_free_inode_cache(struct ext2_inode_cache *icache);
 extern errcode_t ext2fs_flush_icache(ext2_filsys fs);
 extern errcode_t ext2fs_get_next_inode_full(ext2_inode_scan scan,
 					    ext2_ino_t *ino,
diff --git a/lib/ext2fs/freefs.c b/lib/ext2fs/freefs.c
index 28c4132..d3e815c 100644
--- a/lib/ext2fs/freefs.c
+++ b/lib/ext2fs/freefs.c
@@ -18,8 +18,6 @@
 #include "ext2_fs.h"
 #include "ext2fsP.h"
 
-static void ext2fs_free_inode_cache(struct ext2_inode_cache *icache);
-
 void ext2fs_free(ext2_filsys fs)
 {
 	if (!fs || (fs->magic != EXT2_ET_MAGIC_EXT2FS_FILSYS))
@@ -65,21 +63,6 @@ void ext2fs_free(ext2_filsys fs)
 }
 
 /*
- * Free the inode cache structure
- */
-static void ext2fs_free_inode_cache(struct ext2_inode_cache *icache)
-{
-	if (--icache->refcount)
-		return;
-	if (icache->buffer)
-		ext2fs_free_mem(&icache->buffer);
-	if (icache->cache)
-		ext2fs_free_mem(&icache->cache);
-	icache->buffer_blk = 0;
-	ext2fs_free_mem(&icache);
-}
-
-/*
  * This procedure frees a badblocks list.
  */
 void ext2fs_u32_list_free(ext2_u32_list bb)
diff --git a/lib/ext2fs/inode.c b/lib/ext2fs/inode.c
index e47d664..f877146 100644
--- a/lib/ext2fs/inode.c
+++ b/lib/ext2fs/inode.c
@@ -72,6 +72,25 @@ errcode_t ext2fs_flush_icache(ext2_filsys fs)
 	return 0;
 }
 
+/*
+ * Free the inode cache structure
+ */
+void ext2fs_free_inode_cache(struct ext2_inode_cache *icache)
+{
+	int i;
+
+	if (--icache->refcount)
+		return;
+	if (icache->buffer)
+		ext2fs_free_mem(&icache->buffer);
+	for (i = 0; i < icache->cache_size; i++)
+		ext2fs_free_mem(&icache->cache[i].inode);
+	if (icache->cache)
+		ext2fs_free_mem(&icache->cache);
+	icache->buffer_blk = 0;
+	ext2fs_free_mem(&icache);
+}
+
 static errcode_t create_icache(ext2_filsys fs)
 {
 	int		i;
@@ -86,31 +105,32 @@ static errcode_t create_icache(ext2_filsys fs)
 
 	memset(fs->icache, 0, sizeof(struct ext2_inode_cache));
 	retval = ext2fs_get_mem(fs->blocksize, &fs->icache->buffer);
-	if (retval) {
-		ext2fs_free_mem(&fs->icache);
-		return retval;
-	}
+	if (retval)
+		goto errout;
+
 	fs->icache->buffer_blk = 0;
 	fs->icache->cache_last = -1;
 	fs->icache->cache_size = 4;
 	fs->icache->refcount = 1;
 	retval = ext2fs_get_array(fs->icache->cache_size,
-				  sizeof(struct ext2_inode_cache_ent) +
-				  EXT2_INODE_SIZE(fs->super),
+				  sizeof(struct ext2_inode_cache_ent),
 				  &fs->icache->cache);
-	if (retval) {
-		ext2fs_free_mem(&fs->icache->buffer);
-		ext2fs_free_mem(&fs->icache);
-		return retval;
-	}
+	if (retval)
+		goto errout;
 
-	for (i = 0, p = (void *)(fs->icache->cache + fs->icache->cache_size);
-	     i < fs->icache->cache_size;
-	     i++, p += EXT2_INODE_SIZE(fs->super))
-		fs->icache->cache[i].inode = p;
+	for (i = 0; i < fs->icache->cache_size; i++) {
+		retval = ext2fs_get_mem(EXT2_INODE_SIZE(fs->super),
+					&fs->icache->cache[i].inode);
+		if (retval)
+			goto errout;
+	}
 
 	ext2fs_flush_icache(fs);
 	return 0;
+errout:
+	ext2fs_free_inode_cache(fs->icache);
+	fs->icache = 0;
+	return retval;
 }
 
 errcode_t ext2fs_open_inode_scan(ext2_filsys fs, int buffer_blocks,