[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <19ce4750-b1e3-4db6-821e-a70b6d03f2ec@huaweicloud.com>
Date: Thu, 25 Jul 2024 13:41:38 +0800
From: Baokun Li <libaokun@...weicloud.com>
To: Jan Kara <jack@...e.cz>
Cc: linux-ext4@...r.kernel.org, tytso@....edu, adilger.kernel@...ger.ca,
ritesh.list@...il.com, linux-kernel@...r.kernel.org, yi.zhang@...wei.com,
yangerkun@...wei.com, Baokun Li <libaokun1@...wei.com>,
Baokun Li <libaokun@...weicloud.com>
Subject: Re: [PATCH 06/20] ext4: avoid use-after-free in ext4_ext_show_leaf()
On 2024/7/25 3:16, Jan Kara wrote:
> On Wed 10-07-24 12:06:40, libaokun@...weicloud.com wrote:
>> From: Baokun Li <libaokun1@...wei.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@...wei.com>
> Looks good. Feel free to add:
>
> Reviewed-by: Jan Kara <jack@...e.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
Hi Honza,
Thank you very much for your review!
Yes, it was too confusing, which is why I dropped all ppaths after fixing
the problem. Judging and using the returned path every time would
make the code look a lot simpler.
Thanks,
Baokun
>> ---
>> 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
>>
--
With Best Regards,
Baokun Li
Powered by blists - more mailing lists