[PATCH RESEND v2] media: nxp: imx8-isi: Fix potential out-of-bounds issues

[PATCH RESEND v2] media: nxp: imx8-isi: Fix potential out-of-bounds issues

[Guoniu Zhou]

From: Guoniu Zhou <guoniu.zhou@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.

Fixes: cf21f328fcaf ("media: nxp: Add i.MX8 ISI driver")
Cc: stable@vger.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Guoniu Zhou <guoniu.zhou@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..84e134f4d5fb26be652ac3e6aecd459bb2a0d1c6 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)			\
+({								\
+	typeof(max_val) __max_val = (max_val);			\
+								\
+	clamp((val), max(1U, __max_val >> 4), __max_val);	\
+})
+
 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..8860d89713667d06abc94e6024526fabac46feb7 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  */
+		pix->width = CLAMP_DOWNSCALE_16(pix->width, ctx->queues.out.format.width);
+		pix->height = CLAMP_DOWNSCALE_16(pix->height, ctx->queues.out.format.height);
 	}
 
 	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..c3ffc8a38d1269c0a4e6493b4d75690f01cc87bb 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,
 
 		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

Best regards,
-- 
Guoniu Zhou <guoniu.zhou@nxp.com>

Re: [PATCH RESEND v2] media: nxp: imx8-isi: Fix potential out-of-bounds issues

[Laurent Pinchart]

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@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.

> Fixes: cf21f328fcaf ("media: nxp: Add i.MX8 ISI driver")
> Cc: stable@vger.kernel.org
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> Signed-off-by: Guoniu Zhou <guoniu.zhou@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..84e134f4d5fb26be652ac3e6aecd459bb2a0d1c6 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 ?

> +
>  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..8860d89713667d06abc94e6024526fabac46feb7 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".

> +		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);

>  	}
>  
>  	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..c3ffc8a38d1269c0a4e6493b4d75690f01cc87bb 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.
		 */

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 ?

>  		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

RE: [PATCH RESEND v2] media: nxp: imx8-isi: Fix potential out-of-bounds issues

[G.N. Zhou (OSS)]

Hi Laurent,

Thanks for your review.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Friday, March 20, 2026 5:34 AM
> To: G.N. Zhou (OSS) <guoniu.zhou@oss.nxp.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Shawn Guo
> <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; Stefan Riedmüller <s.riedmueller@phytec.de>;
> Jacopo Mondi <jacopo@jmondi.org>; c.hemp@phytec.de; Frank Li
> <frank.li@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>; linux-
> media@vger.kernel.org; imx@lists.linux.dev; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; G.N. Zhou
> <guoniu.zhou@nxp.com>; stable@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@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@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@vger.kernel.org
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > Signed-off-by: Guoniu Zhou <guoniu.zhou@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