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

[Bragatheswaran Manickavel]

On 13/11/23 18:50, Filipe Manana wrote:
> 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

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.