* [bug report] media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder
@ 2022-03-01 12:42 Dan Carpenter
2022-03-04 15:51 ` Mirela Rabulea
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2022-03-01 12:42 UTC (permalink / raw)
To: mirela.rabulea; +Cc: linux-media
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder
2022-03-01 12:42 [bug report] media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder Dan Carpenter
@ 2022-03-04 15:51 ` Mirela Rabulea
2022-03-07 9:44 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Mirela Rabulea @ 2022-03-04 15:51 UTC (permalink / raw)
To: dan.carpenter; +Cc: linux-media
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder
2022-03-04 15:51 ` Mirela Rabulea
@ 2022-03-07 9:44 ` Dan Carpenter
2022-03-08 13:18 ` [EXT] " Mirela Rabulea
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2022-03-07 9:44 UTC (permalink / raw)
To: Mirela Rabulea; +Cc: linux-media
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [EXT] Re: [bug report] media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder
2022-03-07 9:44 ` Dan Carpenter
@ 2022-03-08 13:18 ` Mirela Rabulea
0 siblings, 0 replies; 4+ messages in thread
From: Mirela Rabulea @ 2022-03-08 13:18 UTC (permalink / raw)
To: dan.carpenter; +Cc: linux-media
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-03-08 13:18 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-01 12:42 [bug report] media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder Dan Carpenter
2022-03-04 15:51 ` Mirela Rabulea
2022-03-07 9:44 ` Dan Carpenter
2022-03-08 13:18 ` [EXT] " Mirela Rabulea
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.