Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 27 Jun 2023 15:43:51 +0200
From:   Alain Volmat <alain.volmat@foss.st.com>
To:     sun yq <yqsun1997@gmail.com>
CC:     <tiffany.lin@mediatek.com>, <andrew-ct.chen@mediatek.com>,
        <yunfei.dong@mediatek.com>, <matthias.bgg@gmail.com>,
        <angelogioacchino.delregno@collabora.com>,
        <linux-media@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-mediatek@lists.infradead.org>, <499671216@qq.com>
Subject: Re: [PATCH] OOB read and write in mtk multiple places
Message-ID: <20230627134314.GA3605030@gnbcxd0016.gnb.st.com>
Mail-Followup-To: sun yq <yqsun1997@gmail.com>, tiffany.lin@mediatek.com,
        andrew-ct.chen@mediatek.com, yunfei.dong@mediatek.com,
        matthias.bgg@gmail.com, angelogioacchino.delregno@collabora.com,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, 499671216@qq.com
References: <20230627081002.1768990-1-yqsun1997@gmail.com>
 <20230627104237.GA3601890@gnbcxd0016.gnb.st.com>
 <CAA0BgY_bKg4qXmtmWqHQW+EOvfXUG0FqRtAJ7A8C40uL1v8qVA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAA0BgY_bKg4qXmtmWqHQW+EOvfXUG0FqRtAJ7A8C40uL1v8qVA@mail.gmail.com>
Precedence: bulk

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
> > >