* [PATCH 0/3] Fix UAF and kmemleak when sanity test
@ 2022-11-01 2:53 Zhang Xiaoxu
2022-11-01 2:53 ` [PATCH 1/3] btrfs: Fix wrong check in btrfs_free_dummy_root() Zhang Xiaoxu
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Zhang Xiaoxu @ 2022-11-01 2:53 UTC (permalink / raw)
To: linux-btrfs, zhangxiaoxu5, clm, josef, dsterba
Zhang Xiaoxu (3):
btrfs: Fix wrong check in btrfs_free_dummy_root()
btrfs: Fix uaf of the ulist in test_multiple_refs()
btrfs: Fix ulist memory leak in test_multiple_refs()
fs/btrfs/tests/btrfs-tests.c | 2 +-
fs/btrfs/tests/qgroup-tests.c | 22 ++++++++++++++++++----
2 files changed, 19 insertions(+), 5 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/3] btrfs: Fix wrong check in btrfs_free_dummy_root() 2022-11-01 2:53 [PATCH 0/3] Fix UAF and kmemleak when sanity test Zhang Xiaoxu @ 2022-11-01 2:53 ` Zhang Xiaoxu 2022-11-01 4:10 ` Anand Jain 2022-11-03 16:44 ` David Sterba 2022-11-01 2:53 ` [PATCH 2/3] btrfs: Fix uaf of the ulist in test_multiple_refs() Zhang Xiaoxu 2022-11-01 2:53 ` [PATCH 3/3] btrfs: Fix ulist memory leak " Zhang Xiaoxu 2 siblings, 2 replies; 9+ messages in thread From: Zhang Xiaoxu @ 2022-11-01 2:53 UTC (permalink / raw) To: linux-btrfs, zhangxiaoxu5, clm, josef, dsterba The btrfs_alloc_dummy_root() use ERR_PTR as the error return value rather than NULL, if error happened, there will be a null-ptr-deref when free the dummy root: BUG: KASAN: null-ptr-deref in btrfs_free_dummy_root+0x21/0x50 [btrfs] Read of size 8 at addr 000000000000002c by task insmod/258926 CPU: 2 PID: 258926 Comm: insmod Tainted: G W 6.1.0-rc2+ #5 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-1.fc33 04/01/2014 Call Trace: <TASK> dump_stack_lvl+0x34/0x44 kasan_report+0xb7/0x140 kasan_check_range+0x145/0x1a0 btrfs_free_dummy_root+0x21/0x50 [btrfs] btrfs_test_free_space_cache+0x1a8c/0x1add [btrfs] btrfs_run_sanity_tests+0x65/0x80 [btrfs] init_btrfs_fs+0xec/0x154 [btrfs] do_one_initcall+0x87/0x2a0 do_init_module+0xdf/0x320 load_module+0x3006/0x3390 __do_sys_finit_module+0x113/0x1b0 do_syscall_64+0x35/0x80 entry_SYSCALL_64_after_hwframe+0x46/0xb0 Fixes: aaedb55bc08f ("Btrfs: add tests for btrfs_get_extent") Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> --- fs/btrfs/tests/btrfs-tests.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/btrfs/tests/btrfs-tests.c b/fs/btrfs/tests/btrfs-tests.c index 9c478fa256f6..d43cb5242fec 100644 --- a/fs/btrfs/tests/btrfs-tests.c +++ b/fs/btrfs/tests/btrfs-tests.c @@ -200,7 +200,7 @@ void btrfs_free_dummy_fs_info(struct btrfs_fs_info *fs_info) void btrfs_free_dummy_root(struct btrfs_root *root) { - if (!root) + if (IS_ERR_OR_NULL(root)) return; /* Will be freed by btrfs_free_fs_roots */ if (WARN_ON(test_bit(BTRFS_ROOT_IN_RADIX, &root->state))) -- 2.31.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] btrfs: Fix wrong check in btrfs_free_dummy_root() 2022-11-01 2:53 ` [PATCH 1/3] btrfs: Fix wrong check in btrfs_free_dummy_root() Zhang Xiaoxu @ 2022-11-01 4:10 ` Anand Jain 2022-11-03 16:44 ` David Sterba 1 sibling, 0 replies; 9+ messages in thread From: Anand Jain @ 2022-11-01 4:10 UTC (permalink / raw) To: Zhang Xiaoxu, linux-btrfs On 11/1/22 10:53, Zhang Xiaoxu wrote: > The btrfs_alloc_dummy_root() use ERR_PTR as the error return value > rather than NULL, if error happened, there will be a null-ptr-deref > when free the dummy root: > > BUG: KASAN: null-ptr-deref in btrfs_free_dummy_root+0x21/0x50 [btrfs] > Read of size 8 at addr 000000000000002c by task insmod/258926 > > CPU: 2 PID: 258926 Comm: insmod Tainted: G W 6.1.0-rc2+ #5 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-1.fc33 04/01/2014 > Call Trace: > <TASK> > dump_stack_lvl+0x34/0x44 > kasan_report+0xb7/0x140 > kasan_check_range+0x145/0x1a0 > btrfs_free_dummy_root+0x21/0x50 [btrfs] > btrfs_test_free_space_cache+0x1a8c/0x1add [btrfs] > btrfs_run_sanity_tests+0x65/0x80 [btrfs] > init_btrfs_fs+0xec/0x154 [btrfs] > do_one_initcall+0x87/0x2a0 > do_init_module+0xdf/0x320 > load_module+0x3006/0x3390 > __do_sys_finit_module+0x113/0x1b0 > do_syscall_64+0x35/0x80 > entry_SYSCALL_64_after_hwframe+0x46/0xb0 > > Fixes: aaedb55bc08f ("Btrfs: add tests for btrfs_get_extent") > Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> Reviewed-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/tests/btrfs-tests.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/tests/btrfs-tests.c b/fs/btrfs/tests/btrfs-tests.c > index 9c478fa256f6..d43cb5242fec 100644 > --- a/fs/btrfs/tests/btrfs-tests.c > +++ b/fs/btrfs/tests/btrfs-tests.c > @@ -200,7 +200,7 @@ void btrfs_free_dummy_fs_info(struct btrfs_fs_info *fs_info) > > void btrfs_free_dummy_root(struct btrfs_root *root) > { > - if (!root) > + if (IS_ERR_OR_NULL(root)) > return; > /* Will be freed by btrfs_free_fs_roots */ > if (WARN_ON(test_bit(BTRFS_ROOT_IN_RADIX, &root->state))) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] btrfs: Fix wrong check in btrfs_free_dummy_root() 2022-11-01 2:53 ` [PATCH 1/3] btrfs: Fix wrong check in btrfs_free_dummy_root() Zhang Xiaoxu 2022-11-01 4:10 ` Anand Jain @ 2022-11-03 16:44 ` David Sterba 2022-11-04 1:09 ` zhangxiaoxu (A) 1 sibling, 1 reply; 9+ messages in thread From: David Sterba @ 2022-11-03 16:44 UTC (permalink / raw) To: Zhang Xiaoxu; +Cc: linux-btrfs, clm, josef, dsterba On Tue, Nov 01, 2022 at 10:53:54AM +0800, Zhang Xiaoxu wrote: > The btrfs_alloc_dummy_root() use ERR_PTR as the error return value > rather than NULL, if error happened, there will be a null-ptr-deref > when free the dummy root: > > BUG: KASAN: null-ptr-deref in btrfs_free_dummy_root+0x21/0x50 [btrfs] > Read of size 8 at addr 000000000000002c by task insmod/258926 > > CPU: 2 PID: 258926 Comm: insmod Tainted: G W 6.1.0-rc2+ #5 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-1.fc33 04/01/2014 > Call Trace: > <TASK> > dump_stack_lvl+0x34/0x44 > kasan_report+0xb7/0x140 > kasan_check_range+0x145/0x1a0 > btrfs_free_dummy_root+0x21/0x50 [btrfs] > btrfs_test_free_space_cache+0x1a8c/0x1add [btrfs] > btrfs_run_sanity_tests+0x65/0x80 [btrfs] > init_btrfs_fs+0xec/0x154 [btrfs] > do_one_initcall+0x87/0x2a0 > do_init_module+0xdf/0x320 > load_module+0x3006/0x3390 > __do_sys_finit_module+0x113/0x1b0 > do_syscall_64+0x35/0x80 > entry_SYSCALL_64_after_hwframe+0x46/0xb0 > > Fixes: aaedb55bc08f ("Btrfs: add tests for btrfs_get_extent") > Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> Added to misc-next, thanks. Patches 2 and 3 are dropped as Filipe sent the complete fixes in his series. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] btrfs: Fix wrong check in btrfs_free_dummy_root() 2022-11-03 16:44 ` David Sterba @ 2022-11-04 1:09 ` zhangxiaoxu (A) 0 siblings, 0 replies; 9+ messages in thread From: zhangxiaoxu (A) @ 2022-11-04 1:09 UTC (permalink / raw) To: dsterba; +Cc: linux-btrfs, clm, josef, dsterba On 2022/11/4 0:44, David Sterba wrote: > On Tue, Nov 01, 2022 at 10:53:54AM +0800, Zhang Xiaoxu wrote: >> The btrfs_alloc_dummy_root() use ERR_PTR as the error return value >> rather than NULL, if error happened, there will be a null-ptr-deref >> when free the dummy root: >> >> BUG: KASAN: null-ptr-deref in btrfs_free_dummy_root+0x21/0x50 [btrfs] >> Read of size 8 at addr 000000000000002c by task insmod/258926 >> >> CPU: 2 PID: 258926 Comm: insmod Tainted: G W 6.1.0-rc2+ #5 >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-1.fc33 04/01/2014 >> Call Trace: >> <TASK> >> dump_stack_lvl+0x34/0x44 >> kasan_report+0xb7/0x140 >> kasan_check_range+0x145/0x1a0 >> btrfs_free_dummy_root+0x21/0x50 [btrfs] >> btrfs_test_free_space_cache+0x1a8c/0x1add [btrfs] >> btrfs_run_sanity_tests+0x65/0x80 [btrfs] >> init_btrfs_fs+0xec/0x154 [btrfs] >> do_one_initcall+0x87/0x2a0 >> do_init_module+0xdf/0x320 >> load_module+0x3006/0x3390 >> __do_sys_finit_module+0x113/0x1b0 >> do_syscall_64+0x35/0x80 >> entry_SYSCALL_64_after_hwframe+0x46/0xb0 >> >> Fixes: aaedb55bc08f ("Btrfs: add tests for btrfs_get_extent") >> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> > > Added to misc-next, thanks. Patches 2 and 3 are dropped as Filipe sent > the complete fixes in his series.Agreed, thanks David and Filipe. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] btrfs: Fix uaf of the ulist in test_multiple_refs() 2022-11-01 2:53 [PATCH 0/3] Fix UAF and kmemleak when sanity test Zhang Xiaoxu 2022-11-01 2:53 ` [PATCH 1/3] btrfs: Fix wrong check in btrfs_free_dummy_root() Zhang Xiaoxu @ 2022-11-01 2:53 ` Zhang Xiaoxu 2022-11-01 8:18 ` Filipe Manana 2022-11-01 2:53 ` [PATCH 3/3] btrfs: Fix ulist memory leak " Zhang Xiaoxu 2 siblings, 1 reply; 9+ messages in thread From: Zhang Xiaoxu @ 2022-11-01 2:53 UTC (permalink / raw) To: linux-btrfs, zhangxiaoxu5, clm, josef, dsterba There is a use-after-free report when do sanity tests: BUG: KASAN: use-after-free in ulist_free+0x25/0xa0 [btrfs] Read of size 8 at addr ffff888296ab4748 by task insmod/78078 CPU: 7 PID: 78078 Comm: insmod Tainted: G W 6.1.0-rc2+ #5 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-1.fc33 04/01/2014 Call Trace: <TASK> dump_stack_lvl+0x34/0x44 print_report+0x171/0x472 kasan_report+0xb7/0x140 ulist_free+0x25/0xa0 [btrfs] test_multiple_refs.constprop.0+0x411/0x470 [btrfs] btrfs_test_qgroups+0x2da/0x300 [btrfs] btrfs_run_sanity_tests.cold+0x5c/0xd5 [btrfs] init_btrfs_fs+0xec/0x154 [btrfs] do_one_initcall+0x87/0x2a0 do_init_module+0xdf/0x320 load_module+0x3006/0x3390 __do_sys_finit_module+0x113/0x1b0 do_syscall_64+0x35/0x80 entry_SYSCALL_64_after_hwframe+0x46/0xb0 Allocated by task 78078: kasan_save_stack+0x1e/0x40 kasan_set_track+0x21/0x30 __kasan_kmalloc+0x7a/0x90 ulist_alloc+0x55/0xe0 [btrfs] btrfs_find_all_roots_safe+0x9d/0x1c0 [btrfs] test_multiple_refs.constprop.0+0x1e0/0x470 [btrfs] btrfs_test_qgroups+0x2da/0x300 [btrfs] btrfs_run_sanity_tests.cold+0x5c/0xd5 [btrfs] init_btrfs_fs+0xec/0x154 [btrfs] do_one_initcall+0x87/0x2a0 do_init_module+0xdf/0x320 load_module+0x3006/0x3390 __do_sys_finit_module+0x113/0x1b0 do_syscall_64+0x35/0x80 entry_SYSCALL_64_after_hwframe+0x46/0xb0 Freed by task 78078: kasan_save_stack+0x1e/0x40 kasan_set_track+0x21/0x30 kasan_save_free_info+0x2a/0x40 ____kasan_slab_free+0x143/0x1b0 __kmem_cache_free+0xc8/0x330 btrfs_qgroup_account_extent+0x402/0x770 [btrfs] test_multiple_refs.constprop.0+0x243/0x470 [btrfs] btrfs_test_qgroups+0x2da/0x300 [btrfs] btrfs_run_sanity_tests.cold+0x5c/0xd5 [btrfs] init_btrfs_fs+0xec/0x154 [btrfs] do_one_initcall+0x87/0x2a0 do_init_module+0xdf/0x320 load_module+0x3006/0x3390 __do_sys_finit_module+0x113/0x1b0 do_syscall_64+0x35/0x80 entry_SYSCALL_64_after_hwframe+0x46/0xb0 Since the ulist already freed after update the qgroup refcnt, the ulist should be set to NULL before the next reuse the variable. Fixes: 442244c96332 ("btrfs: qgroup: Switch self test to extent-oriented qgroup mechanism.") Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> --- fs/btrfs/tests/qgroup-tests.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/fs/btrfs/tests/qgroup-tests.c b/fs/btrfs/tests/qgroup-tests.c index eee1e4459541..4172bef5b4a1 100644 --- a/fs/btrfs/tests/qgroup-tests.c +++ b/fs/btrfs/tests/qgroup-tests.c @@ -347,6 +347,9 @@ static int test_multiple_refs(struct btrfs_root *root, return ret; } + old_roots = NULL; + new_roots = NULL; + if (btrfs_verify_qgroup_counts(fs_info, BTRFS_FS_TREE_OBJECTID, nodesize, nodesize)) { test_err("qgroup counts didn't match expected values"); @@ -380,6 +383,9 @@ static int test_multiple_refs(struct btrfs_root *root, return ret; } + old_roots = NULL; + new_roots = NULL; + if (btrfs_verify_qgroup_counts(fs_info, BTRFS_FS_TREE_OBJECTID, nodesize, 0)) { test_err("qgroup counts didn't match expected values"); -- 2.31.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] btrfs: Fix uaf of the ulist in test_multiple_refs() 2022-11-01 2:53 ` [PATCH 2/3] btrfs: Fix uaf of the ulist in test_multiple_refs() Zhang Xiaoxu @ 2022-11-01 8:18 ` Filipe Manana 0 siblings, 0 replies; 9+ messages in thread From: Filipe Manana @ 2022-11-01 8:18 UTC (permalink / raw) To: Zhang Xiaoxu; +Cc: linux-btrfs, clm, josef, dsterba On Tue, Nov 01, 2022 at 10:53:55AM +0800, Zhang Xiaoxu wrote: > There is a use-after-free report when do sanity tests: > > BUG: KASAN: use-after-free in ulist_free+0x25/0xa0 [btrfs] > Read of size 8 at addr ffff888296ab4748 by task insmod/78078 > > CPU: 7 PID: 78078 Comm: insmod Tainted: G W 6.1.0-rc2+ #5 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-1.fc33 04/01/2014 > Call Trace: > <TASK> > dump_stack_lvl+0x34/0x44 > print_report+0x171/0x472 > kasan_report+0xb7/0x140 > ulist_free+0x25/0xa0 [btrfs] > test_multiple_refs.constprop.0+0x411/0x470 [btrfs] > btrfs_test_qgroups+0x2da/0x300 [btrfs] > btrfs_run_sanity_tests.cold+0x5c/0xd5 [btrfs] > init_btrfs_fs+0xec/0x154 [btrfs] > do_one_initcall+0x87/0x2a0 > do_init_module+0xdf/0x320 > load_module+0x3006/0x3390 > __do_sys_finit_module+0x113/0x1b0 > do_syscall_64+0x35/0x80 > entry_SYSCALL_64_after_hwframe+0x46/0xb0 > > Allocated by task 78078: > kasan_save_stack+0x1e/0x40 > kasan_set_track+0x21/0x30 > __kasan_kmalloc+0x7a/0x90 > ulist_alloc+0x55/0xe0 [btrfs] > btrfs_find_all_roots_safe+0x9d/0x1c0 [btrfs] > test_multiple_refs.constprop.0+0x1e0/0x470 [btrfs] > btrfs_test_qgroups+0x2da/0x300 [btrfs] > btrfs_run_sanity_tests.cold+0x5c/0xd5 [btrfs] > init_btrfs_fs+0xec/0x154 [btrfs] > do_one_initcall+0x87/0x2a0 > do_init_module+0xdf/0x320 > load_module+0x3006/0x3390 > __do_sys_finit_module+0x113/0x1b0 > do_syscall_64+0x35/0x80 > entry_SYSCALL_64_after_hwframe+0x46/0xb0 > > Freed by task 78078: > kasan_save_stack+0x1e/0x40 > kasan_set_track+0x21/0x30 > kasan_save_free_info+0x2a/0x40 > ____kasan_slab_free+0x143/0x1b0 > __kmem_cache_free+0xc8/0x330 > btrfs_qgroup_account_extent+0x402/0x770 [btrfs] > test_multiple_refs.constprop.0+0x243/0x470 [btrfs] > btrfs_test_qgroups+0x2da/0x300 [btrfs] > btrfs_run_sanity_tests.cold+0x5c/0xd5 [btrfs] > init_btrfs_fs+0xec/0x154 [btrfs] > do_one_initcall+0x87/0x2a0 > do_init_module+0xdf/0x320 > load_module+0x3006/0x3390 > __do_sys_finit_module+0x113/0x1b0 > do_syscall_64+0x35/0x80 > entry_SYSCALL_64_after_hwframe+0x46/0xb0 > > Since the ulist already freed after update the qgroup refcnt, the ulist > should be set to NULL before the next reuse the variable. > > Fixes: 442244c96332 ("btrfs: qgroup: Switch self test to extent-oriented qgroup mechanism.") > Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> > --- > fs/btrfs/tests/qgroup-tests.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/fs/btrfs/tests/qgroup-tests.c b/fs/btrfs/tests/qgroup-tests.c > index eee1e4459541..4172bef5b4a1 100644 > --- a/fs/btrfs/tests/qgroup-tests.c > +++ b/fs/btrfs/tests/qgroup-tests.c > @@ -347,6 +347,9 @@ static int test_multiple_refs(struct btrfs_root *root, > return ret; > } > > + old_roots = NULL; > + new_roots = NULL; This could be addressed by having error paths not doing unnecessary free of the new_roots list. > + > if (btrfs_verify_qgroup_counts(fs_info, BTRFS_FS_TREE_OBJECTID, > nodesize, nodesize)) { > test_err("qgroup counts didn't match expected values"); > @@ -380,6 +383,9 @@ static int test_multiple_refs(struct btrfs_root *root, > return ret; > } > > + old_roots = NULL; > + new_roots = NULL; This one isn't needed. As after the btrfs_verify_qgroup_counts() calls there's nothing using or freeing the ulist variables. The way I addressed it in my patchset: From: Filipe Manana <fdmanana@suse.com> Date: Fri, 14 Oct 2022 16:56:29 +0100 Subject: [PATCH 04/18] btrfs: remove pointless ulist frees in error paths of qgroup self tests Several places in the qgroup self tests follow the pattern of freeing the ulist pointer they passed to btrfs_find_all_roots() if the call to that function returned an error. That is pointless because that function always frees the ulist in case it returns an error. So remove those calls to reduce the code size. Signed-off-by: Filipe Manana <fdmanana@suse.com> --- fs/btrfs/tests/qgroup-tests.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/fs/btrfs/tests/qgroup-tests.c b/fs/btrfs/tests/qgroup-tests.c index 96a70ce36f79..65b65d55d1f6 100644 --- a/fs/btrfs/tests/qgroup-tests.c +++ b/fs/btrfs/tests/qgroup-tests.c @@ -227,7 +227,6 @@ static int test_no_shared_qgroup(struct btrfs_root *root, */ ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &old_roots, false); if (ret) { - ulist_free(old_roots); test_err("couldn't find old roots: %d", ret); return ret; } @@ -242,7 +241,6 @@ static int test_no_shared_qgroup(struct btrfs_root *root, ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false); if (ret) { ulist_free(old_roots); - ulist_free(new_roots); test_err("couldn't find old roots: %d", ret); return ret; } @@ -254,17 +252,18 @@ static int test_no_shared_qgroup(struct btrfs_root *root, return ret; } + /* btrfs_qgroup_account_extent() always frees the ulists passed to it. */ + old_roots = NULL; + new_roots = NULL; + if (btrfs_verify_qgroup_counts(fs_info, BTRFS_FS_TREE_OBJECTID, nodesize, nodesize)) { test_err("qgroup counts didn't match expected values"); return -EINVAL; } - old_roots = NULL; - new_roots = NULL; ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &old_roots, false); if (ret) { - ulist_free(old_roots); test_err("couldn't find old roots: %d", ret); return ret; } @@ -278,7 +277,6 @@ static int test_no_shared_qgroup(struct btrfs_root *root, ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false); if (ret) { ulist_free(old_roots); - ulist_free(new_roots); test_err("couldn't find old roots: %d", ret); return ret; } @@ -328,7 +326,6 @@ static int test_multiple_refs(struct btrfs_root *root, ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &old_roots, false); if (ret) { - ulist_free(old_roots); test_err("couldn't find old roots: %d", ret); return ret; } @@ -343,7 +340,6 @@ static int test_multiple_refs(struct btrfs_root *root, ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false); if (ret) { ulist_free(old_roots); - ulist_free(new_roots); test_err("couldn't find old roots: %d", ret); return ret; } @@ -363,7 +359,6 @@ static int test_multiple_refs(struct btrfs_root *root, ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &old_roots, false); if (ret) { - ulist_free(old_roots); test_err("couldn't find old roots: %d", ret); return ret; } @@ -378,7 +373,6 @@ static int test_multiple_refs(struct btrfs_root *root, ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false); if (ret) { ulist_free(old_roots); - ulist_free(new_roots); test_err("couldn't find old roots: %d", ret); return ret; } @@ -404,7 +398,6 @@ static int test_multiple_refs(struct btrfs_root *root, ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &old_roots, false); if (ret) { - ulist_free(old_roots); test_err("couldn't find old roots: %d", ret); return ret; } @@ -419,7 +412,6 @@ static int test_multiple_refs(struct btrfs_root *root, ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false); if (ret) { ulist_free(old_roots); - ulist_free(new_roots); test_err("couldn't find old roots: %d", ret); return ret; } -- 2.35.1 > + > if (btrfs_verify_qgroup_counts(fs_info, BTRFS_FS_TREE_OBJECTID, > nodesize, 0)) { > test_err("qgroup counts didn't match expected values"); > -- > 2.31.1 > ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] btrfs: Fix ulist memory leak in test_multiple_refs() 2022-11-01 2:53 [PATCH 0/3] Fix UAF and kmemleak when sanity test Zhang Xiaoxu 2022-11-01 2:53 ` [PATCH 1/3] btrfs: Fix wrong check in btrfs_free_dummy_root() Zhang Xiaoxu 2022-11-01 2:53 ` [PATCH 2/3] btrfs: Fix uaf of the ulist in test_multiple_refs() Zhang Xiaoxu @ 2022-11-01 2:53 ` Zhang Xiaoxu 2022-11-01 8:15 ` Filipe Manana 2 siblings, 1 reply; 9+ messages in thread From: Zhang Xiaoxu @ 2022-11-01 2:53 UTC (permalink / raw) To: linux-btrfs, zhangxiaoxu5, clm, josef, dsterba There are some meory leak report when do sanity tests: unreferenced object 0xffff888296ab40c0 (size 32): comm "insmod", pid 76176, jiffies 4307414230 (age 309.541s) hex dump (first 32 bytes): 01 00 00 00 00 00 00 00 90 8a b1 30 81 88 ff ff ...........0.... 90 8a b1 30 81 88 ff ff a0 8a b1 30 81 88 ff ff ...0.......0.... backtrace: [<00000000e59989d0>] kmalloc_trace+0x27/0xa0 [<00000000836cc910>] ulist_alloc+0x55/0xe0 [btrfs] [<00000000eb3781d2>] btrfs_find_all_roots_safe+0x9d/0x1c0 [btrfs] [<00000000c7fe3cc7>] test_no_shared_qgroup.constprop.0+0x1f2/0x350 [btrfs] [<00000000f6a6b761>] btrfs_test_qgroups+0x2c8/0x300 [btrfs] [<0000000047d4f295>] btrfs_run_sanity_tests.cold+0x5c/0xd5 [btrfs] [<00000000b09fac49>] init_btrfs_fs+0xec/0x154 [btrfs] [<000000002925cdf3>] do_one_initcall+0x87/0x2a0 [<00000000c5ed267e>] do_init_module+0xdf/0x320 [<000000005f972694>] load_module+0x3006/0x3390 [<0000000070c88d9a>] __do_sys_finit_module+0x113/0x1b0 [<00000000e7029c2b>] do_syscall_64+0x35/0x80 [<000000008ffc1dab>] entry_SYSCALL_64_after_hwframe+0x46/0xb0 unreferenced object 0xffff888130b18a80 (size 64): comm "insmod", pid 76176, jiffies 4307414230 (age 309.542s) hex dump (first 32 bytes): 05 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ c8 40 ab 96 82 88 ff ff c8 40 ab 96 82 88 ff ff .@.......@...... backtrace: [<00000000e59989d0>] kmalloc_trace+0x27/0xa0 [<00000000de27bc34>] ulist_add_merge.part.0+0x67/0x1f0 [btrfs] [<00000000fbdf3c21>] find_parent_nodes+0xd49/0x2880 [btrfs] [<00000000ba4d2155>] btrfs_find_all_roots_safe+0x120/0x1c0 [btrfs] [<00000000c7fe3cc7>] test_no_shared_qgroup.constprop.0+0x1f2/0x350 [btrfs] [<00000000f6a6b761>] btrfs_test_qgroups+0x2c8/0x300 [btrfs] [<0000000047d4f295>] btrfs_run_sanity_tests.cold+0x5c/0xd5 [btrfs] [<00000000b09fac49>] init_btrfs_fs+0xec/0x154 [btrfs] [<000000002925cdf3>] do_one_initcall+0x87/0x2a0 [<00000000c5ed267e>] do_init_module+0xdf/0x320 [<000000005f972694>] load_module+0x3006/0x3390 [<0000000070c88d9a>] __do_sys_finit_module+0x113/0x1b0 [<00000000e7029c2b>] do_syscall_64+0x35/0x80 [<000000008ffc1dab>] entry_SYSCALL_64_after_hwframe+0x46/0xb0 unreferenced object 0xffff88812ae83100 (size 32): comm "insmod", pid 77664, jiffies 4307625383 (age 98.396s) hex dump (first 32 bytes): 01 00 00 00 00 00 00 00 10 f7 7e 23 81 88 ff ff ..........~#.... 10 f7 7e 23 81 88 ff ff 20 f7 7e 23 81 88 ff ff ..~#.... .~#.... backtrace: [<00000000e59989d0>] kmalloc_trace+0x27/0xa0 [<00000000836cc910>] ulist_alloc+0x55/0xe0 [btrfs] [<00000000eb3781d2>] btrfs_find_all_roots_safe+0x9d/0x1c0 [btrfs] [<00000000cacfcca2>] test_multiple_refs.constprop.0+0x1e0/0x470 [btrfs] [<000000008613167f>] btrfs_test_qgroups+0x2da/0x300 [btrfs] [<0000000047d4f295>] btrfs_run_sanity_tests.cold+0x5c/0xd5 [btrfs] [<00000000b09fac49>] init_btrfs_fs+0xec/0x154 [btrfs] [<000000002925cdf3>] do_one_initcall+0x87/0x2a0 [<00000000c5ed267e>] do_init_module+0xdf/0x320 [<000000005f972694>] load_module+0x3006/0x3390 [<0000000070c88d9a>] __do_sys_finit_module+0x113/0x1b0 [<00000000e7029c2b>] do_syscall_64+0x35/0x80 [<000000008ffc1dab>] entry_SYSCALL_64_after_hwframe+0x46/0xb0 Let's free the ulist memory when the no shared qgroup tests finish. Fixes: 442244c96332 ("btrfs: qgroup: Switch self test to extent-oriented qgroup mechanism.") Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> --- fs/btrfs/tests/qgroup-tests.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/fs/btrfs/tests/qgroup-tests.c b/fs/btrfs/tests/qgroup-tests.c index 4172bef5b4a1..043b139cfb8c 100644 --- a/fs/btrfs/tests/qgroup-tests.c +++ b/fs/btrfs/tests/qgroup-tests.c @@ -266,8 +266,10 @@ static int test_no_shared_qgroup(struct btrfs_root *root, } ret = remove_extent_item(root, nodesize, nodesize); - if (ret) + if (ret) { + ulist_free(old_roots); return -EINVAL; + } ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false); if (ret) { @@ -329,8 +331,10 @@ static int test_multiple_refs(struct btrfs_root *root, ret = insert_normal_tree_ref(root, nodesize, nodesize, 0, BTRFS_FS_TREE_OBJECTID); - if (ret) + if (ret) { + ulist_free(old_roots); return ret; + } ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false); if (ret) { @@ -365,8 +369,10 @@ static int test_multiple_refs(struct btrfs_root *root, ret = add_tree_ref(root, nodesize, nodesize, 0, BTRFS_FIRST_FREE_OBJECTID); - if (ret) + if (ret) { + ulist_free(old_roots); return ret; + } ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false); if (ret) { @@ -407,8 +413,10 @@ static int test_multiple_refs(struct btrfs_root *root, ret = remove_extent_ref(root, nodesize, nodesize, 0, BTRFS_FIRST_FREE_OBJECTID); - if (ret) + if (ret) { + ulist_free(old_roots); return ret; + } ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false); if (ret) { -- 2.31.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] btrfs: Fix ulist memory leak in test_multiple_refs() 2022-11-01 2:53 ` [PATCH 3/3] btrfs: Fix ulist memory leak " Zhang Xiaoxu @ 2022-11-01 8:15 ` Filipe Manana 0 siblings, 0 replies; 9+ messages in thread From: Filipe Manana @ 2022-11-01 8:15 UTC (permalink / raw) To: Zhang Xiaoxu; +Cc: linux-btrfs, clm, josef, dsterba On Tue, Nov 01, 2022 at 10:53:56AM +0800, Zhang Xiaoxu wrote: > There are some meory leak report when do sanity tests: > > unreferenced object 0xffff888296ab40c0 (size 32): > comm "insmod", pid 76176, jiffies 4307414230 (age 309.541s) > hex dump (first 32 bytes): > 01 00 00 00 00 00 00 00 90 8a b1 30 81 88 ff ff ...........0.... > 90 8a b1 30 81 88 ff ff a0 8a b1 30 81 88 ff ff ...0.......0.... > backtrace: > [<00000000e59989d0>] kmalloc_trace+0x27/0xa0 > [<00000000836cc910>] ulist_alloc+0x55/0xe0 [btrfs] > [<00000000eb3781d2>] btrfs_find_all_roots_safe+0x9d/0x1c0 [btrfs] > [<00000000c7fe3cc7>] test_no_shared_qgroup.constprop.0+0x1f2/0x350 [btrfs] > [<00000000f6a6b761>] btrfs_test_qgroups+0x2c8/0x300 [btrfs] > [<0000000047d4f295>] btrfs_run_sanity_tests.cold+0x5c/0xd5 [btrfs] > [<00000000b09fac49>] init_btrfs_fs+0xec/0x154 [btrfs] > [<000000002925cdf3>] do_one_initcall+0x87/0x2a0 > [<00000000c5ed267e>] do_init_module+0xdf/0x320 > [<000000005f972694>] load_module+0x3006/0x3390 > [<0000000070c88d9a>] __do_sys_finit_module+0x113/0x1b0 > [<00000000e7029c2b>] do_syscall_64+0x35/0x80 > [<000000008ffc1dab>] entry_SYSCALL_64_after_hwframe+0x46/0xb0 > unreferenced object 0xffff888130b18a80 (size 64): > comm "insmod", pid 76176, jiffies 4307414230 (age 309.542s) > hex dump (first 32 bytes): > 05 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > c8 40 ab 96 82 88 ff ff c8 40 ab 96 82 88 ff ff .@.......@...... > backtrace: > [<00000000e59989d0>] kmalloc_trace+0x27/0xa0 > [<00000000de27bc34>] ulist_add_merge.part.0+0x67/0x1f0 [btrfs] > [<00000000fbdf3c21>] find_parent_nodes+0xd49/0x2880 [btrfs] > [<00000000ba4d2155>] btrfs_find_all_roots_safe+0x120/0x1c0 [btrfs] > [<00000000c7fe3cc7>] test_no_shared_qgroup.constprop.0+0x1f2/0x350 [btrfs] > [<00000000f6a6b761>] btrfs_test_qgroups+0x2c8/0x300 [btrfs] > [<0000000047d4f295>] btrfs_run_sanity_tests.cold+0x5c/0xd5 [btrfs] > [<00000000b09fac49>] init_btrfs_fs+0xec/0x154 [btrfs] > [<000000002925cdf3>] do_one_initcall+0x87/0x2a0 > [<00000000c5ed267e>] do_init_module+0xdf/0x320 > [<000000005f972694>] load_module+0x3006/0x3390 > [<0000000070c88d9a>] __do_sys_finit_module+0x113/0x1b0 > [<00000000e7029c2b>] do_syscall_64+0x35/0x80 > [<000000008ffc1dab>] entry_SYSCALL_64_after_hwframe+0x46/0xb0 > unreferenced object 0xffff88812ae83100 (size 32): > comm "insmod", pid 77664, jiffies 4307625383 (age 98.396s) > hex dump (first 32 bytes): > 01 00 00 00 00 00 00 00 10 f7 7e 23 81 88 ff ff ..........~#.... > 10 f7 7e 23 81 88 ff ff 20 f7 7e 23 81 88 ff ff ..~#.... .~#.... > backtrace: > [<00000000e59989d0>] kmalloc_trace+0x27/0xa0 > [<00000000836cc910>] ulist_alloc+0x55/0xe0 [btrfs] > [<00000000eb3781d2>] btrfs_find_all_roots_safe+0x9d/0x1c0 [btrfs] > [<00000000cacfcca2>] test_multiple_refs.constprop.0+0x1e0/0x470 [btrfs] > [<000000008613167f>] btrfs_test_qgroups+0x2da/0x300 [btrfs] > [<0000000047d4f295>] btrfs_run_sanity_tests.cold+0x5c/0xd5 [btrfs] > [<00000000b09fac49>] init_btrfs_fs+0xec/0x154 [btrfs] > [<000000002925cdf3>] do_one_initcall+0x87/0x2a0 > [<00000000c5ed267e>] do_init_module+0xdf/0x320 > [<000000005f972694>] load_module+0x3006/0x3390 > [<0000000070c88d9a>] __do_sys_finit_module+0x113/0x1b0 > [<00000000e7029c2b>] do_syscall_64+0x35/0x80 > [<000000008ffc1dab>] entry_SYSCALL_64_after_hwframe+0x46/0xb0 > > Let's free the ulist memory when the no shared qgroup tests finish. > > Fixes: 442244c96332 ("btrfs: qgroup: Switch self test to extent-oriented qgroup mechanism.") > Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> There's still one ulist leak missing. I actually had a similar patch here that is part of a much larger patchset but I hadn't sent it yet. So find the difference: From: Filipe Manana <fdmanana@suse.com> Date: Fri, 14 Oct 2022 16:37:10 +0100 Subject: [PATCH 03/18] btrfs: fix ulist leaks in error paths of qgroup self tests In the test_no_shared_qgroup() and test_multiple_refs() qgroup self tests, if we fail to add the tree ref, remove the extent item or remove the extent ref, we are returning from the test function without freeing the "old_roots" ulist that was allocated by the previous calls to btrfs_find_all_roots(). Fix that by calling ulist_free() before returning. Fixes: 442244c96332 ("btrfs: qgroup: Switch self test to extent-oriented qgroup mechanism.") Signed-off-by: Filipe Manana <fdmanana@suse.com> --- fs/btrfs/tests/qgroup-tests.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/fs/btrfs/tests/qgroup-tests.c b/fs/btrfs/tests/qgroup-tests.c index 94b04f10f61c..96a70ce36f79 100644 --- a/fs/btrfs/tests/qgroup-tests.c +++ b/fs/btrfs/tests/qgroup-tests.c @@ -234,8 +234,10 @@ static int test_no_shared_qgroup(struct btrfs_root *root, ret = insert_normal_tree_ref(root, nodesize, nodesize, 0, BTRFS_FS_TREE_OBJECTID); - if (ret) + if (ret) { + ulist_free(old_roots); return ret; + } ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false); if (ret) { @@ -268,8 +270,10 @@ static int test_no_shared_qgroup(struct btrfs_root *root, } ret = remove_extent_item(root, nodesize, nodesize); - if (ret) + if (ret) { + ulist_free(old_roots); return -EINVAL; + } ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false); if (ret) { @@ -331,8 +335,10 @@ static int test_multiple_refs(struct btrfs_root *root, ret = insert_normal_tree_ref(root, nodesize, nodesize, 0, BTRFS_FS_TREE_OBJECTID); - if (ret) + if (ret) { + ulist_free(old_roots); return ret; + } ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false); if (ret) { @@ -364,8 +370,10 @@ static int test_multiple_refs(struct btrfs_root *root, ret = add_tree_ref(root, nodesize, nodesize, 0, BTRFS_FIRST_FREE_OBJECTID); - if (ret) + if (ret) { + ulist_free(old_roots); return ret; + } ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false); if (ret) { @@ -403,8 +411,10 @@ static int test_multiple_refs(struct btrfs_root *root, ret = remove_extent_ref(root, nodesize, nodesize, 0, BTRFS_FIRST_FREE_OBJECTID); - if (ret) + if (ret) { + ulist_free(old_roots); return ret; + } ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false); if (ret) { -- 2.35.1 > --- > fs/btrfs/tests/qgroup-tests.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/fs/btrfs/tests/qgroup-tests.c b/fs/btrfs/tests/qgroup-tests.c > index 4172bef5b4a1..043b139cfb8c 100644 > --- a/fs/btrfs/tests/qgroup-tests.c > +++ b/fs/btrfs/tests/qgroup-tests.c > @@ -266,8 +266,10 @@ static int test_no_shared_qgroup(struct btrfs_root *root, > } > > ret = remove_extent_item(root, nodesize, nodesize); > - if (ret) > + if (ret) { > + ulist_free(old_roots); > return -EINVAL; > + } > > ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false); > if (ret) { > @@ -329,8 +331,10 @@ static int test_multiple_refs(struct btrfs_root *root, > > ret = insert_normal_tree_ref(root, nodesize, nodesize, 0, > BTRFS_FS_TREE_OBJECTID); > - if (ret) > + if (ret) { > + ulist_free(old_roots); > return ret; > + } > > ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false); > if (ret) { > @@ -365,8 +369,10 @@ static int test_multiple_refs(struct btrfs_root *root, > > ret = add_tree_ref(root, nodesize, nodesize, 0, > BTRFS_FIRST_FREE_OBJECTID); > - if (ret) > + if (ret) { > + ulist_free(old_roots); > return ret; > + } > > ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false); > if (ret) { > @@ -407,8 +413,10 @@ static int test_multiple_refs(struct btrfs_root *root, > > ret = remove_extent_ref(root, nodesize, nodesize, 0, > BTRFS_FIRST_FREE_OBJECTID); > - if (ret) > + if (ret) { > + ulist_free(old_roots); > return ret; > + } > > ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false); > if (ret) { > -- > 2.31.1 > ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-11-04 1:09 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-11-01 2:53 [PATCH 0/3] Fix UAF and kmemleak when sanity test Zhang Xiaoxu 2022-11-01 2:53 ` [PATCH 1/3] btrfs: Fix wrong check in btrfs_free_dummy_root() Zhang Xiaoxu 2022-11-01 4:10 ` Anand Jain 2022-11-03 16:44 ` David Sterba 2022-11-04 1:09 ` zhangxiaoxu (A) 2022-11-01 2:53 ` [PATCH 2/3] btrfs: Fix uaf of the ulist in test_multiple_refs() Zhang Xiaoxu 2022-11-01 8:18 ` Filipe Manana 2022-11-01 2:53 ` [PATCH 3/3] btrfs: Fix ulist memory leak " Zhang Xiaoxu 2022-11-01 8:15 ` Filipe Manana
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).