* [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
* [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
* [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 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 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
* 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
* 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
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).