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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <a76a57f8-aad8-46e2-bcc2-acfa35752352@gmail.com>
Date:   Mon, 13 Nov 2023 19:19:56 +0530
From:   Bragatheswaran Manickavel <bragathemanick0908@...il.com>
To:     Filipe Manana <fdmanana@...nel.org>
Cc:     clm@...com, josef@...icpanda.com, dsterba@...e.com,
        linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        syzbot+d66de4cbf532749df35f@...kaller.appspotmail.com
Subject: Re: [PATCH] btrfs: ref-verify: fix memory leaks


On 13/11/23 18:50, Filipe Manana wrote:
> On Sun, Nov 12, 2023 at 4:57 PM Bragatheswaran Manickavel
> <bragathemanick0908@...il.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@...kaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=d66de4cbf532749df35f
>> Signed-off-by: Bragatheswaran Manickavel <bragathemanick0908@...il.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.