linux-ext4 - Re: [PATCH 06/20] ext4: avoid use-after-free in ext4_ext_show

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: <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