linux-kernel - Re: [PATCH] btrfs: ioctl: Fix memory leak on duplicated memory

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: <87ecrugy31.fsf@>
Date: Thu, 25 Sep 2025 20:56:50 +0200
From: Miquel Sabaté Solà <mssola@...ola.com>
To: Filipe Manana <fdmanana@...nel.org>
Cc: Boris Burkov <boris@....io>,  linux-btrfs@...r.kernel.org,  clm@...com,
  dsterba@...e.com,  linux-kernel@...r.kernel.org
Subject: Re: [PATCH] btrfs: ioctl: Fix memory leak on duplicated memory

Miquel Sabaté Solà @ 2025-09-25 20:26 +02:

> Filipe Manana @ 2025-09-25 18:48 +01:
>
>> On Thu, Sep 25, 2025 at 6:25 PM Boris Burkov <boris@....io> wrote:
>>>
>>> On Thu, Sep 25, 2025 at 04:53:31PM +0200, Miquel Sabaté Solà wrote:
>>> > On 'btrfs_ioctl_qgroup_assign' we first duplicate the argument as
>>> > provided by the user, which is kfree'd in the end. But this was not the
>>> > case when allocating memory for 'prealloc'. In this case, if it somehow
>>> > failed, then the previous code would go directly into calling
>>> > 'mnt_drop_write_file', without freeing the string duplicated from the
>>> > user space.
>>> >
>>> > Signed-off-by: Miquel Sabaté Solà <mssola@...ola.com>
>>>
>>> LGTM, thanks for the fix!
>>>
>>> One thing though: I don't like the label names. I think with multiple
>>> cleanups the best way is to name each label with the cleanup it is for.
>>> Once you have some named ones, "out" feels unspecific, and encoding
>>> every single action like "out_sa_drop_write" doesn't scale as you add
>>> more cleanups, so it's just not a useful pattern. It's already quite
>>> clunky with just two.
>>>
>>> If you fixup the names, you can add:
>>>
>>> Reviewed-by: Boris Burkov <boris@....io>
>>>
>>> > ---
>>> >  fs/btrfs/ioctl.c | 3 ++-
>>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>>> >
>>> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>> > index 185bef0df1c2..00381fdbff9d 100644
>>> > --- a/fs/btrfs/ioctl.c
>>> > +++ b/fs/btrfs/ioctl.c
>>> > @@ -3740,7 +3740,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
>>> >               prealloc = kzalloc(sizeof(*prealloc), GFP_KERNEL);
>>> >               if (!prealloc) {
>>> >                       ret = -ENOMEM;
>>> > -                     goto drop_write;
>>> > +                     goto out_sa_drop_write;
>>> >               }
>>> >       }
>>> >
>>> > @@ -3775,6 +3775,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
>>> >
>>> >  out:
>>>
>>> call this free_prealloc
>>>
>>> >       kfree(prealloc);
>>> > +out_sa_drop_write:
>>>
>>> and this one free_args
>>
>>
>> Rather than adding yet one more label, which over time has proven
>> error prone, I'd rather have a single label.
>> Just the existing 'out' label and then the fix would be to replace the
>>
>> goto drop_write;
>>
>> with
>>
>> goto out;
>>
>> kfree() against a NULL pointer is safe.
>
> I wanted to keep it simple and just fix the issue at hand. Actually I
> found out about this as part of a larger refactoring involving cleanup
> functions [1], which would fix the amount of labels as well.

I clearly read too fast here. I applied your suggestion for v2. Sorry
for the noise!

>
> Hence, as David mentions on another email, I would handle cleaning up
> the amount of labels as part of another series.
>
>>
>> Also, missing a Fixes tag which should be:
>>
>> Fixes: 4addc1ffd67a ("btrfs: qgroup: preallocate memory before adding
>> a relation")
>
> I will add it as part of v2, thanks!
>
>>
>> Thanks.
>>
>>>
>>> >       kfree(sa);
>>> >  drop_write:
>>> >       mnt_drop_write_file(file);
>>> > --
>>> > 2.51.0
>>> >
>>>
>
> Thanks for the review,
> Miquel
>
> [1] https://lore.kernel.org/all/87plbh4qe9.fsf@/

Download attachment "signature.asc" of type "application/pgp-signature" (898 bytes)