* [PATCH] ext4: fix slab-use-after-free in ext4_es_insert_extent()
@ 2023-08-14 7:43 Baokun Li
2023-08-14 10:45 ` Jan Kara
0 siblings, 1 reply; 3+ messages in thread
From: Baokun Li @ 2023-08-14 7:43 UTC (permalink / raw)
To: linux-ext4
Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
yangerkun, yukuai3, libaokun1, Yikebaer Aizezi
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@gmail.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@huawei.com>
---
fs/ext4/extents_status.c | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
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);
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);
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] ext4: fix slab-use-after-free in ext4_es_insert_extent()
2023-08-14 7:43 [PATCH] ext4: fix slab-use-after-free in ext4_es_insert_extent() Baokun Li
@ 2023-08-14 10:45 ` Jan Kara
2023-08-15 1:20 ` Baokun Li
0 siblings, 1 reply; 3+ messages in thread
From: Jan Kara @ 2023-08-14 10:45 UTC (permalink / raw)
To: Baokun Li
Cc: linux-ext4, tytso, adilger.kernel, jack, ritesh.list,
linux-kernel, yi.zhang, yangerkun, yukuai3, Yikebaer Aizezi
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@gmail.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@huawei.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
>
> 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
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ext4: fix slab-use-after-free in ext4_es_insert_extent()
2023-08-14 10:45 ` Jan Kara
@ 2023-08-15 1:20 ` Baokun Li
0 siblings, 0 replies; 3+ messages in thread
From: Baokun Li @ 2023-08-15 1:20 UTC (permalink / raw)
To: Jan Kara
Cc: linux-ext4, tytso, adilger.kernel, ritesh.list, linux-kernel,
yi.zhang, yangerkun, yukuai3, Yikebaer Aizezi, Baokun Li
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@gmail.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@huawei.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
.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-08-15 1:21 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-14 7:43 [PATCH] ext4: fix slab-use-after-free in ext4_es_insert_extent() Baokun Li
2023-08-14 10:45 ` Jan Kara
2023-08-15 1:20 ` Baokun Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).