Message-ID: <deec537f-b9f9-46b4-960a-44e70e272d79@huaweicloud.com>
Date: Tue, 30 Jul 2024 16:57:35 +0800
From: Baokun Li <libaokun@...weicloud.com>
To: Ojaswin Mujoo <ojaswin@...ux.ibm.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, Baokun Li <libaokun@...weicloud.com>
Subject: Re: [PATCH 05/20] ext4: fix slab-use-after-free in
 ext4_split_extent_at()

On 2024/7/27 18:36, Ojaswin Mujoo wrote:
> On Wed, Jul 10, 2024 at 12:06:39PM +0800, 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>
>> ---
>>   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
Hi Ojaswin,

We're already there now, updating the path only on recoverable,
non-critical errors (-ENOSPC, -EDQUOT, and -ENOMEM).

Thanks,
Baokun
>>    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
>>
-- 
With Best Regards,
Baokun Li

Powered by blists - more mailing lists