htree_dirblock_to_tree() declares a non-initialized 'err' variable, which is
passed as a reference to another functions expecting them to set this variable
with thei error codes.
It's passed to ext4_bread(), which then passes it to ext4_getblk(). If
ext4_map_blocks() returns 0 due to a lookup failure, leaving the ext4_getblk()
buffer_head uninitialized, it will make ext4_getblk() return to ext4_bread()
without initialize the 'err' variable, and ext4_bread() will return to
htree_dirblock_to_tree() with this variable still uninitialized.
htree_dirblock_to_tree() will pass this variable with garbage back to
ext4_htree_fill_tree(), which expects a number of directory entries added to the
rb-tree. which, in case, might return a fake non-zero value due the garbage left
in the 'err' variable, leading the kernel to an Oops in ext4_dx_readdir(), once
this is expecting a filled rb-tree node, when in turn it will have a NULL-ed
one, causing an invalid page request when trying to get a fname struct from
this NULL-ed rb-tree node in this line:
fname = rb_entry(info->curr_node, struct fname, rb_hash);
The patch itself initializes the err variable in htree_dirblock_to_tree() to
avoid usage mistakes by the called functions, and also fix ext4_getblk() to
return a initialized 'err' variable when ext4_map_blocks() fails a
lookup.
Signed-off-by: Carlos Maiolino <[email protected]>
---
fs/ext4/inode.c | 2 +-
fs/ext4/namei.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index dff171c..80fae26 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -723,7 +723,7 @@ struct buffer_head *ext4_getblk(handle_t *handle, struct inode *inode,
{
struct ext4_map_blocks map;
struct buffer_head *bh;
- int fatal = 0, err;
+ int fatal = 0, err = 0;
J_ASSERT(handle != NULL || create == 0);
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 2a42cc0..262f863 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -839,7 +839,7 @@ static int htree_dirblock_to_tree(struct file *dir_file,
{
struct buffer_head *bh;
struct ext4_dir_entry_2 *de, *top;
- int err, count = 0;
+ int err = 0, count = 0;
dxtrace(printk(KERN_INFO "In htree dirblock_to_tree: block %lu\n",
(unsigned long)block));
--
1.7.11.4
On 9/8/12 7:49 AM, Carlos Maiolino wrote:
> htree_dirblock_to_tree() declares a non-initialized 'err' variable, which is
> passed as a reference to another functions expecting them to set this variable
> with thei error codes.
> It's passed to ext4_bread(), which then passes it to ext4_getblk(). If
> ext4_map_blocks() returns 0 due to a lookup failure, leaving the ext4_getblk()
> buffer_head uninitialized, it will make ext4_getblk() return to ext4_bread()
> without initialize the 'err' variable, and ext4_bread() will return to
> htree_dirblock_to_tree() with this variable still uninitialized.
> htree_dirblock_to_tree() will pass this variable with garbage back to
> ext4_htree_fill_tree(), which expects a number of directory entries added to the
> rb-tree. which, in case, might return a fake non-zero value due the garbage left
> in the 'err' variable, leading the kernel to an Oops in ext4_dx_readdir(), once
> this is expecting a filled rb-tree node, when in turn it will have a NULL-ed
> one, causing an invalid page request when trying to get a fname struct from
> this NULL-ed rb-tree node in this line:
>
> fname = rb_entry(info->curr_node, struct fname, rb_hash);
>
> The patch itself initializes the err variable in htree_dirblock_to_tree() to
> avoid usage mistakes by the called functions, and also fix ext4_getblk() to
> return a initialized 'err' variable when ext4_map_blocks() fails a
> lookup.
>
> Signed-off-by: Carlos Maiolino <[email protected]>
> ---
> fs/ext4/inode.c | 2 +-
> fs/ext4/namei.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index dff171c..80fae26 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -723,7 +723,7 @@ struct buffer_head *ext4_getblk(handle_t *handle, struct inode *inode,
> {
> struct ext4_map_blocks map;
> struct buffer_head *bh;
> - int fatal = 0, err;
> + int fatal = 0, err = 0;
>
> J_ASSERT(handle != NULL || create == 0);
I'm afraid this doesn't fix it. So now err is init to 0, but then:
err = ext4_map_blocks(handle, inode, &map,
create ? EXT4_GET_BLOCKS_CREATE : 0);
so err is immediately reset to whatever ext4_map_blocks returns, which might be 0.
If so, we don't go down this case:
if (err < 0)
*errp = err;
and we do go down this case,
if (err <= 0)
return NULL;
in which case we return with *errp unset.
It needs something like this, though maybe this could be made prettier/clearer.
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index dff171c..25fe8bf 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -732,11 +732,11 @@ struct buffer_head *ext4_getblk(handle_t *handle, struct inode *inode,
err = ext4_map_blocks(handle, inode, &map,
create ? EXT4_GET_BLOCKS_CREATE : 0);
+ *errp = 0;
if (err < 0)
*errp = err;
if (err <= 0)
return NULL;
- *errp = 0;
bh = sb_getblk(inode->i_sb, map.m_pblk);
if (!bh) {
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 2a42cc0..262f863 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -839,7 +839,7 @@ static int htree_dirblock_to_tree(struct file *dir_file,
> {
> struct buffer_head *bh;
> struct ext4_dir_entry_2 *de, *top;
> - int err, count = 0;
> + int err = 0, count = 0;
>
> dxtrace(printk(KERN_INFO "In htree dirblock_to_tree: block %lu\n",
> (unsigned long)block));
>
> I'm afraid this doesn't fix it. So now err is init to 0, but then:
>
> err = ext4_map_blocks(handle, inode, &map,
> create ? EXT4_GET_BLOCKS_CREATE : 0);
>
> so err is immediately reset to whatever ext4_map_blocks returns, which might be 0.
> If so, we don't go down this case:
>
> if (err < 0)
> *errp = err;
>
> and we do go down this case,
>
> if (err <= 0)
> return NULL;
>
> in which case we return with *errp unset.
>
> It needs something like this, though maybe this could be made prettier/clearer.
>
> + *errp = 0;
> if (err < 0)
> *errp = err;
> if (err <= 0)
> return NULL;
> - *errp = 0;
Agreed. just initializing err variable into ext4_getblk() won't ensure *errp
will be filled with 'err' content. Thanks. I'll wait for some extra inputs and
see if is there anything else people have in mind, then release a v2 patch
--
--Carlos