linux-kernel - Re: [PATCH v11 03/12] media: mediatek: jpeg: fix jpeg buffer layout

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <714f2e57521e990d77b80d23a9eb773ca878b07a.camel@mediatek.com>
Date: Thu, 25 Dec 2025 06:05:41 +0000
From: Kyrie Wu (吴晗) <Kyrie.Wu@...iatek.com>
To: "nicolas@...fresne.ca" <nicolas@...fresne.ca>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"mchehab@...nel.org" <mchehab@...nel.org>,
	Kyrie Wu (吴晗) <Kyrie.Wu@...iatek.com>,
	"conor+dt@...nel.org" <conor+dt@...nel.org>, "robh@...nel.org"
	<robh@...nel.org>, "hverkuil-cisco@...all.nl" <hverkuil-cisco@...all.nl>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "matthias.bgg@...il.com"
	<matthias.bgg@...il.com>, "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH v11 03/12] media: mediatek: jpeg: fix jpeg buffer layout

On Tue, 2025-12-16 at 16:22 -0500, Nicolas Dufresne wrote:
> Hi,
> 
> Le mardi 02 décembre 2025 à 17:47 +0800, Kyrie Wu a écrit :
> > For memory alloc operation of jpeg dst buffer: the mallocing
> > memory function interface use vb2_buffer as the base addr.
> > If structure mtk_jpeg_src_buf wants to be allocated to memory,
> > it needs to be placed vb2_v4l2_buffer at the starting position,
> > because structure vb2_buffer is at the starting position of
> > vb2_v4l2_buffer, and the allocated size is set to the size of
> > structure mtk_jpeg_src_buf, so as to ensure that structures
> > mtk_jpeg_src_buf, vb2_v4l2_buffer and vb2_buffer can all be
> > allocated memory.
> 
> Please correct the wording, "mallocing" is not a word, addr ->
> address. I tend
> to do the same, but refrain from giving the code a personality, the
> vb2_buffer
> have no will. This is overall complicated way to simply say:
> 
> 	Fix the size of the allocated mtk_jpeg_src_buf structure.
> 
> > 
> > Fixes: 5fb1c2361e56 ("mtk-jpegenc: add jpeg encode worker
> > interface")
> > 
> 
> Drop the blank line.
> 
> > Signed-off-by: Kyrie Wu <kyrie.wu@...iatek.com>
> > ---
> >  drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c | 2 +-
> >  drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.h | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> > b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> > index 0cf3dc5407e4..bd0afc93d491 100644
> > --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> > +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> > @@ -1099,7 +1099,7 @@ static int mtk_jpeg_queue_init(void *priv,
> > struct vb2_queue *src_vq,
> >  	dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> >  	dst_vq->io_modes = VB2_DMABUF | VB2_MMAP;
> >  	dst_vq->drv_priv = ctx;
> > -	dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> > +	dst_vq->buf_struct_size = sizeof(struct mtk_jpeg_src_buf);
> >  	dst_vq->ops = jpeg->variant->qops;
> >  	dst_vq->mem_ops = &vb2_dma_contig_memops;
> >  	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.h
> > b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.h
> > index 6be5cf30dea1..148fd41759b7 100644
> > --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.h
> > +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.h
> > @@ -85,10 +85,10 @@ struct mtk_jpeg_variant {
> >  };
> >  
> >  struct mtk_jpeg_src_buf {
> > -	u32 frame_num;
> >  	struct vb2_v4l2_buffer b;
> >  	struct list_head list;
> >  	u32 bs_size;
> > +	u32 frame_num;
> 
> I like the change, but this shouldn't be an issue if you use
> container_of, which
> is safer then a plain cast. Please review the code and make sure to
> use it. It
> may be confusing to include a cosmetic change in a fixes.
> 
> Nicolas

Dear Nicolas,

The driver requests memory space for mtk_jpeg_src_buf in the
function of mtk_jpeg_queue_init,
and it used struct vb2_v4l2_buffer b as the starting base address,
the parameter,frame_num, will not get a memory if we keep the old
structure. And a unknown memory would be get if we used container_of
to get the starting address of mtk_jpeg_src_buf.

Thanks.

Regards,
Kyrie.
> 
> >  	struct mtk_jpeg_dec_param dec_param;
> >  
> >  	struct mtk_jpeg_ctx *curr_ctx;