Re: [PATCH v8 2/2] media: nxp: Add i.MX95 CSI pixel formatter v4l2 driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Passing the source streams mask instead of the sink streams mask causes a NULL pointer dereference.
- [High] `csi_formatter_stop_stream()` uses the logical stream ID as the hardware Virtual Channel (VC) ID, corrupting hardware state.
- [High] Unchecked return value from `v4l2_subdev_state_get_format()` leads to kernel panic (NULL pointer dereference).
- [Medium] Mixing goto-based error handling with scope-based cleanups violates subsystem rules.
- [Medium] `csi_formatter_subdev_set_fmt()` incorrectly returns `-EINVAL` when a stream is unrouted.
--

commit 4a48a80a7ebf00e31a1265c66f021dc5ed204a58
Author: Guoniu Zhou <guoniu.zhou@nxp.com>

media: nxp: Add i.MX95 CSI pixel formatter v4l2 driver
    
The CSI pixel formatter is a module found on i.MX95 used to reformat
packet info, pixel and non-pixel data from CSI-2 host controller to
match Pixel Link(PL) definition.

> diff --git a/drivers/media/platform/nxp/imx95-csi-formatter.c b/drivers/media/platform/nxp/imx95-csi-formatter.c
> new file mode 100644
> index 0000000000000..671979444d48b
> --- /dev/null
> +++ b/drivers/media/platform/nxp/imx95-csi-formatter.c
> @@ -0,0 +1,757 @@

[ ... ]

> +static int csi_formatter_subdev_enum_mbus_code(struct v4l2_subdev *sd,
> +					       struct v4l2_subdev_state *sd_state,
> +					       struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	if (code->pad == CSI_FORMATTER_PAD_SOURCE) {
> +		struct v4l2_mbus_framefmt *fmt;
> +
> +		if (code->index > 0)
> +			return -EINVAL;
> +
> +		fmt = v4l2_subdev_state_get_format(sd_state, code->pad,
> +						   code->stream);
> +		code->code = fmt->code;

[Severity: High]
Can fmt be NULL here? If userspace provides an invalid or non-existent
stream ID, the V4L2 core does not validate it, and
v4l2_subdev_state_get_format() will return NULL, which would cause a NULL
pointer dereference.

> +		return 0;
> +	}
> +
> +	if (code->index >= ARRAY_SIZE(formats))
> +		return -EINVAL;
> +
> +	code->code = formats[code->index].code;
> +
> +	return 0;
> +}
> +
> +static int csi_formatter_subdev_set_fmt(struct v4l2_subdev *sd,
> +					struct v4l2_subdev_state *sd_state,
> +					struct v4l2_subdev_format *sdformat)
> +{
> +	struct csi_formatter_pix_format const *format;
> +	struct v4l2_mbus_framefmt *fmt;
> +
> +	if (sdformat->pad == CSI_FORMATTER_PAD_SOURCE)
> +		return v4l2_subdev_get_fmt(sd, sd_state, sdformat);
> +
> +	format = csi_formatter_find_format(sdformat->format.code);
> +	if (!format)
> +		format = &formats[0];
> +
> +	v4l_bound_align_image(&sdformat->format.width, 1, 0xffff, 2,
> +			      &sdformat->format.height, 1, 0xffff, 0, 0);
> +
> +	fmt = v4l2_subdev_state_get_format(sd_state, sdformat->pad,
> +					   sdformat->stream);
> +	*fmt = sdformat->format;

[Severity: High]
Can fmt be NULL here as well? If the stream ID from userspace is invalid,
v4l2_subdev_state_get_format() will return NULL, resulting in a NULL pointer
dereference.

> +
> +	/* Set default code if user set an invalid value */
> +	fmt->code = format->code;
> +
> +	/* Propagate the format from sink stream to source stream */
> +	fmt = v4l2_subdev_state_get_opposite_stream_format(sd_state, sdformat->pad,
> +							   sdformat->stream);
> +	if (!fmt)
> +		return -EINVAL;

[Severity: Medium]
Should this return 0 instead of -EINVAL? If the stream is not currently
routed, v4l2_subdev_state_get_opposite_stream_format() correctly returns
NULL. V4L2 subdevice UAPI semantics allow userspace to set stream formats
regardless of active routing, so it might be better to just ignore
propagation and return 0.

> +
> +	*fmt = sdformat->format;
> +
> +	return 0;
> +}

[ ... ]

> +static void csi_formatter_stop_stream(struct csi_formatter *formatter,
> +				      u64 stream_mask)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < CSI_FORMATTER_VC_NUM; ++i) {
> +		if (!(stream_mask & BIT(i)))
> +			continue;
> +
> +		csi_formatter_write(formatter, CSI_VC_PIXEL_DATA_TYPE(i), 0);

[Severity: High]
Does this clear the wrong hardware registers? In csi_formatter_start_stream(),
the driver correctly resolves the physical Virtual Channel ID (vc) and writes
to CSI_VC_PIXEL_DATA_TYPE(vc).

Here, the logical stream index i is used as the VC ID. Since logical stream
IDs do not necessarily match physical VC IDs, this might fail to disable the
intended stream and potentially corrupt other active streams.

> +	}
> +}

[ ... ]

> +static int csi_formatter_subdev_enable_streams(struct v4l2_subdev *sd,
> +					       struct v4l2_subdev_state *state,
> +					       u32 pad, u64 streams_mask)
> +{
> +	struct csi_formatter *formatter = sd_to_formatter(sd);
> +	struct device *dev = formatter->dev;
> +	u64 sink_streams;
> +	int ret;
> +
> +	sink_streams = v4l2_subdev_state_xlate_streams(state,
> +						       CSI_FORMATTER_PAD_SOURCE,
> +						       CSI_FORMATTER_PAD_SINK,
> +						       &streams_mask);
> +	if (!sink_streams || !streams_mask)
> +		return -EINVAL;
> +
> +	guard(mutex)(&formatter->lock);
> +
> +	if (!formatter->enabled_streams) {
> +		ret = pm_runtime_resume_and_get(formatter->dev);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to resume runtime PM: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	ret = csi_formatter_start_stream(formatter, state, streams_mask);

[Severity: High]
Should this pass sink_streams instead of streams_mask? streams_mask
applies to the source pad, but it is passed directly to
csi_formatter_start_stream(). Inside that helper,
v4l2_subdev_state_get_format() is called using the bits from the source
mask but with CSI_FORMATTER_PAD_SINK.

If the routing maps a sink stream to a differently-numbered source stream,
the format query for the sink pad could return NULL, resulting in a NULL
pointer dereference.

> +	if (ret)
> +		goto err_runtime_put;

[Severity: Medium]
Does this mix goto-based error handling with scope-based cleanups?
According to the kernel cleanup subsystem guidelines, functions must not mix
goto-based error handling with scope-based cleanup macros like guard().
This creates confusing ownership semantics and increases the risk of bugs
during future modifications.

> +
> +	ret = v4l2_subdev_enable_streams(formatter->csi_sd,
> +					 formatter->remote_pad,
> +					 sink_streams);
> +	if (ret)
> +		goto err_stop_stream;
> +
> +	formatter->enabled_streams |= streams_mask;
> +
> +	return 0;
> +
> +err_stop_stream:
> +	csi_formatter_stop_stream(formatter, streams_mask);
> +err_runtime_put:
> +	if (!formatter->enabled_streams)
> +		pm_runtime_put(formatter->dev);
> +	return ret;
> +}

[ ... ]

> +static int csi_formatter_async_register(struct csi_formatter *formatter)
> +{
> +	struct device *dev = formatter->dev;
> +	struct v4l2_async_connection *asc;
> +	int ret;
> +
> +	struct fwnode_handle *ep __free(fwnode_handle) =
> +		fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
> +						FWNODE_GRAPH_ENDPOINT_NEXT);
> +	if (!ep)
> +		return -ENOTCONN;
> +
> +	v4l2_async_subdev_nf_init(&formatter->notifier, &formatter->sd);
> +
> +	asc = v4l2_async_nf_add_fwnode_remote(&formatter->notifier, ep,
> +					      struct v4l2_async_connection);
> +	if (IS_ERR(asc)) {
> +		ret = PTR_ERR(asc);
> +		goto err_cleanup_notifier;

[Severity: Medium]
Similar to above, does this mix goto-based error handling with scope-based
cleanups? The function uses __free() for ep but then uses goto for error
handling.

> +	}
> +
> +	formatter->notifier.ops = &formatter_notify_ops;
> +
> +	ret = v4l2_async_nf_register(&formatter->notifier);
> +	if (ret)
> +		goto err_cleanup_notifier;
> +
> +	ret = v4l2_async_register_subdev(&formatter->sd);
> +	if (ret)
> +		goto err_unregister_notifier;
> +
> +	return 0;
> +
> +err_unregister_notifier:
> +	v4l2_async_nf_unregister(&formatter->notifier);
> +err_cleanup_notifier:
> +	v4l2_async_nf_cleanup(&formatter->notifier);
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525-csi_formatter-v8-0-6b646231224b@oss.nxp.com?part=2