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

[Frank Li <Frank.li@oss.nxp.com>]

On Tue, Jun 23, 2026 at 04:05:48AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
> - [High] Missing NULL check for the return value of v4l2_subdev_state_get_format() in pad operations, causing a NULL pointer dereference.
> - [High] The driver incorrectly assumes source stream IDs match sink stream IDs during stream start, leading to a NULL pointer dereference.
> - [High] Unbalanced PM runtime put in `csi_formatter_subdev_disable_streams` due to state desynchronization on failure.
> - [High] Dangling `csi_sd` pointer leading to Use-After-Free if the remote subdevice is unbound.
> - [Medium] The driver fails to validate stream ID bounds, silently ignoring stream IDs >= 8.
> - [Medium] Overwriting the Virtual Channel Data Type register breaks multiplexed streams sharing the same VC.
> - [Medium] Mixing `goto`-based error handling with scope-based cleanup helpers (`__free` and `guard`) in the same function.
> - [Medium] Setting the format on an unrouted sink stream incorrectly fails with `-EINVAL`.
> --
>
> commit cca7db4730915ca13295d8053b3569d5e6266e0d
> Author: Guoniu Zhou <guoniu.zhou@nxp.com>
> Subject: media: nxp: Add i.MX95 CSI pixel formatter v4l2 driver
>
> This commit introduces the i.MX95 CSI pixel formatter v4l2 driver,
> adding data formatting support to match Pixel Link (PL) definitions.
>
> 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..cfe448fedd37e
> --- /dev/null
> +++ b/drivers/media/platform/nxp/imx95-csi-formatter.c
> @@ -0,0 +1,775 @@
> [ ... ]
> > +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]
> Is there a missing NULL check here?
>
> If the requested stream is not found in the state,
> v4l2_subdev_state_get_format() returns NULL, which would cause a crash when
> dereferencing fmt->code.
>
> Userspace might trigger this by passing an invalid stream ID.

Does it really happen?

>
> > +		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;
> [ ... ]
> > +	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 this result in a NULL pointer dereference?
>
> Similar to the enum_mbus_code operation above, v4l2_subdev_state_get_format()
> can return NULL if the stream is invalid.
>
> > +
> > +	/* 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 -EINVAL when there is no internal route configured?
>
> Valid requests to set the format on an unrouted sink stream will fail here,
> which seems to violate V4L2 semantics where the format on the local sink pad
> should still be accepted even without an active route.
>
> > +
> > +	*fmt = sdformat->format;
> > +
> > +	return 0;
> > +}
> [ ... ]
> > +static int csi_formatter_start_stream(struct csi_formatter *formatter,
> > +				      struct v4l2_subdev_state *state,
> > +				      u64 stream_mask)
> > +{
> [ ... ]
> > +	for (i = 0; i < CSI_FORMATTER_VC_NUM; ++i) {
> > +		if (!(stream_mask & BIT(i)))
> > +			continue;
>
> [Severity: Medium]
> Does this fail to configure stream IDs higher than 7?
>
> The loop terminates at CSI_FORMATTER_VC_NUM (which is 8), silently skipping
> any higher stream IDs present in the stream_mask.
>
> V4L2 subdev routing allows stream IDs up to 63.

Is it true?


>
> > +
> > +		fmt = v4l2_subdev_state_get_format(state,
> > +						   CSI_FORMATTER_PAD_SINK, i);
> > +
> > +		pix_fmt = csi_formatter_find_format(fmt->code);
>
> [Severity: High]
> Could this crash if the source stream ID does not match the sink stream ID?
>
> The index i iterates over the source pad's stream_mask, but it is used here
> to query the format of CSI_FORMATTER_PAD_SINK.
>
> If userspace configures a route where the sink stream ID differs from the
> source stream ID, get_format() will return NULL, leading to a crash when
> accessing fmt->code.
>
> > +
> > +		val = BIT(csi_formatter_get_index_by_dt(pix_fmt->data_type));
> [ ... ]
> > +		/* Store the stream to VC mapping for stop_stream */
> > +		formatter->stream_to_vc[i] = vc;
> > +
> > +		csi_formatter_write(formatter, CSI_VC_PIXEL_DATA_TYPE(vc), val);
>
> [Severity: Medium]
> Will this break multiplexed streams sharing the same virtual channel?
>
> Writing directly to the register completely overwrites the configuration
> with the new single bit, which would destroy any existing active data type
> bindings for that virtual channel.
>
> Would a read-modify-write operation be more appropriate here?
>
> > +		configured_streams |= BIT(i);
> > +	}
> > +
> > +	return 0;
> > +
> > +err_cleanup:
> > +	csi_formatter_stop_stream(formatter, configured_streams);
> > +	return ret;
> > +}
> [ ... ]
> > +static int csi_formatter_subdev_enable_streams(struct v4l2_subdev *sd,
> > +					       struct v4l2_subdev_state *state,
> > +					       u32 pad, u64 streams_mask)
> > +{
> [ ... ]
> > +	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 = csi_formatter_start_stream(formatter, state, streams_mask);
>
> [Severity: High]
> Is the wrong mask passed to csi_formatter_start_stream here?

Please check this one?

>
> It passes the source pad's streams_mask instead of the translated
> sink_streams mask.
>
> > +	if (ret)
> > +		goto err_runtime_put;
> > +
> > +	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;
> > +}
>
> [Severity: Medium]
> Is it safe to mix goto-based error handling with scope-based cleanup helpers
> in this function?
>
> This function uses guard(mutex) for lock management but also contains goto
> labels for error unwinding. Mixing these approaches can create confusing
> resource ownership semantics.

This should be fine, can you check other comments?

Frank

>
> > +
> > +static int csi_formatter_subdev_disable_streams(struct v4l2_subdev *sd,
> > +						struct v4l2_subdev_state *state,
> > +						u32 pad, u64 streams_mask)
> > +{
> [ ... ]
> > +	ret = v4l2_subdev_disable_streams(formatter->csi_sd, formatter->remote_pad,
> > +					  sink_streams);
> > +	if (ret)
> > +		dev_err(formatter->dev, "Failed to disable streams: %d\n", ret);
> > +
> > +	csi_formatter_stop_stream(formatter, streams_mask);
> > +
> > +	formatter->enabled_streams &= ~streams_mask;
> > +
> > +	if (!formatter->enabled_streams)
> > +		pm_runtime_put(formatter->dev);
> > +
> > +	return ret;
> > +}
>
> [Severity: High]
> Can this cause an unbalanced pm_runtime_put() if disable streams fails?
>
> If the upstream v4l2_subdev_disable_streams() call fails, the function
> still clears the bit in enabled_streams and calls pm_runtime_put().
>
> If userspace later retries stopping the stream, this code will run again.
> Because enabled_streams is already zero, the condition
> !formatter->enabled_streams evaluates to true, causing a second
> pm_runtime_put() call.
>
> [ ... ]
> > +static const struct v4l2_async_notifier_operations formatter_notify_ops = {
> > +	.bound = csi_formatter_notify_bound,
> > +};
>
> [Severity: High]
> Does this missing unbind callback lead to a use-after-free?
>
> The driver stores the remote subdevice pointer in csi_sd during the bound
> callback, but does not provide an unbind callback to clear it.
>
> If the remote sensor module is unloaded, the dangling csi_sd pointer could be
> dereferenced by subsequent stream ioctls.
>
> [ ... ]
> > +static int csi_formatter_async_register(struct csi_formatter *formatter)
> > +{
> [ ... ]
> > +	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;
> [ ... ]
> > +	ret = v4l2_async_nf_register(&formatter->notifier);
> > +	if (ret)
> > +		goto err_cleanup_notifier;
> [ ... ]
> > +err_unregister_notifier:
> > +	v4l2_async_nf_unregister(&formatter->notifier);
> > +err_cleanup_notifier:
> > +	v4l2_async_nf_cleanup(&formatter->notifier);
> > +	return ret;
> > +}
>
> [Severity: Medium]
> Is it safe to mix goto-based error handling with scope-based cleanup helpers
> in this function?
>
> This function declares a __free() variable but also uses standard goto
> unwinding labels, which can lead to double-free issues or confusing
> resource semantics.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260623-csi_formatter-v11-0-a792fe9c1502@oss.nxp.com?part=3