Re: [PATCH] drm/nouveau/ga102: Free resources on error in ga102_chan_new()

[Tim Gardner]

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.

> > 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