linux-kernel - Re: [PATCH] media: mediatek: encoder: memset encoder structure data

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ddcd26ad-1bf1-4d14-b341-3c0e1e56cbf3@collabora.com>
Date: Tue, 15 Jul 2025 15:26:22 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Irui Wang <irui.wang@...iatek.com>,
 Hans Verkuil <hverkuil-cisco@...all.nl>,
 Mauro Carvalho Chehab <mchehab@...nel.org>,
 Matthias Brugger <matthias.bgg@...il.com>, nicolas.dufresne@...labora.com
Cc: Project_Global_Chrome_Upstream_Group@...iatek.com,
 linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
 Yunfei Dong <yunfei.dong@...iatek.com>,
 Longfei Wang <longfei.wang@...iatek.com>
Subject: Re: [PATCH] media: mediatek: encoder: memset encoder structure data

Il 15/07/25 10:15, Irui Wang ha scritto:
> Utilized memset to set all bytes of encoder structure to zero,
> this prevents any undefined behavior due to uninitialized use.
> 
> Signed-off-by: Irui Wang <irui.wang@...iatek.com>

This commit needs a Fixes tag, as you're fixing something important here.

Also, please clarify what is this undefined behavior that you've seen and
what problem are you trying to resolve by zeroing all those mem locations.

There's also more feedback, check below...

> ---
>   .../media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c  | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
> index a01dc25a7699..ecac1aec7215 100644
> --- a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
> +++ b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
> @@ -886,6 +886,7 @@ static int vb2ops_venc_start_streaming(struct vb2_queue *q, unsigned int count)
>   			return 0;
>   	}
>   
> +	memset(&param, 0, sizeof(param));

Have you considered doing, instead...

	struct venc_enc_param param = { 0 }; ?

>   	mtk_venc_set_param(ctx, &param);
>   	ret = venc_if_set_param(ctx, VENC_SET_PARAM_ENC, &param);
>   	if (ret) {
> @@ -1021,12 +1022,14 @@ static int mtk_venc_encode_header(void *priv)
>   	struct mtk_vcodec_mem bs_buf;
>   	struct venc_done_result enc_result;

  	struct venc_done_result enc_result = { 0 };

>   
> +	memset(&enc_result, 0, sizeof(enc_result));
>   	dst_buf = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
>   	if (!dst_buf) {
>   		mtk_v4l2_venc_dbg(1, ctx, "No dst buffer");
>   		return -EINVAL;
>   	}
>   
> +	memset(&bs_buf, 0, sizeof(bs_buf));
>   	bs_buf.va = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
>   	bs_buf.dma_addr = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
>   	bs_buf.size = (size_t)dst_buf->vb2_buf.planes[0].length;
> @@ -1143,6 +1146,7 @@ static void mtk_venc_worker(struct work_struct *work)
>   	struct venc_done_result enc_result;
>   	int ret, i;
>   
> +	memset(&enc_result, 0, sizeof(enc_result));

You should probably move this to before the first usage, instead.

>   	/* check dst_buf, dst_buf may be removed in device_run
>   	 * to stored encdoe header so we need check dst_buf and
>   	 * call job_finish here to prevent recursion
> @@ -1175,6 +1179,7 @@ static void mtk_venc_worker(struct work_struct *work)
>   		frm_buf.fb_addr[i].size =
>   				(size_t)src_buf->vb2_buf.planes[i].length;
>   	}
> +	memset(&bs_buf, 0, sizeof(bs_buf));

here it's fine to use memset, as there are multiple ways out before actually using
bs_buf.

Cheers,
Angelo

>   	bs_buf.va = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
>   	bs_buf.dma_addr = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
>   	bs_buf.size = (size_t)dst_buf->vb2_buf.planes[0].length;