[v3,3/3] e2fsprogs/tune2fs: fix memory write overflow

If we apply this patch 'e2fsprogs/tune2fs: rewrite metadata checksums when resizing inode size',
we will trigger a segfault, this is because of the inode cache issues.

Firstly we should notice that in expand_inode_table(), we have change the super block's s_inode_size
to new inode size(for example, 256).

Then we re-compute metadata checksums, see below code flow:
|-->rewrite_metadata_checksums
|----->rewrite_inodes
|-------->ext2fs_write_inode_full
In ext2fs_write_inode_full(), if an inode cache is hit, the below code will be executed:
	/* Check to see if the inode cache needs to be updated */
	if (fs->icache) {
		for (i=0; i < fs->icache->cache_size; i++) {
			if (fs->icache->cache[i].ino == ino) {
				memcpy(fs->icache->cache[i].inode, inode,
				       (bufsize > length) ? length : bufsize);
				break;
			}
		}
	}

Before executing rewrite_inodes(), actually the inode in inode cache is allocated by
old inode size(for example, 128), but here the memcpy will obviously write overflow,
'(bufsize > length) ? length : bufsize' here will return 256(new inode size), so this is
wrong, we need to fix this.
I think we should call ext2fs_free_inode_cache() in expand_inode_table(), to drop the
inode cache, because inode size has changed, if necessary, we will re-create this inode cache.

Steps to reproduce this bug(apply 'e2fsprogs/tune2fs: rewrite metadata checksums when resizing inode size' first).
	dd if=/dev/zero of=file.img bs=1M count=128
	device_name=$(/sbin/losetup -f)
	/sbin/losetup -f file.img
	mkfs.ext4 -I 128 -O ^flex_bg $device_name
	tune2fs -I 256 $device_name

Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 misc/tune2fs.c | 2 ++
 1 file changed, 2 insertions(+)

On Fri, Nov 14, 2014 at 05:15:37PM +0800, Xiaoguang Wang wrote:
> If we apply this patch 'e2fsprogs/tune2fs: rewrite metadata checksums when resizing inode size',
> we will trigger a segfault, this is because of the inode cache issues.
> 
> Firstly we should notice that in expand_inode_table(), we have change the super block's s_inode_size
> to new inode size(for example, 256).
> 
> Then we re-compute metadata checksums, see below code flow:
> |-->rewrite_metadata_checksums
> |----->rewrite_inodes
> |-------->ext2fs_write_inode_full
> In ext2fs_write_inode_full(), if an inode cache is hit, the below code will be executed:
> 	/* Check to see if the inode cache needs to be updated */
> 	if (fs->icache) {
> 		for (i=0; i < fs->icache->cache_size; i++) {
> 			if (fs->icache->cache[i].ino == ino) {
> 				memcpy(fs->icache->cache[i].inode, inode,
> 				       (bufsize > length) ? length : bufsize);
> 				break;
> 			}
> 		}
> 	}
> 
> Before executing rewrite_inodes(), actually the inode in inode cache is allocated by
> old inode size(for example, 128), but here the memcpy will obviously write overflow,
> '(bufsize > length) ? length : bufsize' here will return 256(new inode size), so this is
> wrong, we need to fix this.
> I think we should call ext2fs_free_inode_cache() in expand_inode_table(), to drop the
> inode cache, because inode size has changed, if necessary, we will re-create this inode cache.
> 
> Steps to reproduce this bug(apply 'e2fsprogs/tune2fs: rewrite metadata checksums when resizing inode size' first).
> 	dd if=/dev/zero of=file.img bs=1M count=128
> 	device_name=$(/sbin/losetup -f)
> 	/sbin/losetup -f file.img
> 	mkfs.ext4 -I 128 -O ^flex_bg $device_name
> 	tune2fs -I 256 $device_name
> 
> Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  misc/tune2fs.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> index 91dc7c1..34d2cd9 100644
> --- a/misc/tune2fs.c
> +++ b/misc/tune2fs.c
> @@ -2244,6 +2244,8 @@ static int expand_inode_table(ext2_filsys fs, unsigned long new_ino_size)
>  
>  	/* Update the meta data */
>  	fs->inode_blocks_per_group = new_ino_blks_per_grp;
> +	ext2fs_free_inode_cache(fs->icache);
> +	fs->icache = 0;

By the way (and this is a general question, not one specific to just this
patch), why do some parts of e2fsprogs assign 0 (instead of NULL) to pointers?

--D

>  	fs->super->s_inode_size = new_ino_size;
>  
>  err_out:
> -- 
> 1.8.2.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Applied, thanks.

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html