[PATCH] media: venus: vdec: fixed possible memory leak issue

[PATCH] media: venus: vdec: fixed possible memory leak issue

[Ameer Hamza]

Fixed coverity warning by freeing the allocated memory before return

Addresses-Coverity: 1494120 ("Resource leak")

Signed-off-by: Ameer Hamza <amhamza.mgc@gmail.com>
---
 drivers/media/platform/qcom/venus/helpers.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index 84c3a511ec31..344a42853898 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -197,6 +197,7 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst)
 
 		id = ida_alloc_min(&inst->dpb_ids, VB2_MAX_FRAME, GFP_KERNEL);
 		if (id < 0) {
+			kfree(buf);
 			ret = id;
 			goto fail;
 		}
-- 
2.25.1

Re: [PATCH] media: venus: vdec: fixed possible memory leak issue

[Kieran Bingham]

Hi Ameer,

Quoting Ameer Hamza (2021-12-04 12:11:23)
> Fixed coverity warning by freeing the allocated memory before return
> 
> Addresses-Coverity: 1494120 ("Resource leak")
> 
> Signed-off-by: Ameer Hamza <amhamza.mgc@gmail.com>
> ---
>  drivers/media/platform/qcom/venus/helpers.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> index 84c3a511ec31..344a42853898 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -197,6 +197,7 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst)
>  
>                 id = ida_alloc_min(&inst->dpb_ids, VB2_MAX_FRAME, GFP_KERNEL);
>                 if (id < 0) {
> +                       kfree(buf);
>                         ret = id;
>                         goto fail;

Indeed, this is definitely a leak here.

Normally I think resources would be cleaned up in the fail path in a
situation like this.

That would then make sure that all paths out of this loop will free on
error.

If buf is null, kfree(null) is a valid noop call, so it will not
adversely affect the kzalloc() fail path.

Given that, I would suspect that a cleaner fix is to move the kfree()
from after  " if (!buf->va) { " to immediately after the fail label so
that both dma_alloc_attrs() and ida_alloc_min() failures are cleaned up
in the same way by the same error path.

That way, if anyone later adds another operation in this loop, it won't
get missed and will also clean up correctly.

Regards
--
Kieran

>                 }
> -- 
> 2.25.1
>

[PATCH v2] media: venus: vdec: fixed possible memory leak issue

[Ameer Hamza]

Fixed coverity warning by freeing the allocated memory before return

Addresses-Coverity: 1494120 ("Resource leak")

Signed-off-by: Ameer Hamza <amhamza.mgc@gmail.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

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

[Kieran Bingham]

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@gmail.com>

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

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.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
>

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

[Kieran Bingham]

Quoting Kieran Bingham (2021-12-06 09:55:54)
> 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@gmail.com>
> 
> Of course having suggested it, I believe this is the right fix so:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Oh - and we should probably add a fixes tag:

Fixes: 40d87aafee29 ("media: venus: vdec: decoded picture buffer handling during reconfig sequence")

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

[PATCH v3] media: venus: vdec: fixed possible memory leak issue

[Ameer Hamza]

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

Fixes: 40d87aafee29 ("media: venus: vdec: decoded picture buffer handling during reconfig sequence")

Signed-off-by: Ameer Hamza <amhamza.mgc@gmail.com>

---
Changes in v3:
Updated description and added fix tag
---
 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

Re: [PATCH v3] media: venus: vdec: fixed possible memory leak issue

[Kieran Bingham]

Quoting Ameer Hamza (2021-12-06 10:43:15)
> 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")
> 
> Fixes: 40d87aafee29 ("media: venus: vdec: decoded picture buffer handling during reconfig sequence")
> 

No need for blank lines between those tags, and when someone provides a
Reviewed-by tag, you can collect it into your patch for future versions
unless you feel you've modified the patch so much that it doesn't apply
anymore.

So this can still be added (no need to repost to add to this patch, I
believe the integration scripts likely pick up tags added to a patch,
but won't pick up ones added to previous versions).

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


> Signed-off-by: Ameer Hamza <amhamza.mgc@gmail.com>
> 
> ---
> Changes in v3:
> Updated description and added fix tag
> ---
>  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
>