Re: [PATCH] btrfs: ref-verify: fix memory leaks

[Bragatheswaran Manickavel <bragathemanick0908@gmail.com>]

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.