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

From: Nicolas Dufresne <nicolas@ndufresne.ca>
To: Dan Carpenter <dan.carpenter@linaro.org>,
	Yunfei Dong <yunfei.dong@mediatek.com>
Cc: Tiffany Lin <tiffany.lin@mediatek.com>,
	Andrew-CT Chen <andrew-ct.chen@mediatek.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	 Matthias Brugger <matthias.bgg@gmail.com>,
	AngeloGioacchino Del Regno
	<angelogioacchino.delregno@collabora.com>,
	 Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	linux-media@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	 linux-mediatek@lists.infradead.org,
	kernel-janitors@vger.kernel.org
Subject: Re: [PATCH 1/4] media: mediatek: vcodec: fix potential double free
Date: Thu, 20 Jul 2023 15:36:14 -0400	[thread overview]
Message-ID: <694dcc89ec4ff01ff240a1b924dac98fc1b64ac0.camel@ndufresne.ca> (raw)
In-Reply-To: <ca491aaa-cfc4-4a84-b7fc-b64f3adc6550@moroto.mountain>

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