Messages in this thread | | | Subject | Re: [PATCH] drm/nouveau/ga102: Free resources on error in ga102_chan_new() | From | Tim Gardner <> | Date | Tue, 21 Sep 2021 07:22:18 -0600 |
| |
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.
>> 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
| |