[PATCH] ext4: add checks of block references for non-extent inodes

[PATCH] ext4: add checks of block references for non-extent inodes

[Thiemo Nagel]

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

Dear Ted,

this follows your suggestion from 2009-02-05.  The patch is done 
analogous to Aneesh Kumar's patches for extent validation.  It is based 
on 2.6.29-rc7 with Aneesh's patches:
ext4: Add checks to validate extent entries
ext4: Validate extent details only when read from the disk

Kind regards,

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

[-- Attachment #2: add-blockrefs-checks_vs_2.6.29-rc7.patch --]
[-- Type: text/x-patch, Size: 1753 bytes --]

--- download/linux-2.6.29-rc7-vanilla-extcheck/fs/ext4/inode.c	2009-03-12 16:10:31.000000000 +0100
+++ linux-2.6.29-rc7/fs/ext4/inode.c	2009-03-12 16:09:27.000000000 +0100
@@ -371,6 +371,34 @@
 	return n;
 }
 
+static int __ext4_check_blockref(const char *function, struct inode *inode,
+				 unsigned int *p, unsigned int max) {
+
+	struct ext4_super_block *es = EXT4_SB(inode->i_sb)->s_es;
+	unsigned int offset = 0;
+	while (offset < max && p[offset]) {
+		if (unlikely(p[offset] < le32_to_cpu(es->s_first_data_block) ||
+			     (p[offset] > ext4_blocks_count(es)))) {
+			ext4_error(inode->i_sb, function,
+				   "bad block reference in inode #%lu, "
+				   "offset=%u, blockref=%u",
+				   inode->i_ino, offset, p[offset]);
+			return -EIO;
+		}
+		offset++;
+	}
+	return 0;
+}
+
+
+#define ext4_check_indirect_blockref(inode, bh)                         \
+        __ext4_check_blockref(__func__, inode, (__le32 *)(bh)->b_data,  \
+			      EXT4_ADDR_PER_BLOCK((inode)->i_sb))
+
+#define ext4_check_inode_blockref(inode)                                \
+        __ext4_check_blockref(__func__, inode, EXT4_I(inode)->i_data,   \
+			      EXT4_NDIR_BLOCKS)
+
 /**
  *	ext4_get_branch - read the chain of indirect blocks leading to data
  *	@inode: inode in question
@@ -418,6 +446,9 @@
 		bh = sb_bread(sb, le32_to_cpu(p->key));
 		if (!bh)
 			goto failure;
+		if (ext4_check_indirect_blockref(inode, bh))
+			goto failure;
+                  
 		add_chain(++p, bh, (__le32 *)bh->b_data + *++offsets);
 		/* Reader: end */
 		if (!p->key)
@@ -4302,11 +4333,13 @@
 	if (ei->i_flags & EXT4_EXTENTS_FL) {
 		/* Validate extent which is part of inode */
 		ret = ext4_ext_check_inode(inode);
-		if (ret) {
-			brelse(bh);
-			goto bad_inode;
-		}

[PATCH] ext4: check block references only when read from disk

[Thiemo Nagel]

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

And this one should limit the checks to freshly-read block only.

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



[-- Attachment #2: check-blockrefs-only-when-read-from-disk_vs_2.6.29-rc7.patch --]
[-- Type: text/x-patch, Size: 793 bytes --]

--- download/linux-2.6.29-rc7-vanilla-extcheck/fs/ext4/inode.c	2009-03-12 16:44:19.000000000 +0100
+++ linux-2.6.29-rc7/fs/ext4/inode.c	2009-03-12 16:40:28.000000000 +0100
@@ -443,12 +443,20 @@
 	if (!p->key)
 		goto no_block;
 	while (--depth) {
-		bh = sb_bread(sb, le32_to_cpu(p->key));
-		if (!bh)
-			goto failure;
-		if (ext4_check_indirect_blockref(inode, bh))
+		bh = sb_getblk(sb, le32_to_cpu(p->key));
+		if (unlikely(!bh))
 			goto failure;
                   
+		if (!bh_uptodate_or_lock(bh)) {
+			if (bh_submit_read(bh) < 0) {
+				put_bh(bh);
+				goto failure;
+			}
+			/* validate the extent entries */
+			if (ext4_check_indirect_blockref(inode, bh))
+				goto failure;
+		}
+		
 		add_chain(++p, bh, (__le32 *)bh->b_data + *++offsets);
 		/* Reader: end */
 		if (!p->key)

Re: [PATCH] ext4: check block references only when read from disk

[Thiemo Nagel]

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

Oops.  Fixed comment.

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

[-- Attachment #2: check-blockrefs-only-when-read-from-disk_vs_2.6.29-rc7.patch --]
[-- Type: text/x-patch, Size: 791 bytes --]

--- download/linux-2.6.29-rc7-vanilla-extcheck/fs/ext4/inode.c	2009-03-12 16:44:19.000000000 +0100
+++ linux-2.6.29-rc7/fs/ext4/inode.c	2009-03-12 16:40:28.000000000 +0100
@@ -443,12 +443,20 @@
 	if (!p->key)
 		goto no_block;
 	while (--depth) {
-		bh = sb_bread(sb, le32_to_cpu(p->key));
-		if (!bh)
-			goto failure;
-		if (ext4_check_indirect_blockref(inode, bh))
+		bh = sb_getblk(sb, le32_to_cpu(p->key));
+		if (unlikely(!bh))
 			goto failure;
                   
+		if (!bh_uptodate_or_lock(bh)) {
+			if (bh_submit_read(bh) < 0) {
+				put_bh(bh);
+				goto failure;
+			}
+			/* validate block references */
+			if (ext4_check_indirect_blockref(inode, bh))
+				goto failure;
+		}
+		
 		add_chain(++p, bh, (__le32 *)bh->b_data + *++offsets);
 		/* Reader: end */
 		if (!p->key)

[PATCH v2] ext4: add checks of block references for non-extent inodes

[Thiemo Nagel]

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

Dear Ted,

I'm sending an improved patch as I've come to the conclusion that the 
previous patch is too lenient in two ways:
* off-by-one in the check of the upper block limit
* it shouldn't stop when encountering a reference to block number zero 
because, if I'm not mistaken, references behind it still might be 
accessed in sparse files / when seeking behind the end of a file.

On the other hand, I decided to drop the check against 
s_first_data_block at the low end to improve performance, since the 
purpose of the patch is to prevent access to blocks outside the 
filesystem, and not to do the best-possible consistency check against 
indirect blocks, which probably is better done in fsck.

Anyways, in case you would be interested in having more checks here (eg. 
as a compile-time option), I have available a more sophisticated patch 
which also checks for non-zero block references behind the end of the file.

Kind regards,

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



[-- Attachment #2: add-blockref-checks.patch2 --]
[-- Type: text/plain, Size: 1670 bytes --]

--- linux-2.6.29-rc7/fs/ext4/inode.c.orig	2009-03-20 11:35:45.000000000 +0100
+++ linux-2.6.29-rc7/fs/ext4/inode.c	2009-03-20 13:48:25.000000000 +0100
@@ -371,6 +371,34 @@
 	return n;
 }
 
+static int __ext4_check_blockref(const char *function, struct inode *inode,
+				 unsigned int *p, unsigned int max) {
+
+	unsigned int maxblocks = ext4_blocks_count(EXT4_SB(inode->i_sb)->s_es);
+	unsigned int *bref = p;
+	while (bref < p+max) {
+		if (unlikely(*bref >= maxblocks)) {
+			ext4_error(inode->i_sb, function,
+				   "block reference %u >= max (%u) "
+				   "in inode #%lu, offset=%u",
+				   *bref, maxblocks,
+				   inode->i_ino, bref-p);
+ 			return -EIO;
+ 		}
+		bref++;
+ 	}
+ 	return 0;
+}
+
+
+#define ext4_check_indirect_blockref(inode, bh)                         \
+        __ext4_check_blockref(__func__, inode, (__le32 *)(bh)->b_data,  \
+			      EXT4_ADDR_PER_BLOCK((inode)->i_sb))
+
+#define ext4_check_inode_blockref(inode)                                \
+        __ext4_check_blockref(__func__, inode, EXT4_I(inode)->i_data,   \
+			      EXT4_NDIR_BLOCKS)
+
 /**
  *	ext4_get_branch - read the chain of indirect blocks leading to data
  *	@inode: inode in question
@@ -418,6 +446,9 @@
 		bh = sb_bread(sb, le32_to_cpu(p->key));
 		if (!bh)
 			goto failure;
+		if (ext4_check_indirect_blockref(inode, bh))
+			goto failure;
+                  
 		add_chain(++p, bh, (__le32 *)bh->b_data + *++offsets);
 		/* Reader: end */
 		if (!p->key)
@@ -4302,11 +4333,13 @@
 	if (ei->i_flags & EXT4_EXTENTS_FL) {
 		/* Validate extent which is part of inode */
 		ret = ext4_ext_check_inode(inode);
-		if (ret) {
-			brelse(bh);
-			goto bad_inode;
-		}

Re: [PATCH v2] ext4: add checks of block references for non-extent inodes

[Theodore Tso]

Thanks, I've combined these your two patches ("add checks of block
references for non-extent inodes" and "check block references only
when read from disk") into a single patch and added it to the ext4
patch queue.

						- Ted

Re: [PATCH v2] ext4: add checks of block references for non-extent inodes

[Theodore Tso]

On Fri, Mar 20, 2009 at 03:15:56PM +0100, Thiemo Nagel wrote:
> Dear Ted,
>
> I'm sending an improved patch as I've come to the conclusion that the  
> previous patch is too lenient in two ways:
> * off-by-one in the check of the upper block limit
> * it shouldn't stop when encountering a reference to block number zero  
> because, if I'm not mistaken, references behind it still might be  
> accessed in sparse files / when seeking behind the end of a file.

Note: this patch has a rather fatal flaw; it doens't check to make
sure that file is a regular file or directory first.  If you have a
fast symlink, ext4_iget() will blow up since it's a non-extent file,
but the i_blocks[] will contain an ASCII string.

I'll fix up your patch to deal add the appropriate checking.

          	    	       	       	   - Ted

Re: [PATCH v2] ext4: add checks of block references for non-extent inodes

[Aneesh Kumar K.V]

On Fri, Mar 20, 2009 at 03:15:56PM +0100, Thiemo Nagel wrote:
> Dear Ted,
>
> I'm sending an improved patch as I've come to the conclusion that the  
> previous patch is too lenient in two ways:
> * off-by-one in the check of the upper block limit
> * it shouldn't stop when encountering a reference to block number zero  
> because, if I'm not mistaken, references behind it still might be  
> accessed in sparse files / when seeking behind the end of a file.
>
> On the other hand, I decided to drop the check against  
> s_first_data_block at the low end to improve performance, since the  
> purpose of the patch is to prevent access to blocks outside the  
> filesystem, and not to do the best-possible consistency check against  
> indirect blocks, which probably is better done in fsck.
>
> Anyways, in case you would be interested in having more checks here (eg.  
> as a compile-time option), I have available a more sophisticated patch  
> which also checks for non-zero block references behind the end of the 
> file.
>
> Kind regards,
>
> Signed-off-by: Thiemo Nagel <thiemo.nagel@ph.tum.de>
>
>

> --- linux-2.6.29-rc7/fs/ext4/inode.c.orig	2009-03-20 11:35:45.000000000 +0100
> +++ linux-2.6.29-rc7/fs/ext4/inode.c	2009-03-20 13:48:25.000000000 +0100
> @@ -371,6 +371,34 @@
>  	return n;
>  }
> 
> +static int __ext4_check_blockref(const char *function, struct inode *inode,
> +				 unsigned int *p, unsigned int max) {
> +
> +	unsigned int maxblocks = ext4_blocks_count(EXT4_SB(inode->i_sb)->s_es);
> +	unsigned int *bref = p;
> +	while (bref < p+max) {
> +		if (unlikely(*bref >= maxblocks)) {
> +			ext4_error(inode->i_sb, function,
> +				   "block reference %u >= max (%u) "
> +				   "in inode #%lu, offset=%u",
> +				   *bref, maxblocks,
> +				   inode->i_ino, bref-p);
> + 			return -EIO;
> + 		}
> +		bref++;
> + 	}
> + 	return 0;
> +}
> +
> +
> +#define ext4_check_indirect_blockref(inode, bh)                         \
> +        __ext4_check_blockref(__func__, inode, (__le32 *)(bh)->b_data,  \
> +			      EXT4_ADDR_PER_BLOCK((inode)->i_sb))
> +
> +#define ext4_check_inode_blockref(inode)                                \
> +        __ext4_check_blockref(__func__, inode, EXT4_I(inode)->i_data,   \
> +			      EXT4_NDIR_BLOCKS)
> +
>  /**
>   *	ext4_get_branch - read the chain of indirect blocks leading to data
>   *	@inode: inode in question
> @@ -418,6 +446,9 @@
>  		bh = sb_bread(sb, le32_to_cpu(p->key));
>  		if (!bh)
>  			goto failure;
> +		if (ext4_check_indirect_blockref(inode, bh))
> +			goto failure;
> +                  


Since on errors=continue we are not adding the bh to the chain. We leak
a buffer_head reference here. I guess we need a put_bh before goto
failure.



-aneesh

Re: [PATCH v2] ext4: add checks of block references for non-extent inodes

[Thiemo Nagel]

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

Dear Ted, Aneesh, Stephen,

thanks a lot for your comments and fixes!  The attached patch (applied 
on-top) should fix the
* buffer head leak spotted by Aneesh
* warning reported by Stephen

Kind regards,

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




[-- Attachment #2: fixup-blockref-checks.patch --]
[-- Type: text/x-patch, Size: 894 bytes --]

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 9ab845e..69715e5 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -380,9 +380,9 @@ static int __ext4_check_blockref(const char *function, struct inode *inode,
 		if (unlikely(*bref >= maxblocks)) {
 			ext4_error(inode->i_sb, function,
 				   "block reference %u >= max (%u) "
-				   "in inode #%lu, offset=%u",
+				   "in inode #%lu, offset=%i",
 				   *bref, maxblocks,
-				   inode->i_ino, bref-p);
+				   inode->i_ino, (int)(bref-p));
  			return -EIO;
  		}
 		bref++;
@@ -453,8 +453,10 @@ static Indirect *ext4_get_branch(struct inode *inode, int depth,
 				goto failure;
 			}
 			/* validate block references */
-			if (ext4_check_indirect_blockref(inode, bh))
+			if (ext4_check_indirect_blockref(inode, bh)) {
+				put_bh(bh);
 				goto failure;
+			}
 		}
 		
 		add_chain(++p, bh, (__le32 *)bh->b_data + *++offsets);

Re: [PATCH v2] ext4: add checks of block references for non-extent inodes

[Theodore Tso]

On Tue, Mar 31, 2009 at 10:41:27AM +0200, Thiemo Nagel wrote:
> Dear Ted, Aneesh, Stephen,
>
> thanks a lot for your comments and fixes!  The attached patch (applied  
> on-top) should fix the
> * buffer head leak spotted by Aneesh
> * warning reported by Stephen
>
> Kind regards,

Thanks, I've merged this into your patch in the ext4 patch queue.

	     	    	      	   	    - Ted

Re: [PATCH v2] ext4: add checks of block references for non-extent inodes

[Thiemo Nagel]

Theodore Tso wrote:
> On Tue, Mar 31, 2009 at 10:41:27AM +0200, Thiemo Nagel wrote:
>> Dear Ted, Aneesh, Stephen,
>>
>> thanks a lot for your comments and fixes!  The attached patch (applied  
>> on-top) should fix the
>> * buffer head leak spotted by Aneesh
>> * warning reported by Stephen
> 
> Thanks, I've merged this into your patch in the ext4 patch queue.

Thank you!

BTW:  Do you want this patch also for ext2/3 ?

Kind regards,

Thiemo