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

[Dan Carpenter <dan.carpenter@oracle.com>]

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