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 <[email protected]>
---
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(+)
diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
index 9f9992b..4c40c07 100644
--- a/fs/ext2/balloc.c
+++ b/fs/ext2/balloc.c
@@ -1194,6 +1194,27 @@ static int ext2_has_free_blocks(struct ext2_sb_info *sbi)
}
/*
+ * 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
* @goal: given target block(filesystem wide)
diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h
index 170939f..3fb9368 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 fcbe586..d5c7d09 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 1a5e3bf..b7f896f 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",
--
2.4.11
On Tue, Jun 28, 2016 at 12:42:46PM +0200, Carlos Maiolino wrote:
I apologize, I should have mentioned in the $SUBJ it is an EXT2 patch.
> 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 <[email protected]>
> ---
> 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(+)
>
> diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
> index 9f9992b..4c40c07 100644
> --- a/fs/ext2/balloc.c
> +++ b/fs/ext2/balloc.c
> @@ -1194,6 +1194,27 @@ static int ext2_has_free_blocks(struct ext2_sb_info *sbi)
> }
>
> /*
> + * 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
> * @goal: given target block(filesystem wide)
> diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h
> index 170939f..3fb9368 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 fcbe586..d5c7d09 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 1a5e3bf..b7f896f 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",
> --
> 2.4.11
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Carlos
On Tue, Jun 28, 2016 at 12:42:46PM +0200, Carlos Maiolino wrote:
> 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 <[email protected]>
Thanks, applied. I'll carry this patch in the ext4 tree.
- Ted