OOB read and write in mdp_prepare_buffer

[PATCH] OOB read and write in mdp_prepare_buffer

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

From: yqsun1997 <yqsun1997@gmail.com>

Because format in struct img_image_buffer max index is IMG_MAX_PLANES ==3,
The num_planes max index is 8.so will be OOB like in mdp_prepare_buffer.

static void mdp_prepare_buffer(struct img_image_buffer *b,
                               struct mdp_frame *frame, struct vb2_buffer *vb)
{
        struct v4l2_pix_format_mplane *pix_mp = &frame->format.fmt.pix_mp;
        unsigned int i;

        b->format.colorformat = frame->mdp_fmt->mdp_color;
        b->format.ycbcr_prof = frame->ycbcr_prof;
        for (i = 0; i < pix_mp->num_planes; ++i) {
                u32 stride = mdp_fmt_get_stride(frame->mdp_fmt,
                        pix_mp->plane_fmt[i].bytesperline, i);

                b->format.plane_fmt[i].stride = stride;  //oob
                ......

Signed-off-by: yqsun1997 <yqsun1997@gmail.com>
---
 drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h
index ae0396806..e2e991a34 100644
--- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h
+++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h
@@ -11,7 +11,7 @@
 
 #define IMG_MAX_HW_INPUTS	3
 #define IMG_MAX_HW_OUTPUTS	4
-#define IMG_MAX_PLANES		3
+#define IMG_MAX_PLANES		8
 #define IMG_MAX_COMPONENTS	20
 
 struct img_crop {
-- 
2.39.2

Re: [PATCH] OOB read and write in mdp_prepare_buffer

Posted by Hans Verkuil 1 year, 4 months ago

Hi!

On 6/27/23 10:27, yqsun1997@gmail.com wrote:
> From: yqsun1997 <yqsun1997@gmail.com>
> 
> Because format in struct img_image_buffer max index is IMG_MAX_PLANES ==3,
> The num_planes max index is 8.so will be OOB like in mdp_prepare_buffer.
> 
> static void mdp_prepare_buffer(struct img_image_buffer *b,
>                                 struct mdp_frame *frame, struct vb2_buffer *vb)
> {
>          struct v4l2_pix_format_mplane *pix_mp = &frame->format.fmt.pix_mp;
>          unsigned int i;
> 
>          b->format.colorformat = frame->mdp_fmt->mdp_color;
>          b->format.ycbcr_prof = frame->ycbcr_prof;
>          for (i = 0; i < pix_mp->num_planes; ++i) {

Unless there is a bug in the driver, pix_mp->num_planes will never 
exceed 3. Userspace might certainly pass more than 3 planes, but only 
the first pix_mp->num_planes should ever be used.

If pix_mp->num_planes can ever be more than 3, then that would be the 
real bug.

Regards,

	Hans

>                  u32 stride = mdp_fmt_get_stride(frame->mdp_fmt,
>                          pix_mp->plane_fmt[i].bytesperline, i);
> 
>                  b->format.plane_fmt[i].stride = stride;  //oob
>                  ......
> 
> Signed-off-by: yqsun1997 <yqsun1997@gmail.com>
> ---
>   drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h
> index ae0396806..e2e991a34 100644
> --- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h
> +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h
> @@ -11,7 +11,7 @@
>   
>   #define IMG_MAX_HW_INPUTS	3
>   #define IMG_MAX_HW_OUTPUTS	4
> -#define IMG_MAX_PLANES		3
> +#define IMG_MAX_PLANES		8
>   #define IMG_MAX_COMPONENTS	20
>   
>   struct img_crop {

Re: [PATCH] OOB read and write in mdp_prepare_buffer

Posted by Alain Volmat 1 year, 4 months ago

Hi,

On Tue, Jun 27, 2023 at 04:27:31PM +0800, yqsun1997@gmail.com wrote:
> From: yqsun1997 <yqsun1997@gmail.com>
> 
> Because format in struct img_image_buffer max index is IMG_MAX_PLANES ==3,
> The num_planes max index is 8.so will be OOB like in mdp_prepare_buffer.

Similarly as your other patch, could you describe why you need to
increase the IMG_MAX_PLANES while I suspect your driver only needs to
deal with 3 planes.  While the maximum num_planes value that can be
given by the user is 8, this has to be first compared to the configured
format prior to reaching this function.

> 
> static void mdp_prepare_buffer(struct img_image_buffer *b,
>                                struct mdp_frame *frame, struct vb2_buffer *vb)
> {
>         struct v4l2_pix_format_mplane *pix_mp = &frame->format.fmt.pix_mp;
>         unsigned int i;
> 
>         b->format.colorformat = frame->mdp_fmt->mdp_color;
>         b->format.ycbcr_prof = frame->ycbcr_prof;
>         for (i = 0; i < pix_mp->num_planes; ++i) {
>                 u32 stride = mdp_fmt_get_stride(frame->mdp_fmt,
>                         pix_mp->plane_fmt[i].bytesperline, i);
> 
>                 b->format.plane_fmt[i].stride = stride;  //oob
>                 ......
> 
> Signed-off-by: yqsun1997 <yqsun1997@gmail.com>
> ---
>  drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h
> index ae0396806..e2e991a34 100644
> --- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h
> +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h
> @@ -11,7 +11,7 @@
>  
>  #define IMG_MAX_HW_INPUTS	3
>  #define IMG_MAX_HW_OUTPUTS	4
> -#define IMG_MAX_PLANES		3
> +#define IMG_MAX_PLANES		8
>  #define IMG_MAX_COMPONENTS	20
>  
>  struct img_crop {
> -- 
> 2.39.2
> 

Regards,
Alain

Re: [PATCH] OOB read and write in mdp_prepare_buffer

Posted by sun yq 1 year, 4 months ago

Hi,
Because there are many functions using the plane, increasing the max
number of the plane is to maximize the solution to all possible oob
places.

On Tue, Jun 27, 2023 at 10:06 PM Alain Volmat <alain.volmat@foss.st.com> wrote:
>
> Hi,
>
> On Tue, Jun 27, 2023 at 04:27:31PM +0800, yqsun1997@gmail.com wrote:
> > From: yqsun1997 <yqsun1997@gmail.com>
> >
> > Because format in struct img_image_buffer max index is IMG_MAX_PLANES ==3,
> > The num_planes max index is 8.so will be OOB like in mdp_prepare_buffer.
>
> Similarly as your other patch, could you describe why you need to
> increase the IMG_MAX_PLANES while I suspect your driver only needs to
> deal with 3 planes.  While the maximum num_planes value that can be
> given by the user is 8, this has to be first compared to the configured
> format prior to reaching this function.
>
> >
> > static void mdp_prepare_buffer(struct img_image_buffer *b,
> >                                struct mdp_frame *frame, struct vb2_buffer *vb)
> > {
> >         struct v4l2_pix_format_mplane *pix_mp = &frame->format.fmt.pix_mp;
> >         unsigned int i;
> >
> >         b->format.colorformat = frame->mdp_fmt->mdp_color;
> >         b->format.ycbcr_prof = frame->ycbcr_prof;
> >         for (i = 0; i < pix_mp->num_planes; ++i) {
> >                 u32 stride = mdp_fmt_get_stride(frame->mdp_fmt,
> >                         pix_mp->plane_fmt[i].bytesperline, i);
> >
> >                 b->format.plane_fmt[i].stride = stride;  //oob
> >                 ......
> >
> > Signed-off-by: yqsun1997 <yqsun1997@gmail.com>
> > ---
> >  drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h
> > index ae0396806..e2e991a34 100644
> > --- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h
> > +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h
> > @@ -11,7 +11,7 @@
> >
> >  #define IMG_MAX_HW_INPUTS    3
> >  #define IMG_MAX_HW_OUTPUTS   4
> > -#define IMG_MAX_PLANES               3
> > +#define IMG_MAX_PLANES               8
> >  #define IMG_MAX_COMPONENTS   20
> >
> >  struct img_crop {
> > --
> > 2.39.2
> >
>
> Regards,
> Alain

Re: [PATCH] OOB read and write in mdp_prepare_buffer

Posted by Alain Volmat 1 year, 4 months ago

Hi,

On Wed, Jun 28, 2023 at 07:28:54AM +0800, sun yq wrote:
> Hi,
> Because there are many functions using the plane, increasing the max
> number of the plane is to maximize the solution to all possible oob
> places.

I don't think it is the right approach then.  If the HW is only handling
3 planes, there should be no reason to have to allocate for 8 planes.  I
suspect that this 8 value is coming from the maximum allowed plane
number in V4L2 right ?
INHO driver should simply be fixed to ensure that num_plane won't go
higher than the real number of plane allocated in the structures.
It should be possible to get the num_plane value from the format
selected.

Alain

> 
> On Tue, Jun 27, 2023 at 10:06 PM Alain Volmat <alain.volmat@foss.st.com> wrote:
> >
> > Hi,
> >
> > On Tue, Jun 27, 2023 at 04:27:31PM +0800, yqsun1997@gmail.com wrote:
> > > From: yqsun1997 <yqsun1997@gmail.com>
> > >
> > > Because format in struct img_image_buffer max index is IMG_MAX_PLANES ==3,
> > > The num_planes max index is 8.so will be OOB like in mdp_prepare_buffer.
> >
> > Similarly as your other patch, could you describe why you need to
> > increase the IMG_MAX_PLANES while I suspect your driver only needs to
> > deal with 3 planes.  While the maximum num_planes value that can be
> > given by the user is 8, this has to be first compared to the configured
> > format prior to reaching this function.
> >
> > >
> > > static void mdp_prepare_buffer(struct img_image_buffer *b,
> > >                                struct mdp_frame *frame, struct vb2_buffer *vb)
> > > {
> > >         struct v4l2_pix_format_mplane *pix_mp = &frame->format.fmt.pix_mp;
> > >         unsigned int i;
> > >
> > >         b->format.colorformat = frame->mdp_fmt->mdp_color;
> > >         b->format.ycbcr_prof = frame->ycbcr_prof;
> > >         for (i = 0; i < pix_mp->num_planes; ++i) {
> > >                 u32 stride = mdp_fmt_get_stride(frame->mdp_fmt,
> > >                         pix_mp->plane_fmt[i].bytesperline, i);
> > >
> > >                 b->format.plane_fmt[i].stride = stride;  //oob
> > >                 ......
> > >
> > > Signed-off-by: yqsun1997 <yqsun1997@gmail.com>
> > > ---
> > >  drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h
> > > index ae0396806..e2e991a34 100644
> > > --- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h
> > > +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h
> > > @@ -11,7 +11,7 @@
> > >
> > >  #define IMG_MAX_HW_INPUTS    3
> > >  #define IMG_MAX_HW_OUTPUTS   4
> > > -#define IMG_MAX_PLANES               3
> > > +#define IMG_MAX_PLANES               8
> > >  #define IMG_MAX_COMPONENTS   20
> > >
> > >  struct img_crop {
> > > --
> > > 2.39.2
> > >
> >
> > Regards,
> > Alain

Re: [PATCH] OOB read and write in mdp_prepare_buffer

Posted by sun yq 1 year, 4 months ago

Hi Alain,

May I ask if you are the person involved in the code?We should  listen
to the opinions of the code owner.

On Wed, Jun 28, 2023 at 2:34 PM Alain Volmat <alain.volmat@foss.st.com> wrote:
>
> Hi,
>
> On Wed, Jun 28, 2023 at 07:28:54AM +0800, sun yq wrote:
> > Hi,
> > Because there are many functions using the plane, increasing the max
> > number of the plane is to maximize the solution to all possible oob
> > places.
>
> I don't think it is the right approach then.  If the HW is only handling
> 3 planes, there should be no reason to have to allocate for 8 planes.  I
> suspect that this 8 value is coming from the maximum allowed plane
> number in V4L2 right ?
> INHO driver should simply be fixed to ensure that num_plane won't go
> higher than the real number of plane allocated in the structures.
> It should be possible to get the num_plane value from the format
> selected.
>
> Alain
>
> >
> > On Tue, Jun 27, 2023 at 10:06 PM Alain Volmat <alain.volmat@foss.st.com> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Jun 27, 2023 at 04:27:31PM +0800, yqsun1997@gmail.com wrote:
> > > > From: yqsun1997 <yqsun1997@gmail.com>
> > > >
> > > > Because format in struct img_image_buffer max index is IMG_MAX_PLANES ==3,
> > > > The num_planes max index is 8.so will be OOB like in mdp_prepare_buffer.
> > >
> > > Similarly as your other patch, could you describe why you need to
> > > increase the IMG_MAX_PLANES while I suspect your driver only needs to
> > > deal with 3 planes.  While the maximum num_planes value that can be
> > > given by the user is 8, this has to be first compared to the configured
> > > format prior to reaching this function.
> > >
> > > >
> > > > static void mdp_prepare_buffer(struct img_image_buffer *b,
> > > >                                struct mdp_frame *frame, struct vb2_buffer *vb)
> > > > {
> > > >         struct v4l2_pix_format_mplane *pix_mp = &frame->format.fmt.pix_mp;
> > > >         unsigned int i;
> > > >
> > > >         b->format.colorformat = frame->mdp_fmt->mdp_color;
> > > >         b->format.ycbcr_prof = frame->ycbcr_prof;
> > > >         for (i = 0; i < pix_mp->num_planes; ++i) {
> > > >                 u32 stride = mdp_fmt_get_stride(frame->mdp_fmt,
> > > >                         pix_mp->plane_fmt[i].bytesperline, i);
> > > >
> > > >                 b->format.plane_fmt[i].stride = stride;  //oob
> > > >                 ......
> > > >
> > > > Signed-off-by: yqsun1997 <yqsun1997@gmail.com>
> > > > ---
> > > >  drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h
> > > > index ae0396806..e2e991a34 100644
> > > > --- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h
> > > > +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h
> > > > @@ -11,7 +11,7 @@
> > > >
> > > >  #define IMG_MAX_HW_INPUTS    3
> > > >  #define IMG_MAX_HW_OUTPUTS   4
> > > > -#define IMG_MAX_PLANES               3
> > > > +#define IMG_MAX_PLANES               8
> > > >  #define IMG_MAX_COMPONENTS   20
> > > >
> > > >  struct img_crop {
> > > > --
> > > > 2.39.2
> > > >
> > >
> > > Regards,
> > > Alain

Re: [PATCH] OOB read and write in mdp_prepare_buffer

Posted by Chen-Yu Tsai 1 year, 4 months ago

On Wed, Jun 28, 2023 at 12:09 PM sun yq <yqsun1997@gmail.com> wrote:
>
> Hi Alain,
>
> May I ask if you are the person involved in the code?We should  listen
> to the opinions of the code owner.

Please don't top post.

Alain has a valid point. Please describe in detail how you are running
into or detecting this problem. Are you running into this for real? Or
is this from a static analyzer? Or are you simply eyeballing the issue?


ChenYu

> On Wed, Jun 28, 2023 at 2:34 PM Alain Volmat <alain.volmat@foss.st.com> wrote:
> >
> > Hi,
> >
> > On Wed, Jun 28, 2023 at 07:28:54AM +0800, sun yq wrote:
> > > Hi,
> > > Because there are many functions using the plane, increasing the max
> > > number of the plane is to maximize the solution to all possible oob
> > > places.
> >
> > I don't think it is the right approach then.  If the HW is only handling
> > 3 planes, there should be no reason to have to allocate for 8 planes.  I
> > suspect that this 8 value is coming from the maximum allowed plane
> > number in V4L2 right ?
> > INHO driver should simply be fixed to ensure that num_plane won't go
> > higher than the real number of plane allocated in the structures.
> > It should be possible to get the num_plane value from the format
> > selected.
> >
> > Alain
> >
> > >
> > > On Tue, Jun 27, 2023 at 10:06 PM Alain Volmat <alain.volmat@foss.st.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tue, Jun 27, 2023 at 04:27:31PM +0800, yqsun1997@gmail.com wrote:
> > > > > From: yqsun1997 <yqsun1997@gmail.com>
> > > > >
> > > > > Because format in struct img_image_buffer max index is IMG_MAX_PLANES ==3,
> > > > > The num_planes max index is 8.so will be OOB like in mdp_prepare_buffer.
> > > >
> > > > Similarly as your other patch, could you describe why you need to
> > > > increase the IMG_MAX_PLANES while I suspect your driver only needs to
> > > > deal with 3 planes.  While the maximum num_planes value that can be
> > > > given by the user is 8, this has to be first compared to the configured
> > > > format prior to reaching this function.
> > > >
> > > > >
> > > > > static void mdp_prepare_buffer(struct img_image_buffer *b,
> > > > >                                struct mdp_frame *frame, struct vb2_buffer *vb)
> > > > > {
> > > > >         struct v4l2_pix_format_mplane *pix_mp = &frame->format.fmt.pix_mp;
> > > > >         unsigned int i;
> > > > >
> > > > >         b->format.colorformat = frame->mdp_fmt->mdp_color;
> > > > >         b->format.ycbcr_prof = frame->ycbcr_prof;
> > > > >         for (i = 0; i < pix_mp->num_planes; ++i) {
> > > > >                 u32 stride = mdp_fmt_get_stride(frame->mdp_fmt,
> > > > >                         pix_mp->plane_fmt[i].bytesperline, i);
> > > > >
> > > > >                 b->format.plane_fmt[i].stride = stride;  //oob
> > > > >                 ......
> > > > >
> > > > > Signed-off-by: yqsun1997 <yqsun1997@gmail.com>
> > > > > ---
> > > > >  drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h
> > > > > index ae0396806..e2e991a34 100644
> > > > > --- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h
> > > > > +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-type.h
> > > > > @@ -11,7 +11,7 @@
> > > > >
> > > > >  #define IMG_MAX_HW_INPUTS    3
> > > > >  #define IMG_MAX_HW_OUTPUTS   4
> > > > > -#define IMG_MAX_PLANES               3
> > > > > +#define IMG_MAX_PLANES               8
> > > > >  #define IMG_MAX_COMPONENTS   20
> > > > >
> > > > >  struct img_crop {
> > > > > --
> > > > > 2.39.2
> > > > >
> > > >
> > > > Regards,
> > > > Alain
>