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
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 <[email protected]>
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <[email protected]>
> 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
On 10/13/2010 10:56 PM, ext Andreas Dilger wrote:
>> that was introduced with:
>> >
>> > commit acfa1823d33859b0db77701726c9ca5ccc6e6f25
>> > Author: Andreas Dilger<[email protected]>
>> > 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
Roman Borisov wrote:
> On 10/13/2010 10:56 PM, ext Andreas Dilger wrote:
>>> that was introduced with:
>>> >
>>> > commit acfa1823d33859b0db77701726c9ca5ccc6e6f25
>>> > Author: Andreas Dilger<[email protected]>
>>> > 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
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
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" <[email protected]>
Cc: Jan Kara <[email protected]>
Cc: Brad Spengler <[email protected]>
---
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
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" <[email protected]>
Cc: Jan Kara <[email protected]>
Cc: Brad Spengler <[email protected]>
---
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
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" <[email protected]>
Cc: Brad Spengler <[email protected]>
---
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
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" <[email protected]>
Cc: Brad Spengler <[email protected]>
---
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
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
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 <[email protected]>
SUSE Labs, CR
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