linux-ext4 - Re: [PATCH v3 3/3] e2fsprogs/tune2fs: fix memory write overflow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141114185808.GA11781@birch.djwong.org>
Date:	Fri, 14 Nov 2014 10:58:08 -0800
From:	"Darrick J. Wong" <darrick.wong@...cle.com>
To:	Xiaoguang Wang <wangxg.fnst@...fujitsu.com>
Cc:	linux-ext4@...r.kernel.org, tytso@....edu
Subject: Re: [PATCH v3 3/3] e2fsprogs/tune2fs: fix memory write overflow

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@...fujitsu.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@...cle.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html