Re: [PATCH v5 6/8] media: nxp: add V4L2 subdev driver for camera parallel interface (CPI)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] NULL pointer dereference when enabling or disabling streams on an unconnected sink pad.
- [High] Missing format bounds clamping causes integer underflow and massive hardware width configuration.
- [Medium] Missing `V4L2_SUBDEV_FL_STREAMS` flag breaks V4L2 format and routing state initialization.
- [Medium] Reversed pad direction arguments in `v4l2_subdev_state_xlate_streams` leads to improper stream mask mapping.
- [Low] Unconditional overwrite of hardware configuration variable `val` renders `switch` statement dead code.
--

commit 6eecb6f34887a526e9a659dcd6d0a03e320f98ab
Author: Alice Yuan <alice.yuan@nxp.com>

media: nxp: add V4L2 subdev driver for camera parallel interface (CPI)

Add a V4L2 sub-device driver for the CPI controller found on i.MX8QXP,
i.MX8QM, and i.MX93 SoCs. This controller supports parallel camera sensors
and enables image data capture through a parallel interface.

> diff --git a/drivers/media/platform/nxp/imx-parallel-cpi.c b/drivers/media/platform/nxp/imx-parallel-cpi.c
> new file mode 100644
> index 0000000000000..00f5d5f47644b
> --- /dev/null
> +++ b/drivers/media/platform/nxp/imx-parallel-cpi.c
> @@ -0,0 +1,614 @@

[ ... ]

> +static void imx_cpi_config_ctrl_reg1(struct imx_cpi_device *pcpidev,
> +				     const struct v4l2_mbus_framefmt *format)
> +{
> +	const struct imx_cpi_plat_data *pdata = pcpidev->pdata;
> +	u32 pixel_width;
> +	u32 vsync_pulse;
> +	u32 val;
> +
> +	pixel_width = format->width - 1;
> +	vsync_pulse = format->width << 1;
> +
> +	switch (pcpidev->pdata->version) {
> +	case PI_V1:
> +		val = CPI_CTRL_REG1_PIXEL_WIDTH(pixel_width) |
> +		      CPI_CTRL_REG1_VSYNC_PULSE(vsync_pulse);
> +		break;
> +	default:
> +		val = 0; /* Never happen */
> +	}
> +
> +	val = CPI_CTRL_REG1_PIXEL_WIDTH(pixel_width) |
> +	      CPI_CTRL_REG1_VSYNC_PULSE(vsync_pulse);

[Severity: Low]
Is the unconditional assignment to val here intentional? It immediately
overwrites the val computed inside the switch statement above, effectively
rendering the switch block as dead code and dropping any future hardware
version configurations.

> +	writel(val, pcpidev->regs + pdata->interface_ctrl_reg1);
> +}

[ ... ]

> +static int imx_cpi_set_fmt(struct v4l2_subdev *sd,
> +			   struct v4l2_subdev_state *sd_state,
> +			   struct v4l2_subdev_format *sdformat)
> +{
> +	struct imx_cpi_pix_format const *pcpidev_fmt;
> +	struct v4l2_mbus_framefmt *fmt;
> +
> +	/*
> +	 * The Parallel cpi can't transcode in any way, the source format
> +	 * can't be modified.
> +	 */
> +	if (sdformat->pad == V4L2_SUBDEV_1TO1_PADS_SOURCE)
> +		return v4l2_subdev_get_fmt(sd, sd_state, sdformat);
> +
> +	pcpidev_fmt = find_imx_cpi_format(sdformat->format.code);
> +	if (!pcpidev_fmt)
> +		pcpidev_fmt = &imx_cpi_formats[0];
> +
> +	fmt = v4l2_subdev_state_get_format(sd_state, sdformat->pad);
> +
> +	fmt->code = pcpidev_fmt->code;
> +	fmt->width = sdformat->format.width;
> +	fmt->height = sdformat->format.height;

[Severity: High]
Does this code need bounds checking for width and height? If a malicious
application provides a width of 0, it is copied directly to the format
structure here.

Later in imx_cpi_config_ctrl_reg1(), the width is used like this:

    pixel_width = format->width - 1;

which would underflow to 0xffffffff, causing an extremely large dimension to
be written to the hardware configuration.

> +	fmt->field = V4L2_FIELD_NONE;
> +	fmt->colorspace = sdformat->format.colorspace;

[ ... ]

> +static int imx_cpi_disable_streams(struct v4l2_subdev *sd,
> +				   struct v4l2_subdev_state *state, u32 pad,
> +				   u64 streams_mask)
> +{
> +	struct imx_cpi_device *pcpidev = sd_to_imx_cpi_device(sd);
> +	struct media_pad *sink_pad, *remote_pad;
> +	struct device *dev = pcpidev->dev;
> +	struct v4l2_subdev *remote_sd;
> +	u64 mask;
> +	int ret;
> +
> +	sink_pad = &sd->entity.pads[V4L2_SUBDEV_1TO1_PADS_SINK];
> +	remote_pad = media_pad_remote_pad_first(sink_pad);
> +	remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);

[Severity: High]
Could this cause a NULL pointer dereference if the subdevice is not connected?

media_pad_remote_pad_first() returns NULL if no link is enabled on the sink
pad. If userspace sends VIDIOC_SUBDEV_S_STREAM directly to the unconnected
device node, remote_pad->entity would trigger a panic.

> +
> +	mask = v4l2_subdev_state_xlate_streams(state, V4L2_SUBDEV_1TO1_PADS_SINK,
> +					       V4L2_SUBDEV_1TO1_PADS_SOURCE,
> +					       &streams_mask);

[Severity: Medium]
Are the pad arguments reversed here? 

When disable_streams is called, streams_mask originates from the pad passed
to the operation (the SOURCE pad). To find the remote mask, it should
translate from SOURCE to SINK. 

Should this be passing V4L2_SUBDEV_1TO1_PADS_SOURCE as the second argument
and V4L2_SUBDEV_1TO1_PADS_SINK as the third?

> +
> +	ret = v4l2_subdev_disable_streams(remote_sd, remote_pad->index, mask);
> +	if (ret)

[ ... ]

> +static int imx_cpi_enable_streams(struct v4l2_subdev *sd,
> +				  struct v4l2_subdev_state *state, u32 pad,
> +				  u64 streams_mask)
> +{
> +	struct imx_cpi_device *pcpidev = sd_to_imx_cpi_device(sd);
> +	const struct imx_cpi_pix_format *pcpidev_fmt;
> +	const struct v4l2_mbus_framefmt *format;
> +	struct media_pad *sink_pad, *remote_pad;
> +	struct device *dev = pcpidev->dev;
> +	struct v4l2_subdev *remote_sd;
> +	u64 mask;
> +	int ret;
> +
> +	sink_pad = &sd->entity.pads[V4L2_SUBDEV_1TO1_PADS_SINK];
> +	remote_pad = media_pad_remote_pad_first(sink_pad);
> +	remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);

[Severity: High]
Does this have the same NULL pointer dereference issue on unconnected links as
imx_cpi_disable_streams()?

> +
> +	mask = v4l2_subdev_state_xlate_streams(state, V4L2_SUBDEV_1TO1_PADS_SINK,
> +					       V4L2_SUBDEV_1TO1_PADS_SOURCE,
> +					       &streams_mask);

[Severity: Medium]
Similarly, do the pad arguments need to be swapped here to correctly
translate from SOURCE to SINK?

> +
> +	format = v4l2_subdev_state_get_format(state, V4L2_SUBDEV_1TO1_PADS_SINK);

[ ... ]

> +static int imx_cpi_probe(struct platform_device *pdev)
> +{
> +	struct imx_cpi_device *pcpidev;
> +	struct device *dev = &pdev->dev;
> +	struct v4l2_subdev *sd;
> +	int ret;

[ ... ]

> +	sd->internal_ops = &imx_cpi_internal_ops;
> +	snprintf(sd->name, sizeof(sd->name), "parallel-%s",
> +		 dev_name(pcpidev->dev));
> +
> +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;

[Severity: Medium]
Should V4L2_SUBDEV_FL_STREAMS also be set here?

Without this flag, the V4L2 core falls back to allocating the legacy
state->pads array. This causes the routing and stream configuration
set up by v4l2_subdev_set_routing_with_fmt() in imx_cpi_init_state()
to be ignored, which leaves the initial format reporting uninitialized.

> +
> +	sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> +	sd->entity.ops = &imx_cpi_entity_ops;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-imx8qxp_pcam-v5-0-7fa6c8e7fba7@nxp.com?part=6