From: yqsun1997 <[email protected]>
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 <[email protected]>
---
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
Hi,
On Tue, Jun 27, 2023 at 04:27:31PM +0800, [email protected] wrote:
> From: yqsun1997 <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
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 <[email protected]> wrote:
>
> Hi,
>
> On Tue, Jun 27, 2023 at 04:27:31PM +0800, [email protected] wrote:
> > From: yqsun1997 <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
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 <[email protected]> wrote:
> >
> > Hi,
> >
> > On Tue, Jun 27, 2023 at 04:27:31PM +0800, [email protected] wrote:
> > > From: yqsun1997 <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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
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 <[email protected]> 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 <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Jun 27, 2023 at 04:27:31PM +0800, [email protected] wrote:
> > > > From: yqsun1997 <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > ---
> > > > 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
On Wed, Jun 28, 2023 at 12:09 PM sun yq <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tue, Jun 27, 2023 at 04:27:31PM +0800, [email protected] wrote:
> > > > > From: yqsun1997 <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > > ---
> > > > > 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
>
Hi!
On 6/27/23 10:27, [email protected] wrote:
> From: yqsun1997 <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 {