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

[<ffffffff8125da9f>] __sync_dirty_buffer+0xaf/0x100
[<ffffffff8125db03>] sync_dirty_buffer+0x13/0x20
[<ffffffffa03f0d57>] ext2_sync_super+0xb7/0xc0 [ext2]
[<ffffffffa03f10b9>] ext2_error+0x119/0x130 [ext2]
[<ffffffffa03e9d93>] ext2_free_blocks+0x83/0x350 [ext2]
[<ffffffffa03f3d03>] ext2_xattr_delete_inode+0x173/0x190 [ext2]
[<ffffffffa03ee9e9>] ext2_evict_inode+0xc9/0x130 [ext2]
[<ffffffff8123fd23>] evict+0xb3/0x180
[<ffffffff81240008>] iput+0x1b8/0x240
[<ffffffff8123c4ac>] d_delete+0x11c/0x150
[<ffffffff8122fa7e>] vfs_rmdir+0xfe/0x120
[<ffffffff812340ee>] do_rmdir+0x17e/0x1f0
[<ffffffff81234dd6>] SyS_rmdir+0x16/0x20
[<ffffffff81838cf2>] entry_SYSCALL_64_fastpath+0x1a/0xa4
[<ffffffffffffffff>] 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 <cmaiolino@redhat.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>

diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
index 9f9992b37924a43eb4da2c56ecb9b65f68d628a2..4c40c0786e168bf1c9c92a5c9afe3d538f63142f 100644 (file)
--- 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 170939f379d74d84ca38bf9fd11d22483495fd3d..3fb93681bf7f7fba5f87bff7d9f9ae37f43e4049 100644 (file)
--- 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 fcbe58641e401bf79597a6d3ca0fdea0b5136d87..d5c7d09919f318066c1be57cf446624875ba229a 100644 (file)
--- 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 1a5e3bff0b63c93454324a919fc1e008c0bc0193..b7f896f3f7a7fb83160ee049f2c119908668cb64 100644 (file)
--- 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",