On Tue, Apr 24, 2018 at 12:56:54PM +0200, Lukas Czerner wrote: > @@ -354,8 +356,8 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb, > /* check whether the inode table block number is set */ > blk = ext4_inode_table(sb, desc); > offset = blk - group_first_block; > - if (offset < 0 || EXT4_B2C(sbi, offset) >= sb->s_blocksize || > - EXT4_B2C(sbi, offset + sbi->s_itb_per_group) >= sb->s_blocksize) > + if (offset < 0 || EXT4_B2C(sbi, offset + sbi->s_itb_per_group) >= > + EXT4_CLUSTERS_PER_GROUP(sb)) > return blk; > next_zero_bit = ext4_find_next_zero_bit(bh->b_data, > EXT4_B2C(sbi, offset + sbi->s_itb_per_group), The two checks of offset and offset + sbi->s_itb_per_group are necessary because a maliciously crafted file system can take advantage of unsigned integer overflow such that offset is a very large number, but offset + sbi->s_itb_per_group is a legal offset. So we have ot keep both checks. As it happens I was working on a similar patch (but was slowed down by my attendance at LSF/Mm). So I've combined your patch with mine, and came up with this. - Ted >From 33444e3f7da8ae9840286732c0d7bbf8f9d8471b Mon Sep 17 00:00:00 2001 From: Lukas Czerner <lczerner@xxxxxxxxxx> Date: Tue, 24 Apr 2018 11:31:44 -0400 Subject: [PATCH] ext4: fix bitmap position validation Currently in ext4_valid_block_bitmap() we expect the bitmap to be positioned anywhere between 0 and s_blocksize clusters, but that's wrong because the bitmap can be placed anywhere in the block group. This causes false positives when validating bitmaps on perfectly valid file system layouts. Fix it by checking whether the bitmap is within the group boundary. The problem can be reproduced using the following mkfs -t ext3 -E stride=256 /dev/vdb1 mount /dev/vdb1 /mnt/test cd /mnt/test wget https://cdn.kernel.org/pub/linux/kernel/v4.x/linux-4.16.3.tar.xz tar xf linux-4.16.3.tar.xz This will result in the warnings in the logs EXT4-fs error (device vdb1): ext4_validate_block_bitmap:399: comm tar: bg 84: block 2774529: invalid block bitmap [ Changed slightly for clarity and to not drop a overflow test -- TYT ] Signed-off-by: Lukas Czerner <lczerner@xxxxxxxxxx> Signed-off-by: Theodore Ts'o <tytso@xxxxxxx> Reported-by: Ilya Dryomov <idryomov@xxxxxxxxx> Fixes: 7dac4a1726a9 ("ext4: add validity checks for bitmap block numbers") Cc: stable@xxxxxxxxxxxxxxx --- fs/ext4/balloc.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c index a33d8fb1bf2a..508b905d744d 100644 --- a/fs/ext4/balloc.c +++ b/fs/ext4/balloc.c @@ -321,6 +321,7 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb, struct ext4_sb_info *sbi = EXT4_SB(sb); ext4_grpblk_t offset; ext4_grpblk_t next_zero_bit; + ext4_grpblk_t max_bit = EXT4_CLUSTERS_PER_GROUP(sb); ext4_fsblk_t blk; ext4_fsblk_t group_first_block; @@ -338,7 +339,7 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb, /* check whether block bitmap block number is set */ blk = ext4_block_bitmap(sb, desc); offset = blk - group_first_block; - if (offset < 0 || EXT4_B2C(sbi, offset) >= sb->s_blocksize || + if (offset < 0 || EXT4_B2C(sbi, offset) >= max_bit || !ext4_test_bit(EXT4_B2C(sbi, offset), bh->b_data)) /* bad block bitmap */ return blk; @@ -346,7 +347,7 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb, /* check whether the inode bitmap block number is set */ blk = ext4_inode_bitmap(sb, desc); offset = blk - group_first_block; - if (offset < 0 || EXT4_B2C(sbi, offset) >= sb->s_blocksize || + if (offset < 0 || EXT4_B2C(sbi, offset) >= max_bit || !ext4_test_bit(EXT4_B2C(sbi, offset), bh->b_data)) /* bad block bitmap */ return blk; @@ -354,8 +355,8 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb, /* check whether the inode table block number is set */ blk = ext4_inode_table(sb, desc); offset = blk - group_first_block; - if (offset < 0 || EXT4_B2C(sbi, offset) >= sb->s_blocksize || - EXT4_B2C(sbi, offset + sbi->s_itb_per_group) >= sb->s_blocksize) + if (offset < 0 || EXT4_B2C(sbi, offset) >= max_bit || + EXT4_B2C(sbi, offset + sbi->s_itb_per_group) >= max_bit) return blk; next_zero_bit = ext4_find_next_zero_bit(bh->b_data, EXT4_B2C(sbi, offset + sbi->s_itb_per_group), -- 2.16.1.72.g5be1f00a9a