linux-kernel - Re: Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk

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:   Mon, 1 Jun 2020 13:41:48 +1000
From:   Ben Skeggs <skeggsb@...il.com>
To:     dinghao.liu@....edu.cn
Cc:     kjlu@....edu, David Airlie <airlied@...ux.ie>,
        ML nouveau <nouveau@...ts.freedesktop.org>,
        LKML <linux-kernel@...r.kernel.org>,
        ML dri-devel <dri-devel@...ts.freedesktop.org>,
        Ben Skeggs <bskeggs@...hat.com>, Markus.Elfring@....de
Subject: Re: Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new

On Mon, 1 Jun 2020 at 13:37, Ben Skeggs <skeggsb@...il.com> wrote:
>
> On Mon, 1 Jun 2020 at 13:27, <dinghao.liu@....edu.cn> wrote:
> >
> >
> > Hi Ben,
> >
> > > > When gk20a_clk_ctor() returns an error code, pointer "clk"
> > > > should be released. It's the same when gm20b_clk_new()
> > > > returns from elsewhere following this call.
> > > This shouldn't be necessary.  If a subdev constructor fails, and
> > > returns a pointer, the core will call the destructor to clean things
> > > up.
> > >
> >
> > I'm not familiar with the behavior of the caller of gm20b_clk_new().
> > If the subdev constructor fails, the core will check the pointer
> > (here is "pclk"), then it's ok and there is no bug (Do you mean
> > this?). If the core executes error handling code only according to
> > the error code, there may be a memory leak bug (the caller cannot
> > know if -ENOMEM comes from the failure of kzalloc or gk20a_clk_ctor).
> > If the core always calls the destructor as long as the constructor
> > fails (even if the kzalloc fails), we may have a double free bug.
> >
> > Would you like to give a more detailed explanation about the behavior
> > of the core?
> If there's *any* error, it'll check the pointer, if it's non-NULL,
> it'll call the destructor.  If kzalloc() fails, the pointer will be
> NULL, there's no double-free bug.  *every* subdev is written this way
> to avoid duplicating cleanup logic.
Actually, gm20b_clk_new_speedo0() may have a bug here if kzalloc()
fails as it doesn't overwrite the previous pointer from
gm20b_clk_new().  That whole ctor() sequence is written a little
strangely.

>
> Ben.
> >
> > Regards,
> > Dinghao