[PATCH] media: renesas: vsp1: fix CLU/LUT entity leaks upon pool allocation failure

[PATCH] media: renesas: vsp1: fix CLU/LUT entity leaks upon pool allocation failure

[Dawei Feng]

In vsp1_clu_create() and vsp1_lut_create(), VSP1 entities are initialized
prior to allocating the body pools. If vsp1_dl_body_pool_create() fails,
both functions return -ENOMEM directly. This skips the necessary cleanup
for the already initialized entities, leading to resource leaks.

Fix this by introducing a common error label to consolidate the error
handling. Route the pool allocation failures to this label to ensure
vsp1_entity_destroy() is properly invoked.

The bug was first flagged by an experimental analysis tool we are
developing for kernel memory-management bugs while analyzing
v6.13-rc1. The tool is still under development and is not yet publicly
available. Manual inspection confirms that both bugs are still present
in v7.1-rc6.

An x86_64 allyesconfig build of vsp1_clu.o and vsp1_lut.o showed no new
warnings. As we do not have access to a Renesas R-Car system with a
VSP1 instance to test with, and no available emulated platform exposes
one, no runtime testing was able to be performed.

Fixes: 5d7936b8e27d ("media: vsp1: Convert display lists to use new body pool")
Cc: stable@vger.kernel.org

Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
Signed-off-by: Dawei Feng <dawei.feng@seu.edu.cn>
---
 drivers/media/platform/renesas/vsp1/vsp1_clu.c | 13 +++++++++----
 drivers/media/platform/renesas/vsp1/vsp1_lut.c | 13 +++++++++----
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/renesas/vsp1/vsp1_clu.c b/drivers/media/platform/renesas/vsp1/vsp1_clu.c
index 04c466c4da81..46b90c7b955a 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_clu.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_clu.c
@@ -236,8 +236,10 @@ struct vsp1_clu *vsp1_clu_create(struct vsp1_device *vsp1)
 	 */
 	clu->pool = vsp1_dl_body_pool_create(clu->entity.vsp1, 3, CLU_SIZE + 1,
 					     0);
-	if (!clu->pool)
-		return ERR_PTR(-ENOMEM);
+	if (!clu->pool) {
+		ret = -ENOMEM;
+		goto error;
+	}
 
 	/* Initialize the control handler. */
 	v4l2_ctrl_handler_init(&clu->ctrls, 2);
@@ -249,11 +251,14 @@ struct vsp1_clu *vsp1_clu_create(struct vsp1_device *vsp1)
 	if (clu->ctrls.error) {
 		dev_err(vsp1->dev, "clu: failed to initialize controls\n");
 		ret = clu->ctrls.error;
-		vsp1_entity_destroy(&clu->entity);
-		return ERR_PTR(ret);
+		goto error;
 	}
 
 	v4l2_ctrl_handler_setup(&clu->ctrls);
 
 	return clu;
+
+error:
+	vsp1_entity_destroy(&clu->entity);
+	return ERR_PTR(ret);
 }
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_lut.c b/drivers/media/platform/renesas/vsp1/vsp1_lut.c
index 94bdedcc5c92..d508bcbc7a6e 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_lut.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_lut.c
@@ -195,8 +195,10 @@ struct vsp1_lut *vsp1_lut_create(struct vsp1_device *vsp1)
 	 * both the queued and pending dl entries.
 	 */
 	lut->pool = vsp1_dl_body_pool_create(vsp1, 3, LUT_SIZE, 0);
-	if (!lut->pool)
-		return ERR_PTR(-ENOMEM);
+	if (!lut->pool) {
+		ret = -ENOMEM;
+		goto error;
+	}
 
 	/* Initialize the control handler. */
 	v4l2_ctrl_handler_init(&lut->ctrls, 1);
@@ -207,11 +209,14 @@ struct vsp1_lut *vsp1_lut_create(struct vsp1_device *vsp1)
 	if (lut->ctrls.error) {
 		dev_err(vsp1->dev, "lut: failed to initialize controls\n");
 		ret = lut->ctrls.error;
-		vsp1_entity_destroy(&lut->entity);
-		return ERR_PTR(ret);
+		goto error;
 	}
 
 	v4l2_ctrl_handler_setup(&lut->ctrls);
 
 	return lut;
+
+error:
+	vsp1_entity_destroy(&lut->entity);
+	return ERR_PTR(ret);
 }
-- 
2.34.1

Re: [PATCH] media: renesas: vsp1: fix CLU/LUT entity leaks upon pool allocation failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] vsp1_hgo_create() and vsp1_hgt_create() trigger a kernel oops if vsp1_entity_init() fails, due to an error pointer dereference in vsp1_entity_destroy().
--

commit e81aa1a80df0db63e68e1a79f5ff07f7bd58f096
Author: Dawei Feng <dawei.feng@seu.edu.cn>

media: renesas: vsp1: fix CLU/LUT entity leaks upon pool allocation failure

This commit fixes resource leaks in vsp1_clu_create() and vsp1_lut_create()
by consolidating error handling. If vsp1_dl_body_pool_create() or control
initialization fails, it now uses a common error label to properly invoke
vsp1_entity_destroy().

> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_clu.c b/drivers/media/platform/renesas/vsp1/vsp1_clu.c
> index a6e4bcab5101a..b0b85a00957ac 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_clu.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_clu.c
[ ... ]
> @@ -248,11 +250,14 @@ struct vsp1_clu *vsp1_clu_create(struct vsp1_device *vsp1)
>  	if (clu->ctrls.error) {
>  		dev_err(vsp1->dev, "clu: failed to initialize controls\n");
>  		ret = clu->ctrls.error;
> -		vsp1_entity_destroy(&clu->entity);
> -		return ERR_PTR(ret);
> +		goto error;
>  	}
>  
>  	v4l2_ctrl_handler_setup(&clu->ctrls);
>  
>  	return clu;
> +
> +error:
> +	vsp1_entity_destroy(&clu->entity);

[Severity: High]
This is a pre-existing issue, but does calling vsp1_entity_destroy() risk
a kernel panic if the entity state is an error pointer?

In drivers/media/platform/renesas/vsp1/vsp1_entity.c:vsp1_entity_init(),
if state allocation fails, it assigns an ERR_PTR to entity->state:

drivers/media/platform/renesas/vsp1/vsp1_entity.c:vsp1_entity_init() {
    ...
    entity->state = __v4l2_subdev_state_alloc(&entity->subdev,
                                              "vsp1:state->lock", &key);
    if (IS_ERR(entity->state)) {
        media_entity_cleanup(&entity->subdev.entity);
        return PTR_ERR(entity->state);
    }
    ...
}

If vsp1_entity_destroy() is later called, it unconditionally frees the state:

drivers/media/platform/renesas/vsp1/vsp1_entity.c:vsp1_entity_destroy() {
    ...
    __v4l2_subdev_state_free(entity->state);
    ...
}

However, __v4l2_subdev_state_free() only checks for NULL, not IS_ERR():

drivers/media/v4l2-core/v4l2-subdev.c:__v4l2_subdev_state_free() {
    if (!state)
        return;

    mutex_destroy(&state->_lock);
    ...
}

This causes a blind dereference of the error pointer.

While this patch calls vsp1_entity_destroy() after vsp1_entity_init() has
succeeded (making the state pointer valid here), callers like vsp1_hgo_create()
and vsp1_hgt_create() handle vsp1_entity_init() failures by immediately calling
vsp1_entity_destroy().

Could this cause a crash if state allocation fails during probe?

> +	return ERR_PTR(ret);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604073654.1365717-1-dawei.feng@seu.edu.cn?part=1