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

[Thiemo Nagel <thiemo.nagel@ph.tum.de>]

[-- Attachment #1: Type: text/plain, Size: 1825 bytes --]

Theodore Tso wrote:
> On Mon, Jan 05, 2009 at 09:50:13PM +0100, Thiemo Nagel wrote:
>> I have chosen unsigned long for the sole reason to avoid truncation in  
>> the assignment
>>
>> db_count = (sbi->s_groups_count + EXT4_DESC_PER_BLOCK(sb) - 1) /
>> 	   EXT4_DESC_PER_BLOCK(sb);
>>
>> where the operands on the right side are of type unsigned long and  
>> ext4_group_t (which is typedef unsigned long), so I don't think to make  
>> db_count an unsigned long is hurting anything.
> 
> Err, no.  ext4_group_t is typedef'ed to be an unsigned int.  

fs/ext4/ext4_i.h, line 34:
typedef unsigned long ext4_group_t;

> And there
> are plenty of places in both the kernel and userspace code where the
> number of groups is assumed to a quantity that can be held in a 2**32
> bit field.  This isn't a problem, because normally the number of
> blocks per group is fs->blocksize*8.  So for a 4k block filesystem,
> the number of blocks per group is 32768, or 2**15.  So that means an
> effective limit of 2**47 blocks before we overflow 2**32 block group
> type width, and with 4k blocks, that means a max volume size of 512
> petabytes.   
> 
>> But maybe it's not desireable to allow filesystems which are mountable  
>> on x86_64 but not on x86_32?  Then a different solution would be to  
>> enforce s_groups_count < (1<<31).
> 
> I'd say enforce s_groups_count < 2**32, because that's the limit we
> have everywhere else.

Fine.

I had another look at the kmalloc() call:  It's not a problem because 
s_groups_count is divided by s_desc_per_block, which always is larger 
than the pointer size.

I have updated the

Patch: fix null pointer dereference on mount

Enforce 1 <= s_groups_count <= 2**32 - s_desc_per_block to avoid invalid 
memory accesses later in the code.

Signed-off-by: Thiemo Nagel <thiemo.nagel@ph.tum.de>


[-- Attachment #2: null_deref.patch2 --]
[-- Type: text/plain, Size: 1649 bytes --]

--- linux-2.6.28-orig/fs/ext4/super.c	2008-12-25 00:26:37.000000000 +0100
+++ linux-2.6.28/fs/ext4/super.c	2009-01-05 23:22:28.000000000 +0100
@@ -1873,8 +1873,8 @@
 	char *cp;
 	int ret = -EINVAL;
 	int blocksize;
-	int db_count;
-	int i;
+	unsigned int db_count;
+	unsigned int i;
 	int needs_recovery, has_huge_files;
 	__le32 features;
 	__u64 blocks_count;
@@ -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),
@@ -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)) {
+		printk(KERN_WARNING "EXT4-fs: groups count too large: %lu "
+		       "(block count %llu, first data block %u, blocks per group %lu)\n",
+			sbi->s_groups_count,
+			ext4_blocks_count(es),
+			le32_to_cpu(es->s_first_data_block),
+			EXT4_BLOCKS_PER_GROUP(sb));
+		goto failed_mount;
+	}
 	db_count = (sbi->s_groups_count + EXT4_DESC_PER_BLOCK(sb) - 1) /
 		   EXT4_DESC_PER_BLOCK(sb);
 	sbi->s_group_desc = kmalloc(db_count * sizeof(struct buffer_head *),