lkml.org 
[lkml]   [2023]   [Nov]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] btrfs: ref-verify: fix memory leaks
From

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.

\
 
 \ /
  Last update: 2023-11-20 13:58    [W:0.098 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site