From: Karol Herbst <kherbst@redhat.com>
To: Tim Gardner <tim.gardner@canonical.com>
Cc: nouveau <nouveau@lists.freedesktop.org>,
Ben Skeggs <bskeggs@redhat.com>, David Airlie <airlied@linux.ie>,
Daniel Vetter <daniel@ffwll.ch>,
dri-devel <dri-devel@lists.freedesktop.org>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [Nouveau] [PATCH] drm/nouveau/ga102: Free resources on error in ga102_chan_new()
Date: Tue, 21 Sep 2021 19:04:52 +0200 [thread overview]
Message-ID: <CACO55tt-j6cPD0zT2Gk3fu2rNYk=h8=zy5bq8=EJ25f8QsXb6Q@mail.gmail.com> (raw)
In-Reply-To: <9b778fe8-be29-f8aa-a40f-b640e9d31cc6@canonical.com>
On Tue, Sep 21, 2021 at 3:22 PM Tim Gardner <tim.gardner@canonical.com> wrote:
>
>
>
> On 9/20/21 8:07 PM, Karol Herbst wrote:
> > On Mon, Sep 20, 2021 at 8:17 PM Tim Gardner <tim.gardner@canonical.com> wrote:
> >>
> >> Coverity complains of a resource leak in ga102_chan_new():
> >>
> >> CID 119637 (#7 of 7): Resource leak (RESOURCE_LEAK)
> >> 13. leaked_storage: Variable chan going out of scope leaks the storage it points to.
> >> 190 return ret;
> >>
> >> Fix this by freeing 'chan' in the error path.
> >>
> >
> > yeah, this is actually a false positive. I ran your patch through
> > kasan and got a use-after-free as we deallocate the passed in pointer
> > after calling the function pointer to the new function. One might
> > argue that the programming style isn't the best and we should be
> > explicit about freeing memory though.
> >
>
> So the caller of this constructor has to look at the error return code
> and decide whether the value stored in *pobject can be freed ? I guess
> if the caller initializes the value at *pobject to be NULL then it can
> kfree() regardless.
>
yeah, so *pobject is set, hence the caller freeing the object on
error. I am not a big fan of this to be honest, but Ben told me he has
a bigger rework of how all of this works pending anyway and I think we
should keep things like this in mind so it's easier for others to
understand if the code is actually correct or not.
Anyway, this all happens inside nvkm_ioctl_new. We have the call to
"oclass.ctor(&oclass, data, size, &object)" which ends up calling
ga102_chan_new, and on error we do "nvkm_object_del(&object)", which
does some cleanups and calls kfree.
> >> Cc: Ben Skeggs <bskeggs@redhat.com>
> >> Cc: David Airlie <airlied@linux.ie>
> >> Cc: Daniel Vetter <daniel@ffwll.ch>
> >> Cc: Karol Herbst <kherbst@redhat.com>
> >> Cc: dri-devel@lists.freedesktop.org
> >> Cc: nouveau@lists.freedesktop.org
> >> Cc: linux-kernel@vger.kernel.org
> >> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> >> ---
> >> .../gpu/drm/nouveau/nvkm/engine/fifo/ga102.c | 20 ++++++++++++-------
> >> 1 file changed, 13 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/ga102.c b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/ga102.c
> >> index f897bef13acf..4dbdfb53e65f 100644
> >> --- a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/ga102.c
> >> +++ b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/ga102.c
> >> @@ -175,19 +175,21 @@ ga102_chan_new(struct nvkm_device *device,
> >> }
> >> }
> >>
> >> - if (!chan->ctrl.runl)
> >> - return -ENODEV;
> >> + if (!chan->ctrl.runl) {
> >> + ret = -ENODEV;
> >> + goto free_chan;
> >> + }
> >>
> >> chan->ctrl.chan = nvkm_rd32(device, chan->ctrl.runl + 0x004) & 0xfffffff0;
> >> args->token = nvkm_rd32(device, chan->ctrl.runl + 0x008) & 0xffff0000;
> >>
> >> ret = nvkm_memory_new(device, NVKM_MEM_TARGET_INST, 0x1000, 0x1000, true, &chan->mthd);
> >> if (ret)
> >> - return ret;
> >> + goto free_chan;
> >>
> >> ret = nvkm_memory_new(device, NVKM_MEM_TARGET_INST, 0x1000, 0x1000, true, &chan->inst);
> >> if (ret)
> >> - return ret;
> >> + goto free_chan;
> >>
> >> nvkm_kmap(chan->inst);
> >> nvkm_wo32(chan->inst, 0x010, 0x0000face);
> >> @@ -209,11 +211,11 @@ ga102_chan_new(struct nvkm_device *device,
> >>
> >> ret = nvkm_memory_new(device, NVKM_MEM_TARGET_INST, 0x1000, 0x1000, true, &chan->user);
> >> if (ret)
> >> - return ret;
> >> + goto free_chan;
> >>
> >> ret = nvkm_memory_new(device, NVKM_MEM_TARGET_INST, 0x1000, 0x1000, true, &chan->runl);
> >> if (ret)
> >> - return ret;
> >> + goto free_chan;
> >>
> >> nvkm_kmap(chan->runl);
> >> nvkm_wo32(chan->runl, 0x00, 0x80030001);
> >> @@ -228,10 +230,14 @@ ga102_chan_new(struct nvkm_device *device,
> >>
> >> ret = nvkm_vmm_join(vmm, chan->inst);
> >> if (ret)
> >> - return ret;
> >> + goto free_chan;
> >>
> >> chan->vmm = nvkm_vmm_ref(vmm);
> >> return 0;
> >> +
> >> +free_chan:
> >> + kfree(chan);
> >> + return ret;
> >> }
> >>
> >> static const struct nvkm_device_oclass
> >> --
> >> 2.33.0
> >>
> >
>
> --
> -----------
> Tim Gardner
> Canonical, Inc
>
prev parent reply other threads:[~2021-09-21 17:05 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-20 18:16 [Nouveau] [PATCH] drm/nouveau/ga102: Free resources on error in ga102_chan_new() Tim Gardner
2021-09-21 2:07 ` Karol Herbst
2021-09-21 13:22 ` Tim Gardner
2021-09-21 17:04 ` Karol Herbst [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CACO55tt-j6cPD0zT2Gk3fu2rNYk=h8=zy5bq8=EJ25f8QsXb6Q@mail.gmail.com' \
--to=kherbst@redhat.com \
--cc=airlied@linux.ie \
--cc=bskeggs@redhat.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nouveau@lists.freedesktop.org \
--cc=tim.gardner@canonical.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).