linux-ext4 - Re: [PATCH] ext4: fix bitmap position validation

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 30 Apr 2018 10:24:53 +0200
From:   Lukas Czerner <lczerner@...hat.com>
To:     "Theodore Y. Ts'o" <tytso@....edu>
Cc:     linux-ext4@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] ext4: fix bitmap position validation

On Wed, Apr 25, 2018 at 09:39:29AM +0200, Lukas Czerner wrote:
> On Tue, Apr 24, 2018 at 11:43:30AM -0400, Theodore Y. Ts'o wrote:
> > 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.
> 
> Hi Ted,
> 
> maybe I am missing something but offset is signed int, but
> s_itb_per_group is unsigned long, so if I recall the arithmetic
> conversions correctly the offset will be converted to unsigned long and
> the restult will be unsigned long.
> Moreover s_itb_per_group value is sanitized when read and can't be very
> big so the restult will always fit into unsigned long hence no overflow
> is possible. Anyway that was my thought process when I removed the
> additional check.
> 
> However it we have a maliciously created fs then
> 
> blk = ext4_inode_table(sb, desc);
> 
> might be either too big or too small and so
> 
> offset = blk - group_first_block;
> 
> might over/underflow giving us wrong offset that still by chance can land
> into the block group. So maybe we need to check that blk is within the file
> system and/or that offset does not overflow. Maybe making offset type
> ext4_fsblk_t and checking that the result satisfies (offset <= blk) will
> be enough ?
> 
> Have fun at LSF/MM :)
> 
> -Lukas

Well, so much for a discussion :-/ The patch is upstream now...

> 
> > 
> > 					- Ted
> > 
> > From 33444e3f7da8ae9840286732c0d7bbf8f9d8471b Mon Sep 17 00:00:00 2001
> > From: Lukas Czerner <lczerner@...hat.com>
> > 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@...hat.com>
> > Signed-off-by: Theodore Ts'o <tytso@....edu>
> > Reported-by: Ilya Dryomov <idryomov@...il.com>
> > Fixes: 7dac4a1726a9 ("ext4: add validity checks for bitmap block numbers")
> > Cc: stable@...r.kernel.org
> > ---
> >  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
> >