linux-ext4 - Re: [PATCH] ext4: fix null pointer deref on mount

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: <20090105234411.GD14500@mit.edu>
Date:	Mon, 5 Jan 2009 18:44:11 -0500
From:	Theodore Tso <tytso@....edu>
To:	Thiemo Nagel <thiemo.nagel@...tum.de>
Cc:	Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH] ext4: fix null pointer deref on mount

> @@ -2145,9 +2145,11 @@
>  	if (EXT4_BLOCKS_PER_GROUP(sb) == 0)
>  		goto cantfind_ext4;
>  
> -	/* ensure blocks_count calculation below doesn't sign-extend */
> -	if (ext4_blocks_count(es) + EXT4_BLOCKS_PER_GROUP(sb) <
> -	    le32_to_cpu(es->s_first_data_block) + 1) {
> +	/*
> +	 * ensure blocks_count calculation below doesn't sign-extend
> +	 * and after do_div() still blocks_count > 0
> +	 */
> +	if (ext4_blocks_count(es) < le32_to_cpu(es->s_first_data_block) + 1) {
>  		printk(KERN_WARNING "EXT4-fs: bad geometry: block count %llu, "
>  		       "first data block %u, blocks per group %lu\n",
>  			ext4_blocks_count(es),

I'd rewrite the test as:

	/*
	 * It makes no sense for the first data block to be beyond the end
	 * of the filesystem.
	 */
	if (le32_to_cpu(es->s_first_data_block) >= ext4_blocks_count(es)) {
  		printk(KERN_WARNING "EXT4-fs: bad geometry: first data"
			"block %u is beyond end of filesystem(%llu)\n",
			le32_to_cpu(es->s_first_data_block),
			ext4_blocks_count(es));
		...

There's no point printing the blocks per group if it's no longer in
the test, and having the comment talk about the physical meaning of
the superblock rather some esoteric explanation about sign extension
and do_div() is much more understable.

> @@ -2160,6 +2162,15 @@
>  			EXT4_BLOCKS_PER_GROUP(sb) - 1);
>  	do_div(blocks_count, EXT4_BLOCKS_PER_GROUP(sb));
>  	sbi->s_groups_count = blocks_count;
> +	if (sbi->s_groups_count > ((uint64_t)1<<32) - EXT4_DESC_PER_BLOCK(sb)) {

This can't possibly work, given that s_groups_count is an unsigned
int.  Even if were unsigned long, it wouldn't have worked on an x86_32
machine, since unsigned long is 32 bits on an x86_32.  See why it's
BadBadBad to use unsigned long?  It means that people running on
x86_64 machines can get deluded into thinking that code will work,
when in fact it won't work everywhere.

The blocks_count variable *is* an __u64, so simply using blocks_count
in the test would fix this problem.

						- Ted
--
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