linux-ext4 - Re: [PATCH 05/20] ext4: fix slab-use-after-free in ext4_split_extent

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240724191321.rgdevlcordkjhpgy@quack3>
Date: Wed, 24 Jul 2024 21:13:21 +0200
From: Jan Kara <jack@...e.cz>
To: libaokun@...weicloud.com
Cc: linux-ext4@...r.kernel.org, tytso@....edu, adilger.kernel@...ger.ca,
	jack@...e.cz, ritesh.list@...il.com, linux-kernel@...r.kernel.org,
	yi.zhang@...wei.com, yangerkun@...wei.com,
	Baokun Li <libaokun1@...wei.com>, stable@...nel.org
Subject: Re: [PATCH 05/20] ext4: fix slab-use-after-free in
 ext4_split_extent_at()

On Wed 10-07-24 12:06:39, libaokun@...weicloud.com wrote:
> From: Baokun Li <libaokun1@...wei.com>
> 
> 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@...nel.org
> Signed-off-by: Baokun Li <libaokun1@...wei.com>

Looks good. Feel free to add:

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

									Honza

> ---
>  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;
> +
>  	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
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR