linux-kernel - Re: [PATCH v2 1/2] of: overlay: Fix memory leak in of_overlay

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 5 Dec 2017 14:58:42 +0100
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Frank Rowand <frowand.list@...il.com>
Cc:     Geert Uytterhoeven <geert+renesas@...der.be>,
        Pantelis Antoniou <pantelis.antoniou@...sulko.com>,
        Rob Herring <robh+dt@...nel.org>,
        Colin King <colin.king@...onical.com>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] of: overlay: Fix memory leak in of_overlay_apply()
 error path

Hi Frank,

On Tue, Dec 5, 2017 at 2:45 PM, Frank Rowand <frowand.list@...il.com> wrote:
> On 12/05/17 03:01, Geert Uytterhoeven wrote:
>> On Tue, Dec 5, 2017 at 3:07 AM, Frank Rowand <frowand.list@...il.com> wrote:
>>> Also, the previous version of the patch, and the discussion around the resulting
>>> bug make me think that I should not have moved 'kfree(ovcs)' into
>>> free_overlay_changeset(), because that kfree is then not very visible in the
>>> error path of of_overlay_apply().  Could you remove 'kfree(ovcs)' from
>>> free_overlay_changeset(), and instead call it immediately after each call
>>> to free_overlay_changeset()?
>>
>> Actually I like that free_overlay_changeset() takes care of the deallocation,
>> especially in light of the kojectification op top from bbb-overlays, which
>> means you cannot just call kfree(ovcs) anymore (I know this won't go upstream
>> anytime soon, but I need overlay configfs for my development and testing).
>
> OK, knowing that kobjectification is being considered I am willing to leave the
> kfree(ovcs) where it is for now.
>
>> Perhaps the allocation of ovcs should be moved into free_overlay_changeset(),
>
>                                           I think this ^^^^^^^^^^^^^^^^^^^^^^^
> is a typo, and you meant init_overlay_changeset().

Yes it is.

>> and the latter being renamed to alloc_overlay_changeset()?
>> That way allocation and freeing become symmetrical.
>> It would move the allocation under the mutexes, though.
>
> I considered moving the kzalloc() into init_overlay_changeset() when I
> created it, but decided not to because the type of the first argument of
> init_overlay_changeset() would change from
>    struct overlay_changeset *
> to
>    struct overlay_changeset **,
> and usage of ovcs would become _slightly_ more ugly and complex in
> init_overlay_changeset().

I would let alloc_overlay_changeset() return struct overlay_changeset *
instead.

If you care about why it failed, it can return ERR_PTR(error) instead of
NULL ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds