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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Missing NULL pointer check for v4l2_subdev_state_get_opposite_stream_format().
- [High] Potential NULL pointer dereference when accessing the index of the remote pad.
- [High] Use-After-Free (UAF) due to missing .unbind callback for the remote subdevice.
- [Medium] Runtime PM autosuspend is bypassed due to incorrect put API usage.
- [Medium] Hardware streams are left active if getting the frame descriptor fails during stop.
- [Low] Mixing goto-based error handling and __free() resource cleanup.
- [Low] Out-of-bounds bitwise shift when route->source_stream is greater than or equal to 64.
--

--- Patch [3]: [PATCH v14 3/3] media: nxp: Add i.MX95 CSI pixel formatter v4l2 driver ---
commit 0950cb0319055988838660ff8ee3c761b6925d73
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
> --- /dev/null
> +++ b/drivers/media/platform/nxp/imx95-csi-formatter.c

[ ... ]

> +static int csi_formatter_subdev_set_fmt(struct v4l2_subdev *sd,
> +					struct v4l2_subdev_state *sd_state,
> +					struct v4l2_subdev_format *format)
> +{

[ ... ]

> +	/* Propagate the format from sink stream to source stream */
> +	fmt = v4l2_subdev_state_get_opposite_stream_format(sd_state, format->pad,
> +							   format->stream);
> +	*fmt = format->format;

[Severity: High]
If userspace configures the routing to disable or omit the route for a specific
sink stream, could v4l2_subdev_state_get_opposite_stream_format() return NULL
here and cause a dereference?

[ ... ]

> +static void csi_formatter_stop_stream(struct csi_formatter *formatter,
> +				      struct v4l2_subdev_state *state,
> +				      u64 stream_mask)
> +{

[ ... ]

> +	ret = v4l2_subdev_call(formatter->remote_sd, pad, get_frame_desc,
> +			       formatter->remote_pad, &fd);
> +	if (ret < 0 && ret != -ENOIOCTLCMD) {
> +		dev_err(formatter->dev, "Failed to get frame desc: %d\n", ret);
> +		return;
> +	}

[Severity: Medium]
If the remote subdevice's get_frame_desc callback returns an error, does
this early return bypass the register clearing phase? Since the driver
continues the teardown process, this might leave the hardware streams active.

> +
> +	if (ret == -ENOIOCTLCMD) {
> +		fd.type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> +		fd.num_entries = 1;
> +	}
> +
> +	for_each_active_route(&state->routing, route) {
> +		if (!(stream_mask & BIT_ULL(route->source_stream)))
> +			continue;

[Severity: Low]
If userspace configures route->source_stream to a value greater than 63,
does this trigger an out-of-bounds bitwise shift?

[ ... ]

> +static int csi_formatter_start_stream(struct csi_formatter *formatter,
> +				      struct v4l2_subdev_state *state,
> +				      u64 stream_mask)
> +{

[ ... ]

> +	for_each_active_route(&state->routing, route) {
> +		if (!(stream_mask & BIT_ULL(route->source_stream)))
> +			continue;

[Severity: Low]
Does this also run into an out-of-bounds shift if route->source_stream
exceeds 63?

[ ... ]

> +static int csi_formatter_subdev_enable_streams(struct v4l2_subdev *sd,
> +					       struct v4l2_subdev_state *state,
> +					       u32 pad, u64 streams_mask)
> +{

[ ... ]

> +	ret = v4l2_subdev_enable_streams(formatter->remote_sd,
> +					 formatter->remote_pad,
> +					 sink_streams);

[Severity: High]
Without an .unbind callback to clear formatter->remote_sd when the remote
subdevice is removed, could this result in a use-after-free if the remote
module is unloaded and we later try to enable streams?

[ ... ]

> +err_runtime_put:
> +	if (!formatter->enabled_streams)
> +		pm_runtime_put(formatter->dev);
> +	return ret;
> +}

[Severity: Medium]
Does dropping the PM reference here without calling pm_runtime_mark_last_busy()
bypass the 1000ms autosuspend delay, causing the device to suspend instantly?

> +
> +static int csi_formatter_subdev_disable_streams(struct v4l2_subdev *sd,
> +						struct v4l2_subdev_state *state,
> +						u32 pad, u64 streams_mask)
> +{

[ ... ]

> +	if (!formatter->enabled_streams)
> +		pm_runtime_put(formatter->dev);
> +
> +	return ret;
> +}

[Severity: Medium]
Similarly, does this pm_runtime_put() also skip marking the device busy,
missing the autosuspend delay?

[ ... ]

> +static int csi_formatter_notify_bound(struct v4l2_async_notifier *notifier,
> +				      struct v4l2_subdev *sd,
> +				      struct v4l2_async_connection *asc)
> +{

[ ... ]

> +	ret = v4l2_create_fwnode_links_to_pad(sd, sink, link_flags);
> +	if (ret < 0)
> +		return ret;
> +
> +	formatter->remote_pad = media_pad_remote_pad_first(sink)->index;

[Severity: High]
If v4l2_create_fwnode_links_to_pad() returns 0 but hasn't actually created
any links (for example, due to a malformed device tree), could
media_pad_remote_pad_first(sink) return NULL and cause a crash here?

> +
> +	return 0;
> +}
> +
> +static const struct v4l2_async_notifier_operations formatter_notify_ops = {
> +	.bound = csi_formatter_notify_bound,
> +};

[Severity: High]
Is this missing an .unbind callback to clean up formatter->remote_sd when the
remote subdevice is unregistered?

> +
> +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);

[ ... ]

> +err_unregister_notifier:
> +	v4l2_async_nf_unregister(&formatter->notifier);
> +err_cleanup_notifier:
> +	v4l2_async_nf_cleanup(&formatter->notifier);
> +	return ret;
> +}

[Severity: Low]
Is it safe to mix the __free() cleanup attribute for the fwnode_handle with
traditional goto-based error handling in this function? The cleanup
guidelines recommend converting all resources to scope-based cleanup or
none at all to avoid future double-free issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-csi_formatter-v14-0-ca239fbb441f@oss.nxp.com?part=3