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

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