OOB read and write in mtk multiple places

[PATCH] OOB read and write in mtk multiple places

Posted by yqsun1997@gmail.com 1 year, 4 months ago

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

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

Posted by Alain Volmat 1 year, 4 months ago

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
>

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

Posted by sun yq 1 year, 4 months ago

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
mtk_dip_vb2_video_queue_setup, the max plane size of this module is 8


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

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

Posted by Alain Volmat 1 year, 4 months ago

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