On 07/02/2016 09:49 AM, Darrick J. Wong wrote:
> On Fri, Jul 01, 2016 at 03:06:41PM +0200, Vegard Nossum wrote:
>> Hi,
>>
>> I've found that sbi->s_es->s_reserved_gdt_blocks is not validated before
>> being used, so e.g. a value of 25600 will overflow the buffer head and
>> corrupt random kernel memory (I've observed 20+ different stack traces
>> due to this bug! many of them long after the code has finished).
>
> This function merely initializes a bitmap block once ext4 decides to start
> using the block group, which could be a long time after mount...
>
>> The following patch fixes it for me:
>>
>> diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
>> index 3020fd7..1ea5054 100644
>> --- a/fs/ext4/balloc.c
>> +++ b/fs/ext4/balloc.c
>> @@ -208,6 +208,9 @@ static int ext4_init_block_bitmap(struct super_block
>> *sb,
>>   	memset(bh->b_data, 0, sb->s_blocksize);
>>
>>   	bit_max = ext4_num_base_meta_clusters(sb, block_group);
>> +	if ((bit_max >> 3) >= bh->b_size)
>> +		return -EFSCORRUPTED;
>> +
>>   	for (bit = 0; bit < bit_max; bit++)
>>   		ext4_set_bit(bit, bh->b_data);
>>
>> However, I think there are potentially more bugs later in this function
>> where offsets are not validated so it needs to be reviewed carefully.
>>
>> Another question is whether we should do the validation earlier, e.g. in
>> ext4_fill_super(). I'm not too familiar with the code, but maybe
>> something like the attached patch would be better? It does seem to fix
>> the issue as well.
>
> ...whereas superblock fields such as s_reserved_gdt_blocks ought to be
> checked (and -EFSCORRUPTED'ed) at mount time.  Since we know that BG 0
> has everything (superblock, old school gdt tables, inodes), maybe we
> should make mount check that ext4_num_base_meta_clusters() >
> NBBY * fs->block_size?
>
> (Kinda surprised ext4 doesn't already do this...)

I ran into a second problem (this time it was num_clusters_in_group()
returning a bogus value) with the same symptoms (random memory
corruptions), the new attached patch fixes both problems by checking the
values at mount time.

I'm using (bit_max >> 3) >= sb->s_blocksize which should be equivalent
to what you suggested (num_clusters() > NBBY * fs->block_size).


Vegard