Re: [PATCH 05/20] ext4: fix slab-use-after-free in ext4_split_extent_at()

[Ojaswin Mujoo]

On Wed, Jul 10, 2024 at 12:06:39PM +0800, libaokun@xxxxxxxxxxxxxxx wrote:
> From: Baokun Li <libaokun1@xxxxxxxxxx>
> 
> We hit the following use-after-free:
> 
> ==================================================================
> BUG: KASAN: slab-use-after-free in ext4_split_extent_at+0xba8/0xcc0
> Read of size 2 at addr ffff88810548ed08 by task kworker/u20:0/40
> CPU: 0 PID: 40 Comm: kworker/u20:0 Not tainted 6.9.0-dirty #724
> Call Trace:
>  <TASK>
>  kasan_report+0x93/0xc0
>  ext4_split_extent_at+0xba8/0xcc0
>  ext4_split_extent.isra.0+0x18f/0x500
>  ext4_split_convert_extents+0x275/0x750
>  ext4_ext_handle_unwritten_extents+0x73e/0x1580
>  ext4_ext_map_blocks+0xe20/0x2dc0
>  ext4_map_blocks+0x724/0x1700
>  ext4_do_writepages+0x12d6/0x2a70
> [...]
> 
> Allocated by task 40:
>  __kmalloc_noprof+0x1ac/0x480
>  ext4_find_extent+0xf3b/0x1e70
>  ext4_ext_map_blocks+0x188/0x2dc0
>  ext4_map_blocks+0x724/0x1700
>  ext4_do_writepages+0x12d6/0x2a70
> [...]
> 
> Freed by task 40:
>  kfree+0xf1/0x2b0
>  ext4_find_extent+0xa71/0x1e70
>  ext4_ext_insert_extent+0xa22/0x3260
>  ext4_split_extent_at+0x3ef/0xcc0
>  ext4_split_extent.isra.0+0x18f/0x500
>  ext4_split_convert_extents+0x275/0x750
>  ext4_ext_handle_unwritten_extents+0x73e/0x1580
>  ext4_ext_map_blocks+0xe20/0x2dc0
>  ext4_map_blocks+0x724/0x1700
>  ext4_do_writepages+0x12d6/0x2a70
> [...]
> ==================================================================
> 
> The flow of issue triggering is as follows:
> 
> ext4_split_extent_at
>   path = *ppath
>   ext4_ext_insert_extent(ppath)
>     ext4_ext_create_new_leaf(ppath)
>       ext4_find_extent(orig_path)
>         path = *orig_path
>         read_extent_tree_block
>           // return -ENOMEM or -EIO
>         ext4_free_ext_path(path)
>           kfree(path)
>         *orig_path = NULL
>   a. If err is -ENOMEM:
>   ext4_ext_dirty(path + path->p_depth)
>   // path use-after-free !!!
>   b. If err is -EIO and we have EXT_DEBUG defined:
>   ext4_ext_show_leaf(path)
>     eh = path[depth].p_hdr
>     // path also use-after-free !!!
> 
> So when trying to zeroout or fix the extent length, call ext4_find_extent()
> to update the path.
> 
> In addition we use *ppath directly as an ext4_ext_show_leaf() input to
> avoid possible use-after-free when EXT_DEBUG is defined, and to avoid
> unnecessary path updates.
> 
> Fixes: dfe5080939ea ("ext4: drop EXT4_EX_NOFREE_ON_ERR from rest of extents handling code")
> Cc: stable@xxxxxxxxxx
> Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
> ---
>  fs/ext4/extents.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 6e5b5baf3aa6..3a70a0739af8 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -3252,6 +3252,25 @@ static int ext4_split_extent_at(handle_t *handle,
>   if (err != -ENOSPC && err != -EDQUOT && err != -ENOMEM)
>     goto out;
>  
> + /*
> +  * Update path is required because previous ext4_ext_insert_extent()
> +  * may have freed or reallocated the path. Using EXT4_EX_NOFAIL
> +  * guarantees that ext4_find_extent() will not return -ENOMEM,
> +  * otherwise -ENOMEM will cause a retry in do_writepages(), and a
> +  * WARN_ON may be triggered in ext4_da_update_reserve_space() due to
> +  * an incorrect ee_len causing the i_reserved_data_blocks exception.
> +  */
> + path = ext4_find_extent(inode, ee_block, ppath,
> +       flags | EXT4_EX_NOFAIL);
> + if (IS_ERR(path)) {
> +   EXT4_ERROR_INODE(inode, "Failed split extent on %u, err %ld",
> +        split, PTR_ERR(path));
> +   return PTR_ERR(path);
> + }
> + depth = ext_depth(inode);
> + ex = path[depth].p_ext;
> + *ppath = path;
> +

Hi Baokun, nice catch! 

I was just wondering if it makes more sense to only update path if we
encounter an error:

  err = ext4_ext_insert_extent(handle, inode, ppath, &newex, flags);
  if (err != -ENOSPC && err != -EDQUOT && err != -ENOMEM)
    goto out;
  else if (err < 0) {
    ...
    path = ext4_find_extent(inode, ee_block, ppath, flags | EXT4_EX_NOFAIL);
  }

I believe that's the only time we'd end up with the use after free issue
and this way we can avoid the (maybe tiny) performance overhead of calling 
the extra find extent. What are your thoughts?

regards,
ojaswin

>   if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
>     if (split_flag & (EXT4_EXT_DATA_VALID1|EXT4_EXT_DATA_VALID2)) {
>       if (split_flag & EXT4_EXT_DATA_VALID1) {
> @@ -3304,7 +3323,7 @@ static int ext4_split_extent_at(handle_t *handle,
>   ext4_ext_dirty(handle, inode, path + path->p_depth);
>   return err;
>  out:
> - ext4_ext_show_leaf(inode, path);
> + ext4_ext_show_leaf(inode, *ppath);
>   return err;
>  }
>  
> -- 
> 2.39.2
>