Messages in this thread | | | Date | Mon, 13 Nov 2023 19:19:56 +0530 | Subject | Re: [PATCH] btrfs: ref-verify: fix memory leaks | From | Bragatheswaran Manickavel <> |
| |
On 13/11/23 18:50, Filipe Manana wrote: > On Sun, Nov 12, 2023 at 4:57 PM Bragatheswaran Manickavel > <bragathemanick0908@gmail.com> wrote: >> In btrfs_ref_tree_mod(), when !parent 're' was allocated >> through kmalloc(). In the following code, if an error occurs, >> the execution will be redirected to 'out' or 'out_unlock' and >> the function will be exited. However, on some of the paths, >> 're' are not deallocated and may lead to memory leaks. >> >> For example : lookup_block_entry() for 'be' returns null, the >> out label will be invoked. During that flow ref and ra was >> freed but not re, which can potentially lead to memleak >> >> Reported-and-tested-by: syzbot+d66de4cbf532749df35f@syzkaller.appspotmail.com >> Closes: https://syzkaller.appspot.com/bug?extid=d66de4cbf532749df35f >> Signed-off-by: Bragatheswaran Manickavel <bragathemanick0908@gmail.com> >> --- >> fs/btrfs/ref-verify.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/fs/btrfs/ref-verify.c b/fs/btrfs/ref-verify.c >> index 95d28497de7c..50b59b3dc474 100644 >> --- a/fs/btrfs/ref-verify.c >> +++ b/fs/btrfs/ref-verify.c >> @@ -791,6 +791,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info, >> dump_ref_action(fs_info, ra); >> kfree(ref); >> kfree(ra); >> + kfree(re); > Here it's fine, 're' was not yet added to the rbtree (be->roots). > >> goto out_unlock; >> } else if (be->num_refs == 0) { >> btrfs_err(fs_info, >> @@ -800,6 +801,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info, >> dump_ref_action(fs_info, ra); >> kfree(ref); >> kfree(ra); >> + kfree(re); > Same here. > >> goto out_unlock; >> } >> >> @@ -822,6 +824,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info, >> dump_ref_action(fs_info, ra); >> kfree(ref); >> kfree(ra); >> + kfree(re); > Here it's not ok. 're' was added to the rbtree, so you can't free it, > as later when accessing the tree, it will trigger a use-after-free > bug. > >> goto out_unlock; >> } >> exist->num_refs--; >> @@ -838,6 +841,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info, >> dump_ref_action(fs_info, ra); >> kfree(ref); >> kfree(ra); >> + kfree(re); > Same here, it will lead to a use-after-free. > >> goto out_unlock; >> } >> kfree(ref); >> @@ -849,6 +853,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info, >> dump_ref_action(fs_info, ra); >> kfree(ref); >> kfree(ra); >> + kfree(re); > Same here, it will lead to a use-after-free. > > Thanks. >> goto out_unlock; >> } >> } >> -- >> 2.34.1
Thanks Filipe for reviewing this!
Now, I understood why we shouldn't free 're' after it was added to the tree. In that case, can I send a new patch with first two changes.
| |