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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <cd24187711932f4f533be79de305606bf93697f3.camel@mediatek.com>
Date: Wed, 16 Jul 2025 02:46:25 +0000
From: Irui Wang (王瑞) <Irui.Wang@...iatek.com>
To: "matthias.bgg@...il.com" <matthias.bgg@...il.com>, "mchehab@...nel.org"
	<mchehab@...nel.org>, AngeloGioacchino Del Regno
	<angelogioacchino.delregno@...labora.com>, "nicolas.dufresne@...labora.com"
	<nicolas.dufresne@...labora.com>, "hverkuil-cisco@...all.nl"
	<hverkuil-cisco@...all.nl>
CC: "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-media@...r.kernel.org"
	<linux-media@...r.kernel.org>, "linux-mediatek@...ts.infradead.org"
	<linux-mediatek@...ts.infradead.org>,
	Longfei Wang (王龙飞) <Longfei.Wang@...iatek.com>,
	Project_Global_Chrome_Upstream_Group
	<Project_Global_Chrome_Upstream_Group@...iatek.com>,
	Yunfei Dong (董云飞) <Yunfei.Dong@...iatek.com>
Subject: Re: [PATCH] media: mediatek: encoder: memset encoder structure data

Dear Angelo,

Thanks for your reviewing.

On Tue, 2025-07-15 at 15:26 +0200, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> 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.
OK, fix it in next patch.
> 
> 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 }; ?
OK, fix it in next patch.
> 
> >       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.
OK, fix it in next patch.
> 
> >       /* 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.
We will remove the bs_buf memset, because the bs_buf's variables are
set(va/dma_add/size) after definition.

Thanks!
> 
> 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;
> 
>