[PATCH RESEND v2] media: nxp: imx8-isi: Fix potential out-of-bounds issues
G.N. Zhou (OSS)
guoniu.zhou at oss.nxp.com
Sun Mar 22 23:08:11 PDT 2026
Hi Laurent,
Thanks for your review.
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Sent: Friday, March 20, 2026 5:34 AM
> To: G.N. Zhou (OSS) <guoniu.zhou at oss.nxp.com>
> Cc: Mauro Carvalho Chehab <mchehab at kernel.org>; Shawn Guo
> <shawnguo at kernel.org>; Sascha Hauer <s.hauer at pengutronix.de>;
> Pengutronix Kernel Team <kernel at pengutronix.de>; Fabio Estevam
> <festevam at gmail.com>; Stefan Riedmüller <s.riedmueller at phytec.de>;
> Jacopo Mondi <jacopo at jmondi.org>; c.hemp at phytec.de; Frank Li
> <frank.li at nxp.com>; Aisheng Dong <aisheng.dong at nxp.com>; linux-
> media at vger.kernel.org; imx at lists.linux.dev; linux-arm-
> kernel at lists.infradead.org; linux-kernel at vger.kernel.org; G.N. Zhou
> <guoniu.zhou at nxp.com>; stable at vger.kernel.org
> Subject: Re: [PATCH RESEND v2] media: nxp: imx8-isi: Fix potential out-of-
> bounds issues
>
> Hello Guoniu,
>
> Thank you for the patch.
>
> On Sat, Feb 28, 2026 at 05:35:53PM +0800, Guoniu Zhou wrote:
> > From: Guoniu Zhou <guoniu.zhou at nxp.com>
> >
> > The maximum downscaling factor supported by ISI can be up to 16. Add
> > minimum value constraint before applying the setting to hardware.
> > Otherwise, the process will not respond even when Ctrl+C is executed.
>
> Could you share how you can trigger this from userspace ? I'd like to test it and
> see where the absence of response comes from.
Sure, I found this issue on i.MX952 but other platform like i.MX8MN also could reproduce.
Bellow command using ISI m2m(/dev/video1) function.
root at imx8mnevk:~# v4l2-ctl --list-dev
mxc-isi-cap (platform:32e20000.isi):
/dev/video0
/dev/video1
/dev/media0
$ gst-launch-1.0 videotestsrc num-buffers=100 '!' video/x-raw,width=1920,height=1080,format=BGRx '!' queue '!' v4l2convert '!' video/x-raw,width=32,height=32,format=BGRx '!' queue '!' waylandsink
>
> > Fixes: cf21f328fcaf ("media: nxp: Add i.MX8 ISI driver")
> > Cc: stable at vger.kernel.org
> > Reviewed-by: Frank Li <Frank.Li at nxp.com>
> > Signed-off-by: Guoniu Zhou <guoniu.zhou at nxp.com>
> > ---
> > Changes in v2:
> > - Fix possible side-effects caused by 'max_val' in CLAMP_DOWNSCALE_16
> macro.
> > - Add space line between two paragraph in comment.
> > - Link to v1:
> > https://lore.kernel.org/r/20260122-isi-v1-1-c3ec6e264f13@nxp.com
> > ---
> > drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h | 16
> > ++++++++++++++++ drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
> > | 6 +++--- drivers/media/platform/nxp/imx8-isi/imx8-isi-pipe.c | 6
> > ++----
> > 3 files changed, 21 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> > b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> > index
> >
> 3cbd35305af0f8026c4f76b5eb5d0864f8e36dc3..84e134f4d5fb26be652ac3e6
> aecd
> > 459bb2a0d1c6 100644
> > --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> > @@ -59,6 +59,22 @@ struct v4l2_m2m_dev;
> > #define MXC_ISI_M2M "mxc-isi-m2m"
> > #define MXC_MAX_PLANES 3
> >
> > +/*
> > + * ISI scaling engine works in two parts: it performs pre-decimation
> > +of
> > + * the image followed by bilinear filtering to achieve the desired
> > + * downscaling factor.
> > + *
> > + * The decimation filter provides a maximum downscaling factor of 8,
> > +and
> > + * the subsequent bilinear filter provides a maximum downscaling
> > +factor
> > + * of 2. Combined, the maximum scaling factor can be up to 16.
> > + */
> > +#define CLAMP_DOWNSCALE_16(val, max_val) \
>
> Let's add a MXC_ISI_ prefix.
>
> > +({ \
> > + typeof(max_val) __max_val = (max_val); \
> > + \
> > + clamp((val), max(1U, __max_val >> 4), __max_val); \
>
> I think you should round the division up here:
>
> clamp((val), max(1U, (__max_val + 15) / 16, __max_val); \
>
> Let's assume the input size (max_val) is 1000. Without rounding up, the output
> size will be clamped to 1000 / 16 = 62. This leads to a downscaling factor
> slightly above 16.
>
> > +})
>
> Any reason not to make this an inline function instead of a macro ?
Thank you for the suggestion. You're right - an inline function would be better here for type safety
and easier debugging. The macro was initially used without special consideration. I'll change it
to a static inline function in next version.
>
> > +
> > struct mxc_isi_dev;
> > struct mxc_isi_m2m_ctx;
> >
> > diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
> > b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
> > index
> >
> f425ac7868547da401e86ce5a9b70a9890e72541..8860d89713667d06abc94e
> 602452
> > 6fabac46feb7 100644
> > --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
> > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
> > @@ -509,9 +509,9 @@ __mxc_isi_m2m_try_fmt_vid(struct
> mxc_isi_m2m_ctx *ctx,
> > const enum mxc_isi_video_type type) {
> > if (type == MXC_ISI_VIDEO_M2M_CAP) {
> > - /* Downscaling only */
> > - pix->width = min(pix->width, ctx->queues.out.format.width);
> > - pix->height = min(pix->height, ctx->queues.out.format.height);
> > + /* Downscaling one-sixteenth only */
>
> There are two spaces after "only".
Will remove it.
>
> > + pix->width = CLAMP_DOWNSCALE_16(pix->width, ctx-
> >queues.out.format.width);
> > + pix->height = CLAMP_DOWNSCALE_16(pix->height,
> > +ctx->queues.out.format.height);
>
> To keep lines shorter, you can write
>
> const struct v4l2_pix_format_mplane *format =
> &ctx->queues.out.format;
>
> /* Downscaling only, by up to 16. */
> pix->width = CLAMP_DOWNSCALE_16(pix->width, format-
> >width);
> pix->height = CLAMP_DOWNSCALE_16(pix->height, format-
> >height);
Ok.
>
> > }
> >
> > return mxc_isi_format_try(ctx->m2m->pipe, pix, type); diff --git
> > a/drivers/media/platform/nxp/imx8-isi/imx8-isi-pipe.c
> > b/drivers/media/platform/nxp/imx8-isi/imx8-isi-pipe.c
> > index
> >
> a41c51dd9ce0f2eeb779e9aa2461593b0d635f41..c3ffc8a38d1269c0a4e6493b
> 4d75
> > 690f01cc87bb 100644
> > --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-pipe.c
> > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-pipe.c
> > @@ -647,10 +647,8 @@ static int mxc_isi_pipe_set_selection(struct
> > v4l2_subdev *sd,
> >
>
> There's a comment above that states
>
> /* The sink crop is bound by the sink format downscaling only).
> */
>
> That's not right, let's fix it:
>
> /*
> * The ISI supports downscaling only, with a factor up to 16.
> * Clamp the compose rectangle size accordingly.
> */
Will fix.
>
> Reading the driver code to check how rounding is done when programming the
> hardware, I noticed that we clamp the bilinear scaling ratio to
> ISI_DOWNSCALE_THRESHOLD, defined as 0x4000:
>
> return min_t(u32, from * 0x1000 / (to * *dec),
> ISI_DOWNSCALE_THRESHOLD);
>
> The ratio is documented to be a Q2.12 value, so 0x4000 is x1.0. The code seems
> wrong to me, what do you think ?
Yes, you're right. ISI_DOWNSCALE_THRESHOLD should be 0x2000 because scaling
down by a factor greater than 2 is not supported with the bilinear filter, will fix in
next version. Thanks for pointing out.
>
> > sel->r.left = 0;
> > sel->r.top = 0;
> > - sel->r.width = clamp(sel->r.width, MXC_ISI_MIN_WIDTH,
> > - format->width);
> > - sel->r.height = clamp(sel->r.height, MXC_ISI_MIN_HEIGHT,
> > - format->height);
> > + sel->r.width = CLAMP_DOWNSCALE_16(sel->r.width, format-
> >width);
> > + sel->r.height = CLAMP_DOWNSCALE_16(sel->r.height, format-
> >height);
> >
> > rect = mxc_isi_pipe_get_pad_compose(pipe, state,
> > MXC_ISI_PIPE_PAD_SINK);
> >
> > ---
> > base-commit: eb4ee870747c3a77a9c3c84d84efb64bd481013a
> > change-id: 20260122-isi-74f87fbb9a6f
>
> --
> Regards,
>
> Laurent Pinchart
More information about the linux-arm-kernel
mailing list