[PATCH 1/4] media: mediatek: vcodec: fix potential double free

[PATCH 1/4] media: mediatek: vcodec: fix potential double free

[Dan Carpenter]

The "lat_buf->private_data" needs to be set to NULL to prevent a
double free.  How this would happen is if vdec_msg_queue_init() failed
twice in a row and on the second time it failed earlier than on the
first time.

The vdec_msg_queue_init() function has a loop which does:
	for (i = 0; i < NUM_BUFFER_COUNT; i++) {

Each iteration initializes one element in the msg_queue->lat_buf[] array
and then the clean up function vdec_msg_queue_deinit() frees each
element of the msg_queue->lat_buf[] array.  This clean up code relies
on the assumption that every element is either initialized or zeroed.
Leaving a freed pointer which is non-zero breaks the assumption.

Fixes: b199fe46f35c ("media: mtk-vcodec: Add msg queue feature for lat and core architecture")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c b/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c
index f555341ae708..92ac82eb444e 100644
--- a/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c
+++ b/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c
@@ -231,6 +231,7 @@ void vdec_msg_queue_deinit(struct vdec_msg_queue *msg_queue,
 			mtk_vcodec_mem_free(ctx, mem);
 
 		kfree(lat_buf->private_data);
+		lat_buf->private_data = NULL;
 	}
 
 	cancel_work_sync(&msg_queue->core_work);
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/4] media: mediatek: vcodec: fix resource leaks in vdec_msg_queue_init()

[Dan Carpenter]

If we encounter any error in the vdec_msg_queue_init() then we need
to set "msg_queue->wdma_addr.size = 0;".  Normally, this is done
inside the vdec_msg_queue_deinit() function.  However, if the
first call to allocate &msg_queue->wdma_addr fails, then the
vdec_msg_queue_deinit() function is a no-op.  For that situation, just
set the size to zero explicitly and return.

There were two other error paths which did not clean up before returning.
Change those error paths to goto mem_alloc_err.

Fixes: b199fe46f35c ("media: mtk-vcodec: Add msg queue feature for lat and core architecture")
Fixes: 2f5d0aef37c6 ("media: mediatek: vcodec: support stateless AV1 decoder")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c b/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c
index 92ac82eb444e..be25d56712d8 100644
--- a/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c
+++ b/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c
@@ -307,6 +307,7 @@ int vdec_msg_queue_init(struct vdec_msg_queue *msg_queue,
 	err = mtk_vcodec_mem_alloc(ctx, &msg_queue->wdma_addr);
 	if (err) {
 		mtk_v4l2_err("failed to allocate wdma_addr buf");
+		msg_queue->wdma_addr.size = 0;
 		return -ENOMEM;
 	}
 	msg_queue->wdma_rptr_addr = msg_queue->wdma_addr.dma_addr;
@@ -338,14 +339,14 @@ int vdec_msg_queue_init(struct vdec_msg_queue *msg_queue,
 			err = mtk_vcodec_mem_alloc(ctx, &lat_buf->rd_mv_addr);
 			if (err) {
 				mtk_v4l2_err("failed to allocate rd_mv_addr buf[%d]", i);
-				return -ENOMEM;
+				goto mem_alloc_err;
 			}
 
 			lat_buf->tile_addr.size = VDEC_LAT_TILE_SZ;
 			err = mtk_vcodec_mem_alloc(ctx, &lat_buf->tile_addr);
 			if (err) {
 				mtk_v4l2_err("failed to allocate tile_addr buf[%d]", i);
-				return -ENOMEM;
+				goto mem_alloc_err;
 			}
 		}
 
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] media: mediatek: vcodec: fix resource leaks in vdec_msg_queue_init()

[Nicolas Dufresne]

Hi,

Le mercredi 14 juin 2023 à 16:06 +0300, Dan Carpenter a écrit :
> If we encounter any error in the vdec_msg_queue_init() then we need
> to set "msg_queue->wdma_addr.size = 0;".  Normally, this is done
> inside the vdec_msg_queue_deinit() function.  However, if the
> first call to allocate &msg_queue->wdma_addr fails, then the
> vdec_msg_queue_deinit() function is a no-op.  For that situation, just
> set the size to zero explicitly and return.
> 
> There were two other error paths which did not clean up before returning.
> Change those error paths to goto mem_alloc_err.
> 
> Fixes: b199fe46f35c ("media: mtk-vcodec: Add msg queue feature for lat and core architecture")
> Fixes: 2f5d0aef37c6 ("media: mediatek: vcodec: support stateless AV1 decoder")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

This change looks good to me, thanks again for your work.

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> ---
>  drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c b/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c
> index 92ac82eb444e..be25d56712d8 100644
> --- a/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c
> +++ b/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c
> @@ -307,6 +307,7 @@ int vdec_msg_queue_init(struct vdec_msg_queue *msg_queue,
>  	err = mtk_vcodec_mem_alloc(ctx, &msg_queue->wdma_addr);
>  	if (err) {
>  		mtk_v4l2_err("failed to allocate wdma_addr buf");
> +		msg_queue->wdma_addr.size = 0;
>  		return -ENOMEM;
>  	}
>  	msg_queue->wdma_rptr_addr = msg_queue->wdma_addr.dma_addr;
> @@ -338,14 +339,14 @@ int vdec_msg_queue_init(struct vdec_msg_queue *msg_queue,
>  			err = mtk_vcodec_mem_alloc(ctx, &lat_buf->rd_mv_addr);
>  			if (err) {
>  				mtk_v4l2_err("failed to allocate rd_mv_addr buf[%d]", i);
> -				return -ENOMEM;
> +				goto mem_alloc_err;
>  			}
>  
>  			lat_buf->tile_addr.size = VDEC_LAT_TILE_SZ;
>  			err = mtk_vcodec_mem_alloc(ctx, &lat_buf->tile_addr);
>  			if (err) {
>  				mtk_v4l2_err("failed to allocate tile_addr buf[%d]", i);
> -				return -ENOMEM;
> +				goto mem_alloc_err;
>  			}
>  		}
>  


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/4] media: mediatek: vcodec: Fix potential crash in mtk_vcodec_dbgfs_remove()

[Dan Carpenter]

The list iterator "dbgfs_inst" is always non-NULL.  This means that the
test for NULL inside the loop is unnecessary and it also means that the
test for NULL outside the loop will not work.  If we do not find the item
on the list with the correct the ctx_id then it will free invalid memory
leading to a crash.

Fixes: cd403a6a0419 ("media: mediatek: vcodec: Add a debugfs file to get different useful information")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 .../media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c
index 2151c3967684..2ebf68d33d57 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c
@@ -166,16 +166,13 @@ void mtk_vcodec_dbgfs_remove(struct mtk_vcodec_dev *vcodec_dev, int ctx_id)
 	struct mtk_vcodec_dbgfs_inst *dbgfs_inst;
 
 	list_for_each_entry(dbgfs_inst, &vcodec_dev->dbgfs.dbgfs_head, node) {
-		if (dbgfs_inst && dbgfs_inst->inst_id == ctx_id) {
+		if (dbgfs_inst->inst_id == ctx_id) {
 			vcodec_dev->dbgfs.inst_count--;
-			break;
+			list_del(&dbgfs_inst->node);
+			kfree(dbgfs_inst);
+			return;
 		}
 	}
-
-	if (dbgfs_inst) {
-		list_del(&dbgfs_inst->node);
-		kfree(dbgfs_inst);
-	}
 }
 EXPORT_SYMBOL_GPL(mtk_vcodec_dbgfs_remove);
 
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/4] media: mediatek: vcodec: Fix potential crash in mtk_vcodec_dbgfs_remove()

[Nicolas Dufresne]

Le mercredi 14 juin 2023 à 16:07 +0300, Dan Carpenter a écrit :
> The list iterator "dbgfs_inst" is always non-NULL.  This means that the
> test for NULL inside the loop is unnecessary and it also means that the
> test for NULL outside the loop will not work.  If we do not find the item
> on the list with the correct the ctx_id then it will free invalid memory
> leading to a crash.
> 
> Fixes: cd403a6a0419 ("media: mediatek: vcodec: Add a debugfs file to get different useful information")

Clearly better.

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  .../media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c
> index 2151c3967684..2ebf68d33d57 100644
> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c
> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c
> @@ -166,16 +166,13 @@ void mtk_vcodec_dbgfs_remove(struct mtk_vcodec_dev *vcodec_dev, int ctx_id)
>  	struct mtk_vcodec_dbgfs_inst *dbgfs_inst;
>  
>  	list_for_each_entry(dbgfs_inst, &vcodec_dev->dbgfs.dbgfs_head, node) {
> -		if (dbgfs_inst && dbgfs_inst->inst_id == ctx_id) {
> +		if (dbgfs_inst->inst_id == ctx_id) {
>  			vcodec_dev->dbgfs.inst_count--;
> -			break;
> +			list_del(&dbgfs_inst->node);
> +			kfree(dbgfs_inst);
> +			return;
>  		}
>  	}
> -
> -	if (dbgfs_inst) {
> -		list_del(&dbgfs_inst->node);
> -		kfree(dbgfs_inst);
> -	}
>  }
>  EXPORT_SYMBOL_GPL(mtk_vcodec_dbgfs_remove);
>  


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/4] media: mediatek: vcodec: Improve an error message

[Dan Carpenter]

This is intended to print the error code but there is a typo so it
prints IS_ERR() instead of PTR_ERR().

Fixes: 77f3b023f452 ("media: mediatek: vcodec: Add debugfs interface to get debug information")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
This driver has quite a bit of code to handle the case where DEBUG_FS is
turned off.  It's a bit over engineered.  With debugfs you're normally
just supposed to call the functions and ignore the errors.

But it's also harmless to do it this way.

 drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c
index 2ebf68d33d57..6957105492ae 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c
@@ -185,8 +185,8 @@ void mtk_vcodec_dbgfs_init(struct mtk_vcodec_dev *vcodec_dev, bool is_encode)
 	else
 		vcodec_dev->dbgfs.vcodec_root = debugfs_create_dir("vcodec-dec", NULL);
 	if (IS_ERR(vcodec_dev->dbgfs.vcodec_root))
-		dev_err(&vcodec_dev->plat_dev->dev, "create vcodec dir err:%d\n",
-			IS_ERR(vcodec_dev->dbgfs.vcodec_root));
+		dev_err(&vcodec_dev->plat_dev->dev, "create vcodec dir err:%ld\n",
+			PTR_ERR(vcodec_dev->dbgfs.vcodec_root));
 
 	vcodec_root = vcodec_dev->dbgfs.vcodec_root;
 	debugfs_create_x32("mtk_v4l2_dbg_level", 0644, vcodec_root, &mtk_v4l2_dbg_level);
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/4] media: mediatek: vcodec: Improve an error message

[Nicolas Dufresne]

Thanks,

Le mercredi 14 juin 2023 à 16:19 +0300, Dan Carpenter a écrit :
> This is intended to print the error code but there is a typo so it
> prints IS_ERR() instead of PTR_ERR().
> 
> Fixes: 77f3b023f452 ("media: mediatek: vcodec: Add debugfs interface to get debug information")

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> This driver has quite a bit of code to handle the case where DEBUG_FS is
> turned off.  It's a bit over engineered.  With debugfs you're normally
> just supposed to call the functions and ignore the errors.
> 
> But it's also harmless to do it this way.
> 
>  drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c
> index 2ebf68d33d57..6957105492ae 100644
> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c
> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c
> @@ -185,8 +185,8 @@ void mtk_vcodec_dbgfs_init(struct mtk_vcodec_dev *vcodec_dev, bool is_encode)
>  	else
>  		vcodec_dev->dbgfs.vcodec_root = debugfs_create_dir("vcodec-dec", NULL);
>  	if (IS_ERR(vcodec_dev->dbgfs.vcodec_root))
> -		dev_err(&vcodec_dev->plat_dev->dev, "create vcodec dir err:%d\n",
> -			IS_ERR(vcodec_dev->dbgfs.vcodec_root));
> +		dev_err(&vcodec_dev->plat_dev->dev, "create vcodec dir err:%ld\n",
> +			PTR_ERR(vcodec_dev->dbgfs.vcodec_root));
>  
>  	vcodec_root = vcodec_dev->dbgfs.vcodec_root;
>  	debugfs_create_x32("mtk_v4l2_dbg_level", 0644, vcodec_root, &mtk_v4l2_dbg_level);


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/4] media: mediatek: vcodec: fix potential double free

[Nicolas Dufresne]

Hi,

Just a reminder to Yunfei that this change was originally addressed to him, and
a review was to be expected. Over a month is a bit too long for fixes.

Le mercredi 14 juin 2023 à 16:05 +0300, Dan Carpenter a écrit :
> The "lat_buf->private_data" needs to be set to NULL to prevent a
> double free.  How this would happen is if vdec_msg_queue_init() failed
> twice in a row and on the second time it failed earlier than on the
> first time.
> 
> The vdec_msg_queue_init() function has a loop which does:
> 	for (i = 0; i < NUM_BUFFER_COUNT; i++) {
> 
> Each iteration initializes one element in the msg_queue->lat_buf[] array
> and then the clean up function vdec_msg_queue_deinit() frees each
> element of the msg_queue->lat_buf[] array.  This clean up code relies
> on the assumption that every element is either initialized or zeroed.
> Leaving a freed pointer which is non-zero breaks the assumption.
> 
> Fixes: b199fe46f35c ("media: mtk-vcodec: Add msg queue feature for lat and core architecture")

Unbalanced calls to deinit/init would be unfortunate, but I like keeping it safe
and aligned with the fact everything is clears to 0/null otherwise.

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c b/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c
> index f555341ae708..92ac82eb444e 100644
> --- a/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c
> +++ b/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c
> @@ -231,6 +231,7 @@ void vdec_msg_queue_deinit(struct vdec_msg_queue *msg_queue,
>  			mtk_vcodec_mem_free(ctx, mem);
>  
>  		kfree(lat_buf->private_data);
> +		lat_buf->private_data = NULL;
>  	}
>  
>  	cancel_work_sync(&msg_queue->core_work);


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel