| Message ID | 20231112165648.10537-1-bragathemanick0908@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: ref-verify: fix memory leaks | expand |
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 > >
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.
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); 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); 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); 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); 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); goto out_unlock; } }
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(+)