| 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
| ||
|
Message-ID: <aa03f191-445c-0d2e-d6d7-0a3208d7df7a@huawei.com>
Date: Tue, 15 Aug 2023 09:20:20 +0800
From: Baokun Li <libaokun1@...wei.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>, <yukuai3@...wei.com>,
Yikebaer Aizezi <yikebaer61@...il.com>,
Baokun Li <libaokun1@...wei.com>
Subject: Re: [PATCH] ext4: fix slab-use-after-free in ext4_es_insert_extent()
On 2023/8/14 18:45, Jan Kara wrote:
> On Mon 14-08-23 15:43:51, Baokun Li wrote:
>> Yikebaer reported an issue:
>> ==================================================================
>> BUG: KASAN: slab-use-after-free in ext4_es_insert_extent+0xc68/0xcb0
>> fs/ext4/extents_status.c:894
>> Read of size 4 at addr ffff888112ecc1a4 by task syz-executor/8438
>>
>> CPU: 1 PID: 8438 Comm: syz-executor Not tainted 6.5.0-rc5 #1
>> Call Trace:
>> [...]
>> kasan_report+0xba/0xf0 mm/kasan/report.c:588
>> ext4_es_insert_extent+0xc68/0xcb0 fs/ext4/extents_status.c:894
>> ext4_map_blocks+0x92a/0x16f0 fs/ext4/inode.c:680
>> ext4_alloc_file_blocks.isra.0+0x2df/0xb70 fs/ext4/extents.c:4462
>> ext4_zero_range fs/ext4/extents.c:4622 [inline]
>> ext4_fallocate+0x251c/0x3ce0 fs/ext4/extents.c:4721
>> [...]
>>
>> Allocated by task 8438:
>> [...]
>> kmem_cache_zalloc include/linux/slab.h:693 [inline]
>> __es_alloc_extent fs/ext4/extents_status.c:469 [inline]
>> ext4_es_insert_extent+0x672/0xcb0 fs/ext4/extents_status.c:873
>> ext4_map_blocks+0x92a/0x16f0 fs/ext4/inode.c:680
>> ext4_alloc_file_blocks.isra.0+0x2df/0xb70 fs/ext4/extents.c:4462
>> ext4_zero_range fs/ext4/extents.c:4622 [inline]
>> ext4_fallocate+0x251c/0x3ce0 fs/ext4/extents.c:4721
>> [...]
>>
>> Freed by task 8438:
>> [...]
>> kmem_cache_free+0xec/0x490 mm/slub.c:3823
>> ext4_es_try_to_merge_right fs/ext4/extents_status.c:593 [inline]
>> __es_insert_extent+0x9f4/0x1440 fs/ext4/extents_status.c:802
>> ext4_es_insert_extent+0x2ca/0xcb0 fs/ext4/extents_status.c:882
>> ext4_map_blocks+0x92a/0x16f0 fs/ext4/inode.c:680
>> ext4_alloc_file_blocks.isra.0+0x2df/0xb70 fs/ext4/extents.c:4462
>> ext4_zero_range fs/ext4/extents.c:4622 [inline]
>> ext4_fallocate+0x251c/0x3ce0 fs/ext4/extents.c:4721
>> [...]
>> ==================================================================
>>
>> The flow of issue triggering is as follows:
>> 1. remove es
>> raw es es removed es1
>> |-------------------| -> |----|.......|------|
>>
>> 2. insert es
>> es insert es1 merge with es es1 merge with es and free es1
>> |----|.......|------| -> |------------|------| -> |-------------------|
>>
>> es merges with newes, then merges with es1, frees es1, then determines
>> if es1->es_len is 0 and triggers a UAF.
>>
>> The code flow is as follows:
>> ext4_es_insert_extent
>> es1 = __es_alloc_extent(true);
>> es2 = __es_alloc_extent(true);
>> __es_remove_extent(inode, lblk, end, NULL, es1)
>> __es_insert_extent(inode, &newes, es1) ---> insert es1 to es tree
>> __es_insert_extent(inode, &newes, es2)
>> ext4_es_try_to_merge_right
>> ext4_es_free_extent(inode, es1) ---> es1 is freed
>> if (es1 && !es1->es_len)
>> // Trigger UAF by determining if es1 is used.
>>
>> We determine whether es1 or es2 is used immediately after calling
>> __es_remove_extent() or __es_insert_extent() to avoid triggering a
>> UAF if es1 or es2 is freed.
>>
>> Reported-by: Yikebaer Aizezi <yikebaer61@...il.com>
>> Closes: https://lore.kernel.org/lkml/CALcu4raD4h9coiyEBL4Bm0zjDwxC2CyPiTwsP3zFuhot6y9Beg@mail.gmail.com
>> Fixes: 2a69c450083d ("ext4: using nofail preallocation in ext4_es_insert_extent()")
>> Signed-off-by: Baokun Li <libaokun1@...wei.com>
> Good spotting!
>
>> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
>> index 9b5b8951afb4..cceac517265f 100644
>> --- a/fs/ext4/extents_status.c
>> +++ b/fs/ext4/extents_status.c
>> @@ -878,23 +878,21 @@ void ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
>> err1 = __es_remove_extent(inode, lblk, end, NULL, es1);
>> if (err1 != 0)
>> goto error;
>> + if (es1 && !es1->es_len)
>> + __es_free_extent(es1);
> I'd prefer if you also set es1 to NULL after freeing es1. Something like:
>
> /* Free preallocated extent if it didn't get used. */
> if (es1) {
> if (!es1->es_len)
> __es_free_extent(es1);
> es1 = NULL;
> }
>
> Currently I don't think there's a realistic case how we could trigger the
> retry loop after we've once decided to preallocate entries but it just
> makes the code more obvously correct. Similarly below for es2 and for the
> cases in ext4_es_insert_delayed_block(). Thanks!
>
> Honza
Indeed, the code looks more correct this way！
>>
>> err2 = __es_insert_extent(inode, &newes, es2);
>> if (err2 == -ENOMEM && !ext4_es_must_keep(&newes))
>> err2 = 0;
>> if (err2 != 0)
>> goto error;
>> + if (es2 && !es2->es_len)
>> + __es_free_extent(es2);
>>
>> if (sbi->s_cluster_ratio > 1 && test_opt(inode->i_sb, DELALLOC) &&
>> (status & EXTENT_STATUS_WRITTEN ||
>> status & EXTENT_STATUS_UNWRITTEN))
>> __revise_pending(inode, lblk, len);
>> -
>> - /* es is pre-allocated but not used, free it. */
>> - if (es1 && !es1->es_len)
>> - __es_free_extent(es1);
>> - if (es2 && !es2->es_len)
>> - __es_free_extent(es2);
>> error:
>> write_unlock(&EXT4_I(inode)->i_es_lock);
>> if (err1 || err2)
>> @@ -2047,19 +2045,17 @@ void ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
>> err1 = __es_remove_extent(inode, lblk, lblk, NULL, es1);
>> if (err1 != 0)
>> goto error;
>> + if (es1 && !es1->es_len)
>> + __es_free_extent(es1);
> The same
>
>>
>> err2 = __es_insert_extent(inode, &newes, es2);
>> if (err2 != 0)
>> goto error;
>> + if (es2 && !es2->es_len)
>> + __es_free_extent(es2);
>>
>> if (allocated)
>> __insert_pending(inode, lblk);
>> -
>> - /* es is pre-allocated but not used, free it. */
>> - if (es1 && !es1->es_len)
>> - __es_free_extent(es1);
>> - if (es2 && !es2->es_len)
>> - __es_free_extent(es2);
>> error:
>> write_unlock(&EXT4_I(inode)->i_es_lock);
>> if (err1 || err2)
>> --
>> 2.31.1
>>
Thank you very much for your review!
I will send a new version with your suggestions later!
--
With Best Regards,
Baokun Li
.
Powered by blists - more mailing lists