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: <CACO55ts0kJaHr0QTzqy5r9Gq4vbV=8bMokuVHSCAqMVEOKAgDw@mail.gmail.com>
Date:   Tue, 21 Sep 2021 04:07:41 +0200
From:   Karol Herbst <kherbst@...hat.com>
To:     Tim Gardner <tim.gardner@...onical.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 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.

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