ext3: ext4: Using uninitialized value

ext3: ext4: Using uninitialized value

[Roman Borisov]

Hello,

Could you clarify is there a bug in fs/ext4/namei.c,
ext4_dx_find_entry() and fs/ext4/namei.c, ext3_dx_find_entry()?

<code>
static struct buffer_head * ext3_dx_find_entry(struct inode *dir,
...
struct dx_hash_info hinfo;
...
if (namelen > 2 || name[0] != '.'|| (namelen == 2 && name[1] != '.')) {
if (!(frame = dx_probe(entry, dir, &hinfo, frames, err)))
return NULL;
} else {
frame = frames;
frame->bh = NULL; /* for dx_release() */
frame->at = (struct dx_entry *)frames; /* hack for zero entry*/
dx_set_block(frame->at, 0); /* dx_root block is 0 */
}
hash = hinfo.hash;
...
retval = ext3_htree_next_block(dir, hash, frame,
...
</code>

In the code above: hinfo.hash is not initialized in "else" case.
Should it be initialized as NULL?
Or maybe implementation doesn't assume to call ext3_htree_next_block()
in such case?

Thanks,
Roman

Re: ext3: ext4: Using uninitialized value

[Eric Sandeen]

On 10/13/2010 09:40 AM, Roman Borisov wrote:
> Hello,
> 
> Could you clarify is there a bug in fs/ext4/namei.c,
> ext4_dx_find_entry() and fs/ext4/namei.c, ext3_dx_find_entry()?

that was introduced with:

commit acfa1823d33859b0db77701726c9ca5ccc6e6f25
Author: Andreas Dilger <adilger@clusterfs.com>
Date:   Thu Jun 23 00:09:45 2005 -0700

    [PATCH] Support for dx directories in ext3_get_parent (NFSD)

so maybe Andreas knows offhand ;)  but I think:

> <code>
> static struct buffer_head * ext3_dx_find_entry(struct inode *dir,
> ...
>         if (namelen > 2 || name[0] != '.'|| (namelen == 2 && name[1] != '.')) {

This is a fancy way of saying name is not "." or ".."

>                 if (!(frame = dx_probe(entry, dir, &hinfo, frames, err)))
>                         return NULL;
>         } else {

so here it -is- "." or ".." -

>                 frame = frames;
>                 frame->bh = NULL;                       /* for dx_release() */
>                 frame->at = (struct dx_entry *)frames;  /* hack for zero entry*/
>                 dx_set_block(frame->at, 0);             /* dx_root block is 0 */

now frame->at.block = 0

>         }
>         hash = hinfo.hash;
>         do {
>                 block = dx_get_block(frame->at);

block = 0 (we just put it there)

>                 if (!(bh = ext3_bread (NULL,dir, block, 0, err)))
>                         goto errout;

so we look up block 0 in the dir inode

>                 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));

and get to the dir entry, and search through them for our name

>                 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;
>                         }
> 
>                         if (ext3_match(namelen, name, de)) {

here we should find the . or .. (it's always going to be there, right?)

>                                 *res_dir = de;
>                                 dx_release(frames);
>                                 return bh;

so we return

>                         }
>                 }

before we get here:

> ...
>                 retval = ext3_htree_next_block(dir, hash, frame,
>                                                frames, NULL);
> ...
> </code>

-Eric

> In the code above: hinfo.hash is not initialized in "else" case.
> Should it be initialized as NULL?
> Or maybe implementation doesn't assume to call ext3_htree_next_block()
> in such case?
> 
> Thanks,
> Roman
> 
> --
> 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

Re: ext3: ext4: Using uninitialized value

[Andreas Dilger]

On 2010-10-13, at 10:13, Eric Sandeen wrote:
> On 10/13/2010 09:40 AM, Roman Borisov wrote:
>> Hello,
>> 
>> Could you clarify is there a bug in fs/ext4/namei.c,
>> ext4_dx_find_entry() and fs/ext4/namei.c, ext3_dx_find_entry()?
> 
> that was introduced with:
> 
> commit acfa1823d33859b0db77701726c9ca5ccc6e6f25
> Author: Andreas Dilger <adilger@clusterfs.com>
> Date:   Thu Jun 23 00:09:45 2005 -0700
> 
>   [PATCH] Support for dx directories in ext3_get_parent (NFSD)
> 
> so maybe Andreas knows offhand ;)  but I think:

Your analysis is correct.  I agree it's a bit convoluted, but it avoids replicating a bunch of code.

>> static struct buffer_head * ext3_dx_find_entry(struct inode *dir,
>> ...
>>       if (namelen > 2 || name[0] != '.'|| (namelen == 2 && name[1] != '.'))
>>        } else {
> 
> so here it -is- "." or ".." -
> 
>>               frame = frames;
>>               frame->bh = NULL;                       /* for dx_release() */
>>               frame->at = (struct dx_entry *)frames;  /* hack for zero entry*/
>>               dx_set_block(frame->at, 0);             /* dx_root block is 0 */
>>               if (!(bh = ext3_bread (NULL,dir, block, 0, err)))
>>                       goto errout;
> 
> so we look up block 0 in the dir inode
> 
>>                       if (ext3_match(namelen, name, de)) {
> 
> here we should find the . or .. (it's always going to be there, right?)

Right - it is important to note that the index root block is a "fake" directory block which has just the "." and ".." entries at the beginning (with the ".." spanning the rest of the block), and the rest of the block is holding the index entries.  For a directory index to even exist, it HAS to have the "." and ".." entries in the first block, or there is no place to put the index.

Cheers, Andreas

Re: ext3: ext4: Using uninitialized value

[Roman Borisov]

On 10/13/2010 10:56 PM, ext Andreas Dilger wrote:
>> that was introduced with:
>> >
>> >  commit acfa1823d33859b0db77701726c9ca5ccc6e6f25
>> >  Author: Andreas Dilger<adilger@clusterfs.com>
>> >  Date:   Thu Jun 23 00:09:45 2005 -0700
>> >
>> >     [PATCH] Support for dx directories in ext3_get_parent (NFSD)
>> >
>> >  so maybe Andreas knows offhand;)   but I think:
> Your analysis is correct.  I agree it's a bit convoluted, but it avoids replicating a bunch of code.
>

Thanks a lot! it make sence.

But I just wondering why hash = hinfo->hash is located in separate scope 
where it looks like unitialized.
The same situation in namei.c/dx_probe():
	if (entry)
		ext3fs_dirhash(entry->name, entry->len, hinfo);
	hash = hinfo->hash;
I believe that the implementation doesn't allow to use hash value when 
!entry but why it wasn't designed like:
	if (entry)
	{
		ext3fs_dirhash(entry->name, entry->len, hinfo);
		hash = hinfo->hash;
	}
for example?

Roman

Re: ext3: ext4: Using uninitialized value

[Eric Sandeen]

Roman Borisov wrote:
> On 10/13/2010 10:56 PM, ext Andreas Dilger wrote:
>>> that was introduced with:
>>> >
>>> >  commit acfa1823d33859b0db77701726c9ca5ccc6e6f25
>>> >  Author: Andreas Dilger<adilger@clusterfs.com>
>>> >  Date:   Thu Jun 23 00:09:45 2005 -0700
>>> >
>>> >     [PATCH] Support for dx directories in ext3_get_parent (NFSD)
>>> >
>>> >  so maybe Andreas knows offhand;)   but I think:
>> Your analysis is correct.  I agree it's a bit convoluted, but it
>> avoids replicating a bunch of code.
>>
> 
> Thanks a lot! it make sence.
> 
> But I just wondering why hash = hinfo->hash is located in separate scope
> where it looks like unitialized.
> The same situation in namei.c/dx_probe():
>     if (entry)
>         ext3fs_dirhash(entry->name, entry->len, hinfo);
>     hash = hinfo->hash;
> I believe that the implementation doesn't allow to use hash value when
> !entry but why it wasn't designed like:
>     if (entry)
>     {
>         ext3fs_dirhash(entry->name, entry->len, hinfo);
>         hash = hinfo->hash;
>     }
> for example?

Just a guess, but gcc might start complaining then ;)  (It wasn't smart
enough to see the potential problem with the other way...)

-Eric

> Roman

Re: ext3: ext4: Using uninitialized value

[Ted Ts'o]

On Wed, Oct 13, 2010 at 11:13:55AM -0500, Eric Sandeen wrote:
> On 10/13/2010 09:40 AM, Roman Borisov wrote:
> > Hello,
> > 
> > Could you clarify is there a bug in fs/ext4/namei.c,
> > ext4_dx_find_entry() and fs/ext4/namei.c, ext3_dx_find_entry()?

Let me guess, you compiled the kernel using Clang, right?  I recently
had a similar discussion with Brad about this issue.

> here we should find the . or .. (it's always going to be there, right?)

The problem is if '.' or '..' is not present, then when we call
ext3_htree_next_block(), it's not the fact that hash is uninitialized
which is the problem.  The bigger problem is that the frame structure
is not fully initialized; and we could end up dereference a NULL
pointer (on a 32-bit system) or a pointer filled with stack garbage
(on a 64-bit system).

Fortunately most of the time we'll never hit this case, since '.'  and
'..' handling is dealt with in fs/namei.c, and is never passed down to
the fs-specific layer.  The one exception to this is if the file
system is being used as an NFS file server, in which case it is
possible that the fs layer will be asked to look up "..".

So in the case where the directory is corrupted, and the file system
is being exported via NFS, it's possible that we could get a kernel
Oops.  Since we're only reading from the garbaged pointer, I'm pretty
sure this can't be leveraged into a security exposure, but still, it
would be good to fix this.

I have patches chained to this e-mail that should be applied for
ext3, and which I've queued for ext4.

						- Ted

[PATCH 1/2] ext3: Avoid uninitialized memory references with a corrupted htree directory

[Theodore Ts'o]

If the first htree directory is missing '.' or '..' but is otherwise a
valid directory, and we do a lookup for '.' or '..', it's possible to
dereference an uninitialized memory pointer in ext3_htree_next_block().
Avoid this.

We avoid this by moving the special case from ext3_dx_find_entry() to
ext3_find_entry(); this also means we can optimize ext3_find_entry()
slightly when NFS looks up "..".

Thanks to Brad Spengler for pointing a Clang warning that led me to
look more closely at this code.  The warning was harmless, but it was
useful in pointing out code that was too ugly to live.  This warning was
also reported by Roman Borisov.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Cc: Jan Kara <jack@suse.cz>
Cc: Brad Spengler <spender@grsecurity.net>
---
 fs/ext3/namei.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/fs/ext3/namei.c b/fs/ext3/namei.c
index 2b35ddb..01f12cc 100644
--- a/fs/ext3/namei.c
+++ b/fs/ext3/namei.c
@@ -858,6 +858,7 @@ static struct buffer_head *ext3_find_entry(struct inode *dir,
 	struct buffer_head * bh_use[NAMEI_RA_SIZE];
 	struct buffer_head * bh, *ret = NULL;
 	unsigned long start, block, b;
+	const u8 *name = entry->name;
 	int ra_max = 0;		/* Number of bh's in the readahead
 				   buffer, bh_use[] */
 	int ra_ptr = 0;		/* Current index into readahead
@@ -871,6 +872,16 @@ static struct buffer_head *ext3_find_entry(struct inode *dir,
 	namelen = entry->len;
 	if (namelen > EXT3_NAME_LEN)
 		return NULL;
+	if ((namelen < 2) && (name[0] == '.') &&
+	    (name[1] == '.' || name[1] == '0')) {
+		/*
+		 * "." or ".." will only be in the first block
+		 * NFS may look up ".."; "." should be handled by the VFS
+		 */
+		block = start = 0;
+		nblocks = 1;
+		goto restart;
+	}
 	if (is_dx(dir)) {
 		bh = ext3_dx_find_entry(dir, entry, res_dir, &err);
 		/*
@@ -961,9 +972,8 @@ static struct buffer_head * ext3_dx_find_entry(struct inode *dir,
 			struct qstr *entry, struct ext3_dir_entry_2 **res_dir,
 			int *err)
 {
-	struct super_block * sb;
+	struct super_block *sb = dir->i_sb;
 	struct dx_hash_info	hinfo;
-	u32 hash;
 	struct dx_frame frames[2], *frame;
 	struct ext3_dir_entry_2 *de, *top;
 	struct buffer_head *bh;
@@ -972,18 +982,8 @@ static struct buffer_head * ext3_dx_find_entry(struct inode *dir,
 	int namelen = entry->len;
 	const u8 *name = entry->name;
 
-	sb = dir->i_sb;
-	/* NFS may look up ".." - look at dx_root directory block */
-	if (namelen > 2 || name[0] != '.'|| (namelen == 2 && name[1] != '.')) {
-		if (!(frame = dx_probe(entry, dir, &hinfo, frames, err)))
-			return NULL;
-	} else {
-		frame = frames;
-		frame->bh = NULL;			/* for dx_release() */
-		frame->at = (struct dx_entry *)frames;	/* hack for zero entry*/
-		dx_set_block(frame->at, 0);		/* dx_root block is 0 */
-	}
-	hash = hinfo.hash;
+	if (!(frame = dx_probe(entry, dir, &hinfo, frames, err)))
+		return NULL;
 	do {
 		block = dx_get_block(frame->at);
 		if (!(bh = ext3_bread (NULL,dir, block, 0, err)))
@@ -1009,7 +1009,7 @@ static struct buffer_head * ext3_dx_find_entry(struct inode *dir,
 		}
 		brelse (bh);
 		/* Check to see if we should continue to search */
-		retval = ext3_htree_next_block(dir, hash, frame,
+		retval = ext3_htree_next_block(dir, hinfo.hash, frame,
 					       frames, NULL);
 		if (retval < 0) {
 			ext3_warning(sb, __func__,
-- 
1.7.1

[PATCH 2/2] ext3: Use search_dirblock() in ext3_dx_find_entry()

[Theodore Ts'o]

Use the search_dirblock() in ext3_dx_find_entry().  It makes the code
easier to read, and it takes advantage of common code.  It also saves
100 bytes or so of text space.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Cc: Jan Kara <jack@suse.cz>
Cc: Brad Spengler <spender@grsecurity.net>
---
 fs/ext3/namei.c |   33 ++++++++++++---------------------
 1 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/fs/ext3/namei.c b/fs/ext3/namei.c
index 01f12cc..24e15dd 100644
--- a/fs/ext3/namei.c
+++ b/fs/ext3/namei.c
@@ -975,12 +975,9 @@ static struct buffer_head * ext3_dx_find_entry(struct inode *dir,
 	struct super_block *sb = dir->i_sb;
 	struct dx_hash_info	hinfo;
 	struct dx_frame frames[2], *frame;
-	struct ext3_dir_entry_2 *de, *top;
 	struct buffer_head *bh;
 	unsigned long block;
 	int retval;
-	int namelen = entry->len;
-	const u8 *name = entry->name;
 
 	if (!(frame = dx_probe(entry, dir, &hinfo, frames, err)))
 		return NULL;
@@ -988,26 +985,20 @@ static struct buffer_head * ext3_dx_find_entry(struct inode *dir,
 		block = dx_get_block(frame->at);
 		if (!(bh = ext3_bread (NULL,dir, block, 0, err)))
 			goto errout;
-		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)) {
-			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;
-			}
 
-			if (ext3_match(namelen, name, de)) {
-				*res_dir = de;
-				dx_release(frames);
-				return bh;
-			}
+		retval = search_dirblock(bh, dir, entry,
+					 block << EXT3_BLOCK_SIZE_BITS(sb),
+					 res_dir);
+		if (retval == 1) {
+			dx_release(frames);
+			return bh;
 		}
-		brelse (bh);
+		brelse(bh);
+		if (retval == -1) {
+			*err = ERR_BAD_DX_DIR;
+			goto errout;
+		}
+			
 		/* Check to see if we should continue to search */
 		retval = ext3_htree_next_block(dir, hinfo.hash, frame,
 					       frames, NULL);
-- 
1.7.1

[PATCH 1/2] ext4: Avoid uninitialized memory references in ext3_htree_next_block()

[Theodore Ts'o]

If the first block of htree directory is missing '.' or '..' but is
otherwise a valid directory, and we do a lookup for '.' or '..', it's
possible to dereference an uninitialized memory pointer in
ext4_htree_next_block().

We avoid this by moving the special case from ext4_dx_find_entry() to
ext4_find_entry(); this also means we can optimize ext4_find_entry()
slightly when NFS looks up "..".

Thanks to Brad Spengler for pointing a Clang warning that led me to
look more closely at this code.  The warning was harmless, but it was
useful in pointing out code that was too ugly to live.  This warning was
also reported by Roman Borisov.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Cc: Brad Spengler <spender@grsecurity.net>
---
 fs/ext4/namei.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 314c0d3..ff89449 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -856,6 +856,7 @@ static struct buffer_head * ext4_find_entry (struct inode *dir,
 	struct buffer_head *bh_use[NAMEI_RA_SIZE];
 	struct buffer_head *bh, *ret = NULL;
 	ext4_lblk_t start, block, b;
+	const u8 *name = d_name->name;
 	int ra_max = 0;		/* Number of bh's in the readahead
 				   buffer, bh_use[] */
 	int ra_ptr = 0;		/* Current index into readahead
@@ -870,6 +871,16 @@ static struct buffer_head * ext4_find_entry (struct inode *dir,
 	namelen = d_name->len;
 	if (namelen > EXT4_NAME_LEN)
 		return NULL;
+	if ((namelen < 2) && (name[0] == '.') &&
+	    (name[1] == '.' || name[1] == '0')) {
+		/*
+		 * "." or ".." will only be in the first block
+		 * NFS may look up ".."; "." should be handled by the VFS
+		 */
+		block = start = 0;
+		nblocks = 1;
+		goto restart;
+	}
 	if (is_dx(dir)) {
 		bh = ext4_dx_find_entry(dir, d_name, res_dir, &err);
 		/*
@@ -960,9 +971,8 @@ cleanup_and_exit:
 static struct buffer_head * ext4_dx_find_entry(struct inode *dir, const struct qstr *d_name,
 		       struct ext4_dir_entry_2 **res_dir, int *err)
 {
-	struct super_block * sb;
+	struct super_block * sb = dir->i_sb;
 	struct dx_hash_info	hinfo;
-	u32 hash;
 	struct dx_frame frames[2], *frame;
 	struct ext4_dir_entry_2 *de, *top;
 	struct buffer_head *bh;
@@ -971,18 +981,8 @@ static struct buffer_head * ext4_dx_find_entry(struct inode *dir, const struct q
 	int namelen = d_name->len;
 	const u8 *name = d_name->name;
 
-	sb = dir->i_sb;
-	/* NFS may look up ".." - look at dx_root directory block */
-	if (namelen > 2 || name[0] != '.'||(name[1] != '.' && name[1] != '\0')){
-		if (!(frame = dx_probe(d_name, dir, &hinfo, frames, err)))
-			return NULL;
-	} else {
-		frame = frames;
-		frame->bh = NULL;			/* for dx_release() */
-		frame->at = (struct dx_entry *)frames;	/* hack for zero entry*/
-		dx_set_block(frame->at, 0);		/* dx_root block is 0 */
-	}
-	hash = hinfo.hash;
+	if (!(frame = dx_probe(d_name, dir, &hinfo, frames, err)))
+		return NULL;
 	do {
 		block = dx_get_block(frame->at);
 		if (!(bh = ext4_bread (NULL,dir, block, 0, err)))
@@ -1008,7 +1008,7 @@ static struct buffer_head * ext4_dx_find_entry(struct inode *dir, const struct q
 		}
 		brelse(bh);
 		/* Check to see if we should continue to search */
-		retval = ext4_htree_next_block(dir, hash, frame,
+		retval = ext4_htree_next_block(dir, hinfo.hash, frame,
 					       frames, NULL);
 		if (retval < 0) {
 			ext4_warning(sb,
-- 
1.7.1

[PATCH 2/2] ext4: Use search_dirblock() in ext4_dx_find_entry()

[Theodore Ts'o]

Use the search_dirblock() in ext4_dx_find_entry().  It makes the code
easier to read, and it takes advantage of common code.  It also saves
100 bytes or so of text space.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Cc: Brad Spengler <spender@grsecurity.net>
---
 fs/ext4/namei.c |   33 ++++++++++++---------------------
 1 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index ff89449..eb8b794 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -974,39 +974,30 @@ static struct buffer_head * ext4_dx_find_entry(struct inode *dir, const struct q
 	struct super_block * sb = dir->i_sb;
 	struct dx_hash_info	hinfo;
 	struct dx_frame frames[2], *frame;
-	struct ext4_dir_entry_2 *de, *top;
 	struct buffer_head *bh;
 	ext4_lblk_t block;
 	int retval;
-	int namelen = d_name->len;
-	const u8 *name = d_name->name;
 
 	if (!(frame = dx_probe(d_name, dir, &hinfo, frames, err)))
 		return NULL;
 	do {
 		block = dx_get_block(frame->at);
-		if (!(bh = ext4_bread (NULL,dir, block, 0, err)))
+		if (!(bh = ext4_bread(NULL, dir, block, 0, err)))
 			goto errout;
-		de = (struct ext4_dir_entry_2 *) bh->b_data;
-		top = (struct ext4_dir_entry_2 *) ((char *) de + sb->s_blocksize -
-				       EXT4_DIR_REC_LEN(0));
-		for (; de < top; de = ext4_next_entry(de, sb->s_blocksize)) {
-			int off = (block << EXT4_BLOCK_SIZE_BITS(sb))
-				  + ((char *) de - bh->b_data);
-
-			if (!ext4_check_dir_entry(dir, de, bh, off)) {
-				brelse(bh);
-				*err = ERR_BAD_DX_DIR;
-				goto errout;
-			}
 
-			if (ext4_match(namelen, name, de)) {
-				*res_dir = de;
-				dx_release(frames);
-				return bh;
-			}
+		retval = search_dirblock(bh, dir, d_name,
+					 block << EXT4_BLOCK_SIZE_BITS(sb),
+					 res_dir);
+		if (retval == 1) { 	/* Success! */
+			dx_release(frames);
+			return bh;
 		}
 		brelse(bh);
+		if (retval == -1) {
+			*err = ERR_BAD_DX_DIR;
+			goto errout;
+		}
+
 		/* Check to see if we should continue to search */
 		retval = ext4_htree_next_block(dir, hinfo.hash, frame,
 					       frames, NULL);
-- 
1.7.1

Re: [PATCH 1/2] ext4: Avoid uninitialized memory references in ext3_htree_next_block()

[Andreas Dilger]

On 2010-10-16, at 17:37, Theodore Ts'o wrote:
> @@ -870,6 +871,16 @@ static struct buffer_head * ext4_find_entry (struct 
> +	if ((namelen < 2) && (name[0] == '.') &&
> +	    (name[1] == '.' || name[1] == '0')) {

Shouldn't this be "namelen <= 2"?  Otherwise it won't actually match "..".

Cheers, Andreas

Re: [PATCH 1/2] ext3: Avoid uninitialized memory references with a corrupted htree directory

[Jan Kara]

  Hi Ted,

  thanks for the patch.

On Sat 16-10-10 19:36:59, Theodore Ts'o wrote:
> @@ -871,6 +872,16 @@ static struct buffer_head *ext3_find_entry(struct inode *dir,
>  	namelen = entry->len;
>  	if (namelen > EXT3_NAME_LEN)
>  		return NULL;
> +	if ((namelen < 2) && (name[0] == '.') &&
> +	    (name[1] == '.' || name[1] == '0')) {
  This condition looks wrong... I suspect it should rather be:
  (namelen <= 2) && (name[0] == '.') && (name[1] == '.' || name[1] == 0)
           ^^^ change here                                  and here ^^^
> +		/*
> +		 * "." or ".." will only be in the first block
> +		 * NFS may look up ".."; "." should be handled by the VFS
> +		 */
> +		block = start = 0;
> +		nblocks = 1;
> +		goto restart;
> +	}

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 1/2] ext3: Avoid uninitialized memory references with a corrupted htree directory

[Andreas Dilger]

On 2010-10-18, at 04:05, Jan Kara wrote:
> On Sat 16-10-10 19:36:59, Theodore Ts'o wrote:
>> @@ -871,6 +872,16 @@ static struct buffer_head *ext3_find_entry(struct inode *dir,
>> 	namelen = entry->len;
>> 	if (namelen > EXT3_NAME_LEN)
>> 		return NULL;
>> +	if ((namelen < 2) && (name[0] == '.') &&
>> +	    (name[1] == '.' || name[1] == '0')) {
> 
> This condition looks wrong... I suspect it should rather be:
> (namelen <= 2) && (name[0] == '.') && (name[1] == '.' || name[1] == 0)
>      ^^^ change here                                  and here ^^^

I think it is preferable to use '\0' for the trailing NUL.

Cheers, Andreas