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

[Filipe Manana]

On Sun, Nov 12, 2023 at 4:57 PM Bragatheswaran Manickavel
<bragathemanick0908@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=d66de4cbf532749df35f
> Signed-off-by: Bragatheswaran Manickavel <bragathemanick0908@xxxxxxxxx>
> ---
>  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
>
>