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:	Thu, 7 Jul 2016 22:10:40 +0200
From:	Vegard Nossum <vegard.nossum@...cle.com>
To:	"Darrick J. Wong" <darrick.wong@...cle.com>,
	"Theodore Ts'o" <tytso@....edu>
Cc:	Ext4 Developers List <linux-ext4@...r.kernel.org>,
	linux-fsdevel@...r.kernel.org
Subject: Re: [RFC PATCH] ext4: validate number of meta clusters in group

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

View attachment "0001-ext4-validate-number-of-clusters-in-group.patch" of type "text/x-patch" (4426 bytes)

Powered by blists - more mailing lists