[bug report] media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder

[bug report] media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder

[Dan Carpenter]

Hello Mirela Rabulea,

The patch 2db16c6ed72c: "media: imx-jpeg: Add V4L2 driver for i.MX8
JPEG Encoder/Decoder" from Mar 11, 2021, leads to the following
Smatch static checker warning:

	drivers/media/platform/imx-jpeg/mxc-jpeg.c:1070 mxc_jpeg_queue_setup()
	warn: potential user controlled iterator 'i' (array size 2 vs 7)

drivers/media/platform/imx-jpeg/mxc-jpeg.c
    1053 static int mxc_jpeg_queue_setup(struct vb2_queue *q,
    1054                                 unsigned int *nbuffers,
    1055                                 unsigned int *nplanes,
    1056                                 unsigned int sizes[],
    1057                                 struct device *alloc_ctxs[])
    1058 {
    1059         struct mxc_jpeg_ctx *ctx = vb2_get_drv_priv(q);
    1060         struct mxc_jpeg_q_data *q_data = NULL;
    1061         int i;
    1062 
    1063         q_data = mxc_jpeg_get_q_data(ctx, q->type);
    1064         if (!q_data)
    1065                 return -EINVAL;
    1066 
    1067         /* Handle CREATE_BUFS situation - *nplanes != 0 */
    1068         if (*nplanes) {
    1069                 for (i = 0; i < *nplanes; i++) {
--> 1070                         if (sizes[i] < q_data->sizeimage[i])

Smatch thinks "*nplanes" is controlled by the user in vb2_create_bufs()
and it can be up to VIDEO_MAX_PLANES(8).  Meanwhile the q_data->sizeimage[]
array only has MXC_JPEG_MAX_PLANES(2) elements so this looks to be an
out of bounds access.

    1071                                 return -EINVAL;
    1072                 }
    1073                 return 0;
    1074         }
    1075 
    1076         /* Handle REQBUFS situation */
    1077         *nplanes = q_data->fmt->colplanes;
    1078         for (i = 0; i < *nplanes; i++)
    1079                 sizes[i] = q_data->sizeimage[i];
    1080 
    1081         return 0;
    1082 }

regards,
dan carpenter

Re: [bug report] media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder

[Mirela Rabulea]

Hi,

On Tue, 2022-03-01 at 15:42 +0300, Dan Carpenter wrote:
> 
> Hello Mirela Rabulea,
> 
> The patch 2db16c6ed72c: "media: imx-jpeg: Add V4L2 driver for i.MX8
> JPEG Encoder/Decoder" from Mar 11, 2021, leads to the following
> Smatch static checker warning:
> 
>         drivers/media/platform/imx-jpeg/mxc-jpeg.c:1070
> mxc_jpeg_queue_setup()
>         warn: potential user controlled iterator 'i' (array size 2 vs
> 7)
> 
> drivers/media/platform/imx-jpeg/mxc-jpeg.c
>     1053 static int mxc_jpeg_queue_setup(struct vb2_queue *q,
>     1054                                 unsigned int *nbuffers,
>     1055                                 unsigned int *nplanes,
>     1056                                 unsigned int sizes[],
>     1057                                 struct device *alloc_ctxs[])
>     1058 {
>     1059         struct mxc_jpeg_ctx *ctx = vb2_get_drv_priv(q);
>     1060         struct mxc_jpeg_q_data *q_data = NULL;
>     1061         int i;
>     1062
>     1063         q_data = mxc_jpeg_get_q_data(ctx, q->type);
>     1064         if (!q_data)
>     1065                 return -EINVAL;
>     1066
>     1067         /* Handle CREATE_BUFS situation - *nplanes != 0 */
>     1068         if (*nplanes) {
>     1069                 for (i = 0; i < *nplanes; i++) {
> --> 1070                         if (sizes[i] < q_data->sizeimage[i])
> 
> Smatch thinks "*nplanes" is controlled by the user in
> vb2_create_bufs()
> and it can be up to VIDEO_MAX_PLANES(8).  Meanwhile the q_data-
> >sizeimage[]
> array only has MXC_JPEG_MAX_PLANES(2) elements so this looks to be an
> out of bounds access.

Thanks for pointing this out. I tried to run smatch (for the first
time), and I do not get this warning reported. I'm wondering what am I
missing?

mirela@fsr-ub1664-134:/workssd/linux-next$
/workssd/smatch/smatch_scripts/kchecker drivers/media/platform/imx-
jpeg/
  CHECK   scripts/mod/empty.c
  CALL    scripts/checksyscalls.sh
  CALL    scripts/atomic/check-atomics.sh
  CHECK   arch/arm64/kernel/vdso/vgettimeofday.c
  CHECK   drivers/media/platform/imx-jpeg/mxc-jpeg-hw.c
  CC [M]  drivers/media/platform/imx-jpeg/mxc-jpeg.o
  CHECK   drivers/media/platform/imx-jpeg/mxc-jpeg.c
  LD [M]  drivers/media/platform/imx-jpeg/mxc-jpeg-encdec.o
mirela@fsr-ub1664-134:/workssd/linux-next$ 

I can induce some errors in the source code, and then I also see CHECK
errors.

I have built the kernel database with
smatch/smatch_scripts/build_kernel_data.sh before runing kchecker.

Thanks,
Mirela

> 
>     1071                                 return -EINVAL;
>     1072                 }
>     1073                 return 0;
>     1074         }
>     1075
>     1076         /* Handle REQBUFS situation */
>     1077         *nplanes = q_data->fmt->colplanes;
>     1078         for (i = 0; i < *nplanes; i++)
>     1079                 sizes[i] = q_data->sizeimage[i];
>     1080
>     1081         return 0;
>     1082 }
> 
> regards,
> dan carpenter

Re: [bug report] media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder

[Dan Carpenter]

On Fri, Mar 04, 2022 at 03:51:16PM +0000, Mirela Rabulea wrote:
> Hi,
> 
> On Tue, 2022-03-01 at 15:42 +0300, Dan Carpenter wrote:
> > 
> > Hello Mirela Rabulea,
> > 
> > The patch 2db16c6ed72c: "media: imx-jpeg: Add V4L2 driver for i.MX8
> > JPEG Encoder/Decoder" from Mar 11, 2021, leads to the following
> > Smatch static checker warning:
> > 
> >         drivers/media/platform/imx-jpeg/mxc-jpeg.c:1070
> > mxc_jpeg_queue_setup()
> >         warn: potential user controlled iterator 'i' (array size 2 vs
> > 7)
> > 
> > drivers/media/platform/imx-jpeg/mxc-jpeg.c
> >     1053 static int mxc_jpeg_queue_setup(struct vb2_queue *q,
> >     1054                                 unsigned int *nbuffers,
> >     1055                                 unsigned int *nplanes,
> >     1056                                 unsigned int sizes[],
> >     1057                                 struct device *alloc_ctxs[])
> >     1058 {
> >     1059         struct mxc_jpeg_ctx *ctx = vb2_get_drv_priv(q);
> >     1060         struct mxc_jpeg_q_data *q_data = NULL;
> >     1061         int i;
> >     1062
> >     1063         q_data = mxc_jpeg_get_q_data(ctx, q->type);
> >     1064         if (!q_data)
> >     1065                 return -EINVAL;
> >     1066
> >     1067         /* Handle CREATE_BUFS situation - *nplanes != 0 */
> >     1068         if (*nplanes) {
> >     1069                 for (i = 0; i < *nplanes; i++) {
> > --> 1070                         if (sizes[i] < q_data->sizeimage[i])
> > 
> > Smatch thinks "*nplanes" is controlled by the user in
> > vb2_create_bufs()
> > and it can be up to VIDEO_MAX_PLANES(8).  Meanwhile the q_data-
> > >sizeimage[]
> > array only has MXC_JPEG_MAX_PLANES(2) elements so this looks to be an
> > out of bounds access.
> 
> Thanks for pointing this out. I tried to run smatch (for the first
> time), and I do not get this warning reported. I'm wondering what am I
> missing?
> 
> mirela@fsr-ub1664-134:/workssd/linux-next$
> /workssd/smatch/smatch_scripts/kchecker drivers/media/platform/imx-
> jpeg/
>   CHECK   scripts/mod/empty.c
>   CALL    scripts/checksyscalls.sh
>   CALL    scripts/atomic/check-atomics.sh
>   CHECK   arch/arm64/kernel/vdso/vgettimeofday.c
>   CHECK   drivers/media/platform/imx-jpeg/mxc-jpeg-hw.c
>   CC [M]  drivers/media/platform/imx-jpeg/mxc-jpeg.o
>   CHECK   drivers/media/platform/imx-jpeg/mxc-jpeg.c
>   LD [M]  drivers/media/platform/imx-jpeg/mxc-jpeg-encdec.o
> mirela@fsr-ub1664-134:/workssd/linux-next$ 
> 
> I can induce some errors in the source code, and then I also see CHECK
> errors.
> 
> I have built the kernel database with
> smatch/smatch_scripts/build_kernel_data.sh before runing kchecker.
> 

Oh, sorry.  This check hasn't been published yet it's something I've
just started working on.  If the checker is wrong just ignore it, but
could you give me a hint so I can improve the check?

regards,
dan carpenter

Re: [EXT] Re: [bug report] media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder

[Mirela Rabulea]

Hi Dan,

On Mon, 2022-03-07 at 12:44 +0300, Dan Carpenter wrote:
> Caution: EXT Email
> 
> On Fri, Mar 04, 2022 at 03:51:16PM +0000, Mirela Rabulea wrote:
> > Hi,
> > 
> > On Tue, 2022-03-01 at 15:42 +0300, Dan Carpenter wrote:
> > > Hello Mirela Rabulea,
> > > 
> > > The patch 2db16c6ed72c: "media: imx-jpeg: Add V4L2 driver for
> > > i.MX8
> > > JPEG Encoder/Decoder" from Mar 11, 2021, leads to the following
> > > Smatch static checker warning:
> > > 
> > >         drivers/media/platform/imx-jpeg/mxc-jpeg.c:1070
> > > mxc_jpeg_queue_setup()
> > >         warn: potential user controlled iterator 'i' (array size
> > > 2 vs
> > > 7)
> > > 
> > > drivers/media/platform/imx-jpeg/mxc-jpeg.c
> > >     1053 static int mxc_jpeg_queue_setup(struct vb2_queue *q,
> > >     1054                                 unsigned int *nbuffers,
> > >     1055                                 unsigned int *nplanes,
> > >     1056                                 unsigned int sizes[],
> > >     1057                                 struct device
> > > *alloc_ctxs[])
> > >     1058 {
> > >     1059         struct mxc_jpeg_ctx *ctx = vb2_get_drv_priv(q);
> > >     1060         struct mxc_jpeg_q_data *q_data = NULL;
> > >     1061         int i;
> > >     1062
> > >     1063         q_data = mxc_jpeg_get_q_data(ctx, q->type);
> > >     1064         if (!q_data)
> > >     1065                 return -EINVAL;
> > >     1066
> > >     1067         /* Handle CREATE_BUFS situation - *nplanes != 0
> > > */
> > >     1068         if (*nplanes) {
> > >     1069                 for (i = 0; i < *nplanes; i++) {
> > > --> 1070                         if (sizes[i] < q_data-
> > > >sizeimage[i])
> > > 
> > > Smatch thinks "*nplanes" is controlled by the user in
> > > vb2_create_bufs()
> > > and it can be up to VIDEO_MAX_PLANES(8).  Meanwhile the q_data-
> > > > sizeimage[]
> > > array only has MXC_JPEG_MAX_PLANES(2) elements so this looks to
> > > be an
> > > out of bounds access.
> > 
> > Thanks for pointing this out. I tried to run smatch (for the first
> > time), and I do not get this warning reported. I'm wondering what
> > am I
> > missing?
> > 
> > mirela@fsr-ub1664-134:/workssd/linux-next$
> > /workssd/smatch/smatch_scripts/kchecker drivers/media/platform/imx-
> > jpeg/
> >   CHECK   scripts/mod/empty.c
> >   CALL    scripts/checksyscalls.sh
> >   CALL    scripts/atomic/check-atomics.sh
> >   CHECK   arch/arm64/kernel/vdso/vgettimeofday.c
> >   CHECK   drivers/media/platform/imx-jpeg/mxc-jpeg-hw.c
> >   CC [M]  drivers/media/platform/imx-jpeg/mxc-jpeg.o
> >   CHECK   drivers/media/platform/imx-jpeg/mxc-jpeg.c
> >   LD [M]  drivers/media/platform/imx-jpeg/mxc-jpeg-encdec.o
> > mirela@fsr-ub1664-134:/workssd/linux-next$
> > 
> > I can induce some errors in the source code, and then I also see
> > CHECK
> > errors.
> > 
> > I have built the kernel database with
> > smatch/smatch_scripts/build_kernel_data.sh before runing kchecker.
> > 
> 
> Oh, sorry.  This check hasn't been published yet it's something I've
> just started working on.  If the checker is wrong just ignore it, but
> could you give me a hint so I can improve the check?

The check is good, I sent a patch. I see some drivers do handle better
the *nplanes situation in queue_setup. Others will fail the check
(example:drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c)

Thanks,
Mirela

> 
> regards,
> dan carpenter
>