[RHEL5.11 PATCH 2/3] ext3: validate directory entry data before use

[RHEL5.11 PATCH 2/3] ext3: validate directory entry data before use

[Lukas Czerner]

From: Duane Griffin <duaneg@dghda.com>

BZ 1065434
https://bugzilla.redhat.com/show_bug.cgi?id=1065434

BREW 7074551
https://brewweb.devel.redhat.com/taskinfo?taskID=7074551

Upstream commit 275c0a8f1253a7542ad9726956c918d8a1f694c4

ext3_dx_find_entry uses ext3_next_entry without verifying that the entry
is valid.  If its rec_len == 0 this causes an infinite loop.  Refactor the
loop to check the validity of entries before checking whether they match
and moving onto the next one.

There are other uses of ext3_next_entry in this file which also look
problematic.  They should be reviewed and fixed if/when we have a
test-case that triggers them.

This patch fixes the first case (image hdb.25.softlockup.gz) reported in
http://bugzilla.kernel.org/show_bug.cgi?id=10882.

Signed-off-by: Duane Griffin <duaneg@dghda.com>
Cc: <linux-ext4@vger.kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 fs/ext3/namei.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/fs/ext3/namei.c b/fs/ext3/namei.c
index 8bc0198..69c0540 100644
--- a/fs/ext3/namei.c
+++ b/fs/ext3/namei.c
@@ -994,19 +994,21 @@ static struct buffer_head * ext3_dx_find_entry(struct dentry *dentry,
 		de = (struct ext3_dir_entry_2 *) bh->b_data;
 		top = (struct ext3_dir_entry_2 *) ((char *) de + sb->s_blocksize -
 				       EXT3_DIR_REC_LEN(0));
-		for (; de < top; de = ext3_next_entry(de))
-		if (ext3_match (namelen, name, de)) {
-			if (!ext3_check_dir_entry("ext3_find_entry",
-						  dir, de, bh,
-				  (block<<EXT3_BLOCK_SIZE_BITS(sb))
-					  +((char *)de - bh->b_data))) {
-				brelse (bh);
+		for (; de < top; de = ext3_next_entry(de)) {
+			int off = (block << EXT3_BLOCK_SIZE_BITS(sb))
+				  + ((char *) de - bh->b_data);
+
+			if (!ext3_check_dir_entry(__func__, dir, de, bh, off)) {
+				brelse(bh);
 				*err = ERR_BAD_DX_DIR;
 				goto errout;
 			}
-			*res_dir = de;
-			dx_release (frames);
-			return bh;
+
+			if (ext3_match(namelen, name, de)) {
+				*res_dir = de;
+				dx_release(frames);
+				return bh;
+			}
 		}
 		brelse (bh);
 		/* Check to see if we should continue to search */
-- 
1.8.3.1

Re: [RHEL5.11 PATCH 2/3] ext3: validate directory entry data before use

[Andreas Dilger]

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

On Feb 19, 2014, at 12:56 AM, Lukas Czerner <lczerner@redhat.com> wrote:
> ext3_dx_find_entry uses ext3_next_entry without verifying that the entry
> is valid.  If its rec_len == 0 this causes an infinite loop.  Refactor the
> loop to check the validity of entries before checking whether they match
> and moving onto the next one.
> 
> There are other uses of ext3_next_entry in this file which also look
> problematic.  They should be reviewed and fixed if/when we have a
> test-case that triggers them.

Checking the validity on every access can be CPU expensive.  In ext2
(and I thought ext3/ext4?) this was checked once when the block was
first read from disk and then the buffer was marked verified and not
checked again.  That ensures the contents are valid, but avoids the
potential O(n^2) CPU overhead of validating the same data for each call.

Taking a quick look at the ext4_dx_find_entry->__ext4_read_dirblock()
it is marking the buffer verified if the checksum is validated, but
I can't immediately see where it is verifying the contents?  Maybe that
was lost during the patch to add checksum support, or maybe I'm just
mis-remembering which ext2/3/4 variants this was done for.

Cheers, Andreas

> Signed-off-by: Duane Griffin <duaneg@dghda.com>
> Cc: <linux-ext4@vger.kernel.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> ---
> fs/ext3/namei.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/ext3/namei.c b/fs/ext3/namei.c
> index 8bc0198..69c0540 100644
> --- a/fs/ext3/namei.c
> +++ b/fs/ext3/namei.c
> @@ -994,19 +994,21 @@ static struct buffer_head * ext3_dx_find_entry(struct dentry *dentry,
> 		de = (struct ext3_dir_entry_2 *) bh->b_data;
> 		top = (struct ext3_dir_entry_2 *) ((char *) de + sb->s_blocksize -
> 				       EXT3_DIR_REC_LEN(0));
> -		for (; de < top; de = ext3_next_entry(de))
> -		if (ext3_match (namelen, name, de)) {
> -			if (!ext3_check_dir_entry("ext3_find_entry",
> -						  dir, de, bh,
> -				  (block<<EXT3_BLOCK_SIZE_BITS(sb))
> -					  +((char *)de - bh->b_data))) {
> -				brelse (bh);
> +		for (; de < top; de = ext3_next_entry(de)) {
> +			int off = (block << EXT3_BLOCK_SIZE_BITS(sb))
> +				  + ((char *) de - bh->b_data);
> +
> +			if (!ext3_check_dir_entry(__func__, dir, de, bh, off)) {
> +				brelse(bh);
> 				*err = ERR_BAD_DX_DIR;
> 				goto errout;
> 			}
> -			*res_dir = de;
> -			dx_release (frames);
> -			return bh;
> +
> +			if (ext3_match(namelen, name, de)) {
> +				*res_dir = de;
> +				dx_release(frames);
> +				return bh;
> +			}
> 		}
> 		brelse (bh);
> 		/* Check to see if we should continue to search */
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]