linux-ext4 - Re: [RFC PATCH] ext4: validate number of meta clusters in group

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]

Message-ID: <20160702074903.GA4914@birch.djwong.org>
Date:	Sat, 2 Jul 2016 00:49:04 -0700
From:	"Darrick J. Wong" <darrick.wong@...cle.com>
To:	Vegard Nossum <vegard.nossum@...cle.com>
Cc:	"Theodore Ts'o" <tytso@....edu>,
	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 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...)

--D

> 
> 
> Vegard

> From efcee80eb78816a4d495224ffc624adf04217044 Mon Sep 17 00:00:00 2001
> From: Vegard Nossum <vegard.nossum@...cle.com>
> Date: Fri, 1 Jul 2016 15:03:39 +0200
> Subject: [PATCH] ext4: validate number of base meta clusters in group
> 
> ---
>  fs/ext4/balloc.c |  4 +---
>  fs/ext4/ext4.h   |  2 ++
>  fs/ext4/super.c  | 10 ++++++++++
>  3 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
> index 3020fd7..ec03f01 100644
> --- a/fs/ext4/balloc.c
> +++ b/fs/ext4/balloc.c
> @@ -22,8 +22,6 @@
>  
>  #include <trace/events/ext4.h>
>  
> -static unsigned ext4_num_base_meta_clusters(struct super_block *sb,
> -					    ext4_group_t block_group);
>  /*
>   * balloc.c contains the blocks allocation and deallocation routines
>   */
> @@ -817,7 +815,7 @@ unsigned long ext4_bg_num_gdb(struct super_block *sb, ext4_group_t group)
>   * This function returns the number of file system metadata clusters at
>   * the beginning of a block group, including the reserved gdt blocks.
>   */
> -static unsigned ext4_num_base_meta_clusters(struct super_block *sb,
> +unsigned ext4_num_base_meta_clusters(struct super_block *sb,
>  				     ext4_group_t block_group)
>  {
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index b84aa1ca..e492b0b 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2270,6 +2270,8 @@ extern ext4_grpblk_t ext4_block_group_offset(struct super_block *sb,
>  extern int ext4_bg_has_super(struct super_block *sb, ext4_group_t group);
>  extern unsigned long ext4_bg_num_gdb(struct super_block *sb,
>  			ext4_group_t group);
> +extern unsigned ext4_num_base_meta_clusters(struct super_block *sb,
> +				     ext4_group_t block_group);
>  extern ext4_fsblk_t ext4_new_meta_blocks(handle_t *handle, struct inode *inode,
>  					 ext4_fsblk_t goal,
>  					 unsigned int flags,
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 563555e..b5a9d28 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3641,6 +3641,16 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>  		goto failed_mount;
>  	}
>  	sbi->s_groups_count = blocks_count;
> +
> +	for (i = 0; i < sbi->s_groups_count; ++i) {
> +		int bit_max = ext4_num_base_meta_clusters(sb, i);
> +		if ((bit_max >> 3) >= sb->s_blocksize) {
> +			ext4_msg(sb, KERN_WARNING, "meta cluster base for "
> +				"group %u exceeds block size", i);
> +			goto failed_mount;
> +		}
> +	}
> +
>  	sbi->s_blockfile_groups = min_t(ext4_group_t, sbi->s_groups_count,
>  			(EXT4_MAX_BLOCK_FILE_PHYS / EXT4_BLOCKS_PER_GROUP(sb)));
>  	db_count = (sbi->s_groups_count + EXT4_DESC_PER_BLOCK(sb) - 1) /
> -- 
> 1.9.1
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html