From ff0031d848a0cd7002606f9feef958de8d5edf19 Mon Sep 17 00:00:00 2001 From: Carlos Maiolino Date: Tue, 5 Jul 2016 22:02:41 -0400 Subject: ext2: fix filesystem deadlock while reading corrupted xattr block This bug can be reproducible with fsfuzzer, although, I couldn't reproduce it 100% of my tries, it is quite easily reproducible. During the deletion of an inode, ext2_xattr_delete_inode() does not check if the block pointed by EXT2_I(inode)->i_file_acl is a valid data block, this might lead to a deadlock, when i_file_acl == 1, and the filesystem block size is 1024. In that situation, ext2_xattr_delete_inode, will load the superblock's buffer head (instead of a valid i_file_acl block), and then lock that buffer head, which, ext2_sync_super will also try to lock, making the filesystem deadlock in the following stack trace: root 17180 0.0 0.0 113660 660 pts/0 D+ 07:08 0:00 rmdir /media/test/dir1 [] __sync_dirty_buffer+0xaf/0x100 [] sync_dirty_buffer+0x13/0x20 [] ext2_sync_super+0xb7/0xc0 [ext2] [] ext2_error+0x119/0x130 [ext2] [] ext2_free_blocks+0x83/0x350 [ext2] [] ext2_xattr_delete_inode+0x173/0x190 [ext2] [] ext2_evict_inode+0xc9/0x130 [ext2] [] evict+0xb3/0x180 [] iput+0x1b8/0x240 [] d_delete+0x11c/0x150 [] vfs_rmdir+0xfe/0x120 [] do_rmdir+0x17e/0x1f0 [] SyS_rmdir+0x16/0x20 [] entry_SYSCALL_64_fastpath+0x1a/0xa4 [] 0xffffffffffffffff Fix this by using the same approach ext4 uses to test data blocks validity, implementing ext2_data_block_valid. An another possibility when the superblock is very corrupted, is that i_file_acl is 1, block_count is 1 and first_data_block is 0. For such situations, we might have i_file_acl pointing to a 'valid' block, but still step over the superblock. The approach I used was to also test if the superblock is not in the range described by ext2_data_block_valid() arguments Signed-off-by: Carlos Maiolino Signed-off-by: Theodore Ts'o --- fs/ext2/balloc.c | 21 +++++++++++++++++++++ fs/ext2/ext2.h | 3 +++ fs/ext2/inode.c | 10 ++++++++++ fs/ext2/xattr.c | 9 +++++++++ 4 files changed, 43 insertions(+) (limited to 'fs/ext2') diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c index 9f9992b37924..4c40c0786e16 100644 --- a/fs/ext2/balloc.c +++ b/fs/ext2/balloc.c @@ -1193,6 +1193,27 @@ static int ext2_has_free_blocks(struct ext2_sb_info *sbi) return 1; } +/* + * Returns 1 if the passed-in block region is valid; 0 if some part overlaps + * with filesystem metadata blocksi. + */ +int ext2_data_block_valid(struct ext2_sb_info *sbi, ext2_fsblk_t start_blk, + unsigned int count) +{ + if ((start_blk <= le32_to_cpu(sbi->s_es->s_first_data_block)) || + (start_blk + count < start_blk) || + (start_blk > le32_to_cpu(sbi->s_es->s_blocks_count))) + return 0; + + /* Ensure we do not step over superblock */ + if ((start_blk <= sbi->s_sb_block) && + (start_blk + count >= sbi->s_sb_block)) + return 0; + + + return 1; +} + /* * ext2_new_blocks() -- core block(s) allocation function * @inode: file inode diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h index 170939f379d7..3fb93681bf7f 100644 --- a/fs/ext2/ext2.h +++ b/fs/ext2/ext2.h @@ -367,6 +367,7 @@ struct ext2_inode { */ #define EXT2_VALID_FS 0x0001 /* Unmounted cleanly */ #define EXT2_ERROR_FS 0x0002 /* Errors detected */ +#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */ /* * Mount flags @@ -739,6 +740,8 @@ extern unsigned long ext2_bg_num_gdb(struct super_block *sb, int group); extern ext2_fsblk_t ext2_new_block(struct inode *, unsigned long, int *); extern ext2_fsblk_t ext2_new_blocks(struct inode *, unsigned long, unsigned long *, int *); +extern int ext2_data_block_valid(struct ext2_sb_info *sbi, ext2_fsblk_t start_blk, + unsigned int count); extern void ext2_free_blocks (struct inode *, unsigned long, unsigned long); extern unsigned long ext2_count_free_blocks (struct super_block *); diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c index fcbe58641e40..d5c7d09919f3 100644 --- a/fs/ext2/inode.c +++ b/fs/ext2/inode.c @@ -1389,6 +1389,16 @@ struct inode *ext2_iget (struct super_block *sb, unsigned long ino) ei->i_frag_size = raw_inode->i_fsize; ei->i_file_acl = le32_to_cpu(raw_inode->i_file_acl); ei->i_dir_acl = 0; + + if (ei->i_file_acl && + !ext2_data_block_valid(EXT2_SB(sb), ei->i_file_acl, 1)) { + ext2_error(sb, "ext2_iget", "bad extended attribute block %u", + ei->i_file_acl); + brelse(bh); + ret = -EFSCORRUPTED; + goto bad_inode; + } + if (S_ISREG(inode->i_mode)) inode->i_size |= ((__u64)le32_to_cpu(raw_inode->i_size_high)) << 32; else diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c index 1a5e3bff0b63..b7f896f3f7a7 100644 --- a/fs/ext2/xattr.c +++ b/fs/ext2/xattr.c @@ -759,10 +759,19 @@ void ext2_xattr_delete_inode(struct inode *inode) { struct buffer_head *bh = NULL; + struct ext2_sb_info *sbi = EXT2_SB(inode->i_sb); down_write(&EXT2_I(inode)->xattr_sem); if (!EXT2_I(inode)->i_file_acl) goto cleanup; + + if (!ext2_data_block_valid(sbi, EXT2_I(inode)->i_file_acl, 0)) { + ext2_error(inode->i_sb, "ext2_xattr_delete_inode", + "inode %ld: xattr block %d is out of data blocks range", + inode->i_ino, EXT2_I(inode)->i_file_acl); + goto cleanup; + } + bh = sb_bread(inode->i_sb, EXT2_I(inode)->i_file_acl); if (!bh) { ext2_error(inode->i_sb, "ext2_xattr_delete_inode", -- cgit v1.2.3