[PATCH, v2] ext3: validate directory entry data before use

From: Duane Griffin
Date: Sat Jun 21 2008 - 11:55:20 EST


Various places in fs/ext3/namei.c use ext3_next_entry to loop over
directory entries, but not all of them verify that entries are valid before
attempting to move to the next one. In the case where rec_len == 0 this
causes an infinite loop.

Introduce a new version of ext3_next_entry that checks the validity of the
entry before using its data to find the next one. Rename the original
function to __ext3_next_entry and use it in places where we already know
the data is valid.

Note that the changes to empty_dir follow the original code in reporting
the directory as empty in the presence of errors.

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@xxxxxxxxx>
--

This is version 2. The original patch was an earlier version causing
warnings that I sent by mistake. This version just fixes those.

Please note that I've only properly tested the originally reported failure
case (i.e. the changes to ext3_dx_find_entry).

Reviewers may want to pay particular attention to the changes to do_split,
make_indexed_dir and empty_dir. I've tried to follow the original code's
error handling conventions, as noted above for empty_dir, but I'm not sure
this is the best way to do things.

---

diff --git a/fs/ext3/namei.c b/fs/ext3/namei.c
index 0b8cf80..ea0236b 100644
--- a/fs/ext3/namei.c
+++ b/fs/ext3/namei.c
@@ -185,13 +185,27 @@ static int ext3_dx_add_entry(handle_t *handle, struct dentry *dentry,
* p is at least 6 bytes before the end of page
*/
static inline struct ext3_dir_entry_2 *
-ext3_next_entry(struct ext3_dir_entry_2 *p)
+__ext3_next_entry(struct ext3_dir_entry_2 *p)
{
return (struct ext3_dir_entry_2 *)((char *)p +
ext3_rec_len_from_disk(p->rec_len));
}

/*
+ * Checks the directory entry looks sane before using it to find the next one.
+ * Returns NULL and reports an error if an invalid entry is passed.
+ */
+static inline struct ext3_dir_entry_2 *
+ext3_next_entry(const char *func, struct inode *dir,
+ struct ext3_dir_entry_2 *de, struct buffer_head *bh, int offset)
+{
+ if (ext3_check_dir_entry(func, dir, de, bh, offset))
+ return __ext3_next_entry(de);
+ else
+ return NULL;
+}
+
+/*
* Future: use high four bits of block for coalesce-on-delete flags
* Mask them off for now.
*/
@@ -271,15 +285,17 @@ struct stats
unsigned bcount;
};

-static struct stats dx_show_leaf(struct dx_hash_info *hinfo, struct ext3_dir_entry_2 *de,
- int size, int show_names)
+static struct stats dx_show_leaf(struct dx_hash_info *hinfo, struct inode *dir,
+ struct buffer_head *bh, int size,
+ int show_names)
{
+ struct ext3_dir_entry_2 *de = (struct ext3_dir_entry_2 *) bh->b_data;
unsigned names = 0, space = 0;
char *base = (char *) de;
struct dx_hash_info h = *hinfo;

printk("names: ");
- while ((char *) de < base + size)
+ while (de && (char *) de < base + size)
{
if (de->inode)
{
@@ -295,7 +311,7 @@ static struct stats dx_show_leaf(struct dx_hash_info *hinfo, struct ext3_dir_ent
space += EXT3_DIR_REC_LEN(de->name_len);
names++;
}
- de = ext3_next_entry(de);
+ de = ext3_next_entry("dx_show_leaf", dir, de, bh, 0);
}
printk("(%i)\n", names);
return (struct stats) { names, space, 1 };
@@ -319,7 +335,7 @@ struct stats dx_show_entries(struct dx_hash_info *hinfo, struct inode *dir,
if (!(bh = ext3_bread (NULL,dir, block, 0,&err))) continue;
stats = levels?
dx_show_entries(hinfo, dir, ((struct dx_node *) bh->b_data)->entries, levels - 1):
- dx_show_leaf(hinfo, (struct ext3_dir_entry_2 *) bh->b_data, blocksize, 0);
+ dx_show_leaf(hinfo, dir, bh, blocksize, 0);
names += stats.names;
space += stats.space;
bcount += stats.bcount;
@@ -583,7 +599,7 @@ static int htree_dirblock_to_tree(struct file *dir_file,
top = (struct ext3_dir_entry_2 *) ((char *) de +
dir->i_sb->s_blocksize -
EXT3_DIR_REC_LEN(0));
- for (; de < top; de = ext3_next_entry(de)) {
+ for (; de < top; de = __ext3_next_entry(de)) {
if (!ext3_check_dir_entry("htree_dirblock_to_tree", dir, de, bh,
(block<<EXT3_BLOCK_SIZE_BITS(dir->i_sb))
+((char *)de - bh->b_data))) {
@@ -658,7 +674,12 @@ int ext3_htree_fill_tree(struct file *dir_file, __u32 start_hash,
}
if (start_hash < 2 || (start_hash ==2 && start_minor_hash==0)) {
de = (struct ext3_dir_entry_2 *) frames[0].bh->b_data;
- de = ext3_next_entry(de);
+ de = ext3_next_entry("ext3_htree_fill_tree",
+ dir, de, frames[0].bh, 0);
+ if (de == NULL) {
+ err = -EIO;
+ goto errout;
+ }
if ((err = ext3_htree_store_dirent(dir_file, 2, 0, de)) != 0)
goto errout;
count++;
@@ -726,8 +747,7 @@ static int dx_make_map (struct ext3_dir_entry_2 *de, int size,
count++;
cond_resched();
}
- /* XXX: do we need to check rec_len == 0 case? -Chris */
- de = ext3_next_entry(de);
+ de = __ext3_next_entry(de);
}
return count;
}
@@ -991,24 +1011,28 @@ 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)) {
+ for (; de < top; de = __ext3_next_entry(de)) {
+ int offset = (block << EXT3_BLOCK_SIZE_BITS(sb))
+ + ((char *) de - bh->b_data);
+
if (!ext3_check_dir_entry("ext3_find_entry",
- dir, de, bh,
- (block<<EXT3_BLOCK_SIZE_BITS(sb))
- +((char *)de - bh->b_data))) {
- brelse (bh);
+ dir, de, bh, offset)) {
+ 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 */
- retval = ext3_htree_next_block(dir, hash, frame,
- frames, NULL);
+ retval = ext3_htree_next_block(dir, hash, frame, frames, NULL);
+
if (retval < 0) {
ext3_warning(sb, __func__,
"error reading index page in directory #%lu",
@@ -1141,7 +1165,7 @@ static struct ext3_dir_entry_2* dx_pack_dirents(char *base, int size)

prev = to = de;
while ((char*)de < base + size) {
- next = ext3_next_entry(de);
+ next = __ext3_next_entry(de);
if (de->inode && de->name_len) {
rec_len = EXT3_DIR_REC_LEN(de->name_len);
if (de > to)
@@ -1172,9 +1196,22 @@ static struct ext3_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
struct dx_map_entry *map;
char *data1 = (*bh)->b_data, *data2;
unsigned split, move, size, i;
- struct ext3_dir_entry_2 *de = NULL, *de2;
+ struct ext3_dir_entry_2 *de, *de2;
int err = 0;

+ /* Verify directory entries are sane before trying anything else. */
+ de = (struct ext3_dir_entry_2 *) data1;
+ de2 = (struct ext3_dir_entry_2 *) data1 +
+ dir->i_sb->s_blocksize - EXT3_DIR_REC_LEN(0);
+ while (de < de2) {
+ de = ext3_next_entry("do_split", dir, de, *bh, 0);
+ if (de == NULL) {
+ brelse(*bh);
+ *bh = NULL;
+ goto errout;
+ }
+ }
+
bh2 = ext3_append (handle, dir, &newblock, &err);
if (!(bh2)) {
brelse(*bh);
@@ -1397,8 +1434,15 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
memcpy (data1, de, len);
de = (struct ext3_dir_entry_2 *) data1;
top = data1 + len;
- while ((char *)(de2 = ext3_next_entry(de)) < top)
+
+ while (1) {
+ de2 = ext3_next_entry("make_indexed_dir", dir, de, bh, 0);
+ if (de2 == NULL || (char *) (char *) (char *) (char *) (char *) (char *) (char *) (char *) (char *) de2 >= top) {
+ break;
+ }
de = de2;
+ }
+
de->rec_len = ext3_rec_len_to_disk(data1 + blocksize - (char *) de);
/* Initialize the root; the dot dirents already exist */
de = (struct ext3_dir_entry_2 *) (&root->dotdot);
@@ -1655,7 +1699,7 @@ static int ext3_delete_entry (handle_t *handle,
}
i += ext3_rec_len_from_disk(de->rec_len);
pde = de;
- de = ext3_next_entry(de);
+ de = __ext3_next_entry(de);
}
return -ENOENT;
}
@@ -1792,7 +1836,7 @@ retry:
de->rec_len = ext3_rec_len_to_disk(EXT3_DIR_REC_LEN(de->name_len));
strcpy (de->name, ".");
ext3_set_de_type(dir->i_sb, de, S_IFDIR);
- de = ext3_next_entry(de);
+ de = __ext3_next_entry(de);
de->inode = cpu_to_le32(dir->i_ino);
de->rec_len = ext3_rec_len_to_disk(inode->i_sb->s_blocksize -
EXT3_DIR_REC_LEN(1));
@@ -1825,7 +1869,7 @@ out_stop:
/*
* routine to check that the specified directory is empty (for rmdir)
*/
-static int empty_dir (struct inode * inode)
+static int empty_dir(struct inode *dir, struct inode *inode)
{
unsigned long offset;
struct buffer_head * bh;
@@ -1846,8 +1890,14 @@ static int empty_dir (struct inode * inode)
inode->i_ino);
return 1;
}
+
de = (struct ext3_dir_entry_2 *) bh->b_data;
- de1 = ext3_next_entry(de);
+ de1 = ext3_next_entry("empty_dir", dir, de, bh, 0);
+ if (de1 == NULL) {
+ brelse(bh);
+ return 1;
+ }
+
if (le32_to_cpu(de->inode) != inode->i_ino ||
!le32_to_cpu(de1->inode) ||
strcmp (".", de->name) ||
@@ -1860,8 +1910,8 @@ static int empty_dir (struct inode * inode)
}
offset = ext3_rec_len_from_disk(de->rec_len) +
ext3_rec_len_from_disk(de1->rec_len);
- de = ext3_next_entry(de1);
- while (offset < inode->i_size ) {
+ de = ext3_next_entry("empty_dir", dir, de1, bh, offset);
+ while (de && offset < inode->i_size) {
if (!bh ||
(void *) de >= (void *) (bh->b_data+sb->s_blocksize)) {
err = 0;
@@ -1890,7 +1940,7 @@ static int empty_dir (struct inode * inode)
return 0;
}
offset += ext3_rec_len_from_disk(de->rec_len);
- de = ext3_next_entry(de);
+ de = __ext3_next_entry(de);
}
brelse (bh);
return 1;
@@ -2068,7 +2121,7 @@ static int ext3_rmdir (struct inode * dir, struct dentry *dentry)
goto end_rmdir;

retval = -ENOTEMPTY;
- if (!empty_dir (inode))
+ if (!empty_dir(dir, inode))
goto end_rmdir;

retval = ext3_delete_entry(handle, dir, de, bh);
@@ -2244,7 +2297,7 @@ retry:
}

#define PARENT_INO(buffer) \
- (ext3_next_entry((struct ext3_dir_entry_2 *)(buffer))->inode)
+ (__ext3_next_entry((struct ext3_dir_entry_2 *)(buffer))->inode)

/*
* Anybody can rename anything with this: the permission checks are left to the
@@ -2297,7 +2350,7 @@ static int ext3_rename (struct inode * old_dir, struct dentry *old_dentry,
if (S_ISDIR(old_inode->i_mode)) {
if (new_inode) {
retval = -ENOTEMPTY;
- if (!empty_dir (new_inode))
+ if (!empty_dir(new_dir, new_inode))
goto end_rename;
}
retval = -EIO;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/