Re: [PATCH 06/20] ext4: avoid use-after-free in ext4_ext_show_leaf()

[Jan Kara <jack@suse.cz>]

On Wed 10-07-24 12:06:40, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
> 
> In ext4_find_extent(), path may be freed by error or be reallocated, so
> using a previously saved *ppath may have been freed and thus may trigger
> use-after-free, as follows:
> 
> ext4_split_extent
>   path = *ppath;
>   ext4_split_extent_at(ppath)
>   path = ext4_find_extent(ppath)
>   ext4_split_extent_at(ppath)
>     // ext4_find_extent fails to free path
>     // but zeroout succeeds
>   ext4_ext_show_leaf(inode, path)
>     eh = path[depth].p_hdr
>     // path use-after-free !!!
> 
> Similar to ext4_split_extent_at(), we use *ppath directly as an input to
> ext4_ext_show_leaf(). Fix a spelling error by the way.
> 
> Same problem in ext4_ext_handle_unwritten_extents(). Since 'path' is only
> used in ext4_ext_show_leaf(), remove 'path' and use *ppath directly.
> 
> This issue is triggered only when EXT_DEBUG is defined and therefore does
> not affect functionality.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

I'd just note that this shows that modifying ppath in the called function
was not a great idea as it makes possible use-after-free issues due to
cached values being used very hard to spot and very easy to introduce...

								Honza

> ---
>  fs/ext4/extents.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 3a70a0739af8..1660434fbfc7 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -3328,7 +3328,7 @@ static int ext4_split_extent_at(handle_t *handle,
>  }
>  
>  /*
> - * ext4_split_extents() splits an extent and mark extent which is covered
> + * ext4_split_extent() splits an extent and mark extent which is covered
>   * by @map as split_flags indicates
>   *
>   * It may result in splitting the extent into multiple extents (up to three)
> @@ -3404,7 +3404,7 @@ static int ext4_split_extent(handle_t *handle,
>  			goto out;
>  	}
>  
> -	ext4_ext_show_leaf(inode, path);
> +	ext4_ext_show_leaf(inode, *ppath);
>  out:
>  	return err ? err : allocated;
>  }
> @@ -3869,14 +3869,13 @@ ext4_ext_handle_unwritten_extents(handle_t *handle, struct inode *inode,
>  			struct ext4_ext_path **ppath, int flags,
>  			unsigned int allocated, ext4_fsblk_t newblock)
>  {
> -	struct ext4_ext_path __maybe_unused *path = *ppath;
>  	int ret = 0;
>  	int err = 0;
>  
>  	ext_debug(inode, "logical block %llu, max_blocks %u, flags 0x%x, allocated %u\n",
>  		  (unsigned long long)map->m_lblk, map->m_len, flags,
>  		  allocated);
> -	ext4_ext_show_leaf(inode, path);
> +	ext4_ext_show_leaf(inode, *ppath);
>  
>  	/*
>  	 * When writing into unwritten space, we should not fail to
> @@ -3973,7 +3972,7 @@ ext4_ext_handle_unwritten_extents(handle_t *handle, struct inode *inode,
>  	if (allocated > map->m_len)
>  		allocated = map->m_len;
>  	map->m_len = allocated;
> -	ext4_ext_show_leaf(inode, path);
> +	ext4_ext_show_leaf(inode, *ppath);
>  out2:
>  	return err ? err : allocated;
>  }
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR