linux-kernel - Re: [PATCH v2] media: venus: vdec: fixed possible memory leak issue

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: <163878455464.147210.11589283989656931183@Monstersaurus>
Date:   Mon, 06 Dec 2021 09:55:54 +0000
From:   Kieran Bingham <kieran.bingham@...asonboard.com>
To:     Ameer Hamza <amhamza.mgc@...il.com>, agross@...nel.org,
        bjorn.andersson@...aro.org, mchehab@...nel.org,
        stanimir.varbanov@...aro.org
Cc:     linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, amhamza.mgc@...il.com
Subject: Re: [PATCH v2] media: venus: vdec: fixed possible memory leak issue

Hi Ameer,

Thank you for investigating the alternative suggestion I made.

Quoting Ameer Hamza (2021-12-04 20:55:04)
> Fixed coverity warning by freeing the allocated memory before return

We could probably say that fixing the coverity warning isn't so much the
target of the patch as fixing the memory leak. It's just helpful that
coverity spotted it for us.


I'd write:

The venus_helper_alloc_dpb_bufs() implementation allows an early return
on an error path when checking the id from ida_alloc_min() which would
not release the earlier buffer allocation.

Move the direct kfree() from the error checking of dma_alloc_attrs() to
the common fail path to ensure that allocations are released on all
error paths in this function.

> Addresses-Coverity: 1494120 ("Resource leak")
> 
> Signed-off-by: Ameer Hamza <amhamza.mgc@...il.com>

Of course having suggested it, I believe this is the right fix so:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@...asonboard.com>

> ---
> Changes in v2:
> move kfree() immediately after kfree() as suggested by Kieran Bingham
> ---
>  drivers/media/platform/qcom/venus/helpers.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> index 84c3a511ec31..0bca95d01650 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -189,7 +189,6 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst)
>                 buf->va = dma_alloc_attrs(dev, buf->size, &buf->da, GFP_KERNEL,
>                                           buf->attrs);
>                 if (!buf->va) {
> -                       kfree(buf);
>                         ret = -ENOMEM;
>                         goto fail;
>                 }
> @@ -209,6 +208,7 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst)
>         return 0;
>  
>  fail:
> +       kfree(buf);
>         venus_helper_free_dpb_bufs(inst);
>         return ret;
>  }
> -- 
> 2.25.1
>