linux-kernel - Re: [PATCH] drm/nouveau/ga102: Free resources on error in ga102_chan

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9b778fe8-be29-f8aa-a40f-b640e9d31cc6@canonical.com>
Date:   Tue, 21 Sep 2021 07:22:18 -0600
From:   Tim Gardner <tim.gardner@...onical.com>
To:     Karol Herbst <kherbst@...hat.com>
Cc:     nouveau <nouveau@...ts.freedesktop.org>,
        Ben Skeggs <bskeggs@...hat.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/nouveau/ga102: Free resources on error in
 ga102_chan_new()



On 9/20/21 8:07 PM, Karol Herbst wrote:
> On Mon, Sep 20, 2021 at 8:17 PM Tim Gardner <tim.gardner@...onical.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@...hat.com>
>> Cc: David Airlie <airlied@...ux.ie>
>> Cc: Daniel Vetter <daniel@...ll.ch>
>> Cc: Karol Herbst <kherbst@...hat.com>
>> Cc: dri-devel@...ts.freedesktop.org
>> Cc: nouveau@...ts.freedesktop.org
>> Cc: linux-kernel@...r.kernel.org
>> Signed-off-by: Tim Gardner <tim.gardner@...onical.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