On Wed 10-07-24 12:06:40, libaokun@xxxxxxxxxxxxxxx wrote:
> From: Baokun Li <libaokun1@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
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@xxxxxxxx>
SUSE Labs, CR
