On Tue, Sep 21, 2021 at 3:22 PM Tim Gardner <tim.gardner@xxxxxxxxxxxxx> wrote:
>
>
>
> On 9/20/21 8:07 PM, Karol Herbst wrote:
> > On Mon, Sep 20, 2021 at 8:17 PM Tim Gardner <tim.gardner@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
> >> Cc: David Airlie <airlied@xxxxxxxx>
> >> Cc: Daniel Vetter <daniel@xxxxxxxx>
> >> Cc: Karol Herbst <kherbst@xxxxxxxxxx>
> >> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> >> Cc: nouveau@xxxxxxxxxxxxxxxxxxxxx
> >> Cc: linux-kernel@xxxxxxxxxxxxxxx
> >> Signed-off-by: Tim Gardner <tim.gardner@xxxxxxxxxxxxx>
> >> ---
> >> .../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
>
