| Message ID | 20230814074351.96741-1-libaokun1@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ext4: fix slab-use-after-free in ext4_es_insert_extent() | expand |
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 >
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!
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)
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(-)