diff mbox series

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

Message ID ca491aaa-cfc4-4a84-b7fc-b64f3adc6550@moroto.mountain (mailing list archive)
State Accepted
Delegated to: Hans Verkuil
Headers
Series [1/4] media: mediatek: vcodec: fix potential double free |

Commit Message

Dan Carpenter June 14, 2023, 1:05 p.m. UTC 
  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(+)

Comments

Nicolas Dufresne July 20, 2023, 7:36 p.m. UTC | #1 
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);
diff mbox series

Patch

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