Re: [PATCH] OOB read and write in mtk multiple places

[Alain Volmat <alain.volmat@foss.st.com>]

Hi,

On Tue, Jun 27, 2023 at 08:28:11PM +0800, sun yq wrote:
> When using V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, the number of planes is
> controlled by the user. Only checking the oob at the function may miss
> other functions, so it is appropriate to change the size of the macro.
> You can refer to other functions, such as
o> mtk_dip_vb2_video_queue_setup, the max plane size of this module is 8

To my understanding, the vcodec driver only has formats requiring up to
3 planes.  Could you explain why you need to change this macro to 8 ?
Could you please describe the issue you had which led to this change ?

> 
> 
> On Tue, Jun 27, 2023 at 6:42 PM Alain Volmat <alain.volmat@foss.st.com> wrote:
> >
> > Hi,
> >
> > I had a look at some places where this macro MTK_VCODEC_MAX_PLANES
> > is being used, such as q_data->bytesperline etc.
> > This patch seems to be increasing the table size from 3 to 8 but,
> > if my understanding is correct doesn't solve the issue that
> > (taking the example you give in vidioc_venc_g_fmt) the table
> > bytesperline is accessed taking into account a num_planes values which
> > is unchecked if appropriate for this driver.
> >
> > What are the 8 planes you are referring to ?
> >
> > While increasing the table to 8 might also be necessary, it seems to me
> > that the real OOB access issue should be solved by checking the num of
> > planes value.
> >
> > Regards,
> > Alain
> >
> > On Tue, Jun 27, 2023 at 04:10:02PM +0800, yqsun1997@gmail.com wrote:
> > > From: yqsun1997 <yqsun1997@gmail.com>
> > >
> > > The num_planes max index is 8,
> > > but bytesperline and bytesperline in struct mtk_q_data,
> > > The max index is MTK_VCODEC_MAX_PLANES == 3,
> > > so will cause OOB read and write in multiple places.like vidioc_venc_g_fmt
> > > same as commit 8fbcf730
> > >
> > > Signed-off-by: yqsun1997 <yqsun1997@gmail.com>
> > > ---
> > >  drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
> > > index 9acab54fd..c2c157675 100644
> > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
> > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
> > > @@ -22,7 +22,7 @@
> > >  #define MTK_VCODEC_DEC_NAME  "mtk-vcodec-dec"
> > >  #define MTK_VCODEC_ENC_NAME  "mtk-vcodec-enc"
> > >
> > > -#define MTK_VCODEC_MAX_PLANES        3
> > > +#define MTK_VCODEC_MAX_PLANES        8
> > >  #define MTK_V4L2_BENCHMARK   0
> > >  #define WAIT_INTR_TIMEOUT_MS 1000
> > >  #define IS_VDEC_LAT_ARCH(hw_arch) ((hw_arch) >= MTK_VDEC_LAT_SINGLE_CORE)
> > > --
> > > 2.39.2
> > >