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@xxxxxxxxxxxxxxxx>
> Sent: Friday, March 20, 2026 5:34 AM
> To: G.N. Zhou (OSS) <guoniu.zhou@xxxxxxxxxxx>
> Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>; Shawn Guo
> <shawnguo@xxxxxxxxxx>; Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>;
> Pengutronix Kernel Team <kernel@xxxxxxxxxxxxxx>; Fabio Estevam
> <festevam@xxxxxxxxx>; Stefan Riedmüller <s.riedmueller@xxxxxxxxx>;
> Jacopo Mondi <jacopo@xxxxxxxxxx>; c.hemp@xxxxxxxxx; Frank Li
> <frank.li@xxxxxxx>; Aisheng Dong <aisheng.dong@xxxxxxx>; linux-
> media@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; G.N. Zhou
> <guoniu.zhou@xxxxxxx>; stable@xxxxxxxxxxxxxxx
> 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@xxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx
> > Reviewed-by: Frank Li <Frank.Li@xxxxxxx>
> > Signed-off-by: Guoniu Zhou <guoniu.zhou@xxxxxxx>
> > ---
> > 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@xxxxxxx
> > ---
> >  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