* [Nouveau] [PATCH] drm/nouveau/ga102: Free resources on error in ga102_chan_new()
@ 2021-09-20 18:16 Tim Gardner
2021-09-21 2:07 ` Karol Herbst
0 siblings, 1 reply; 4+ messages in thread
From: Tim Gardner @ 2021-09-20 18:16 UTC (permalink / raw)
To: nouveau
Cc: tim.gardner, Ben Skeggs, David Airlie, Daniel Vetter,
Karol Herbst, dri-devel, linux-kernel
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.
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Nouveau] [PATCH] drm/nouveau/ga102: Free resources on error in ga102_chan_new()
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
0 siblings, 1 reply; 4+ messages in thread
From: Karol Herbst @ 2021-09-21 2:07 UTC (permalink / raw)
To: Tim Gardner
Cc: nouveau, Ben Skeggs, David Airlie, Daniel Vetter, dri-devel, LKML
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.
> 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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Nouveau] [PATCH] drm/nouveau/ga102: Free resources on error in ga102_chan_new()
2021-09-21 2:07 ` Karol Herbst
@ 2021-09-21 13:22 ` Tim Gardner
2021-09-21 17:04 ` Karol Herbst
0 siblings, 1 reply; 4+ messages in thread
From: Tim Gardner @ 2021-09-21 13:22 UTC (permalink / raw)
To: Karol Herbst
Cc: nouveau, Ben Skeggs, David Airlie, Daniel Vetter, dri-devel, LKML
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Nouveau] [PATCH] drm/nouveau/ga102: Free resources on error in ga102_chan_new()
2021-09-21 13:22 ` Tim Gardner
@ 2021-09-21 17:04 ` Karol Herbst
0 siblings, 0 replies; 4+ messages in thread
From: Karol Herbst @ 2021-09-21 17:04 UTC (permalink / raw)
To: Tim Gardner
Cc: nouveau, Ben Skeggs, David Airlie, Daniel Vetter, dri-devel, LKML
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-09-23 14:32 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 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).