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

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

On Thu, Jul 16, 2026 at 07:20:52AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] Missing NULL checks for the return value of `v4l2_subdev_state_get_format()` allow userspace to trigger a kernel panic.
> - [High] The driver caches a pointer to the remote subdevice when bound, but fails to implement an `.unbind` callback to clear it, leading to a Use-After-Free.
> - [Medium] Mixing `goto`-based error handling with scope-based cleanup (`__free` and `guard`) in the same function violates kernel cleanup guidelines.
> - [Medium] `csi_formatter_subdev_set_fmt` partially modifies the subdevice state but returns `-EINVAL` if the stream has no active route.
> - [Medium] The `csi_formatter_read` wrapper ignores the return value of `regmap_read`, potentially leading to uninitialized stack variable usage.
> --
>
> --- Patch [3]: [PATCH v12 3/3] media: nxp: Add i.MX95 CSI pixel formatter v4l2 driver ---
> commit 0cff6156940653a6a408d93ee678b339b7218ebb
> Author: Guoniu Zhou <guoniu.zhou@nxp.com>
> Subject: 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. Add data formatting support.
>
> > 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..cea60327c9729
> > --- /dev/null
> > +++ b/drivers/media/platform/nxp/imx95-csi-formatter.c
>
> [ ... ]
>
> > +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 this cause a NULL pointer dereference?
>
> The V4L2 core does not validate the stream ID provided by userspace in
> VIDIOC_SUBDEV_ENUM_MBUS_CODE against the active routing table before calling
> this function. If a user passes an invalid stream ID,
> v4l2_subdev_state_get_format() returns NULL, which is then unconditionally
> dereferenced here.

Is it true?

>
> > +		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]
> Does this also risk a NULL pointer dereference for the same reason?
> Userspace can provide an invalid stream ID during VIDIOC_SUBDEV_S_FMT,
> resulting in v4l2_subdev_state_get_format() returning NULL.
>
> > +
> > +	/* Set default code if user set an invalid value */
> > +	fmt->code = format->code;
> > +	sdformat->format = *fmt;
> > +
> > +	/* 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]
> Does this leave the subdevice in an inconsistent state?
>
> If a user configures the format on a sink pad stream that does not currently
> have an active route to a source pad, v4l2_subdev_state_get_opposite_stream_format()
> returns NULL. This causes the driver to return -EINVAL after the sink format
> has already been updated in the state earlier in this function. Setting a
> format on an unrouted stream is generally a valid V4L2 operation.

look like make sense,

Does v4l2_subdev_state_get_opposite_stream_format() call before fmt->code?

>
> > +
> > +	*fmt = sdformat->format;
> > +
> > +	return 0;
> > +}
>
> [ ... ]
>
> > +static inline void csi_formatter_read(struct csi_formatter *formatter,
> > +				      unsigned int reg, unsigned int *value)
> > +{
> > +	u32 offset = formatter->reg_offset;
> > +
> > +	regmap_read(formatter->regs, reg + offset, value);
> > +}
>
> [Severity: Medium]
> Can this write uninitialized stack memory to hardware registers?
>
> The wrapper csi_formatter_read() ignores the return value of regmap_read().
> If the read fails, local stack variables remain uninitialized.
>
> For example, when called from csi_formatter_start_stream():
>
>     u32 val;
>     ...
>     csi_formatter_read(formatter, CSI_VC_PIXEL_DATA_TYPE(vc), &val);
>     val |= BIT(csi_formatter_get_index_by_dt(pix_fmt->data_type));
>     csi_formatter_write(formatter, CSI_VC_PIXEL_DATA_TYPE(vc), val);
>
> The driver performs bitwise operations on val and writes it back to the
> CSI_VC_PIXEL_DATA_TYPE register. If regmap_read() failed, this writes
> garbage data to the hardware.

It should be fine. MMIO never return failure.

>
> [ ... ]
>
> > +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);
> > +	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, state, streams_mask);
> > +err_runtime_put:
> > +	if (!formatter->enabled_streams)
> > +		pm_runtime_put(formatter->dev);
> > +	return ret;
> > +}
>
> [Severity: Medium]
> Is it safe to mix guard() with goto labels in the same function?
>
> The kernel cleanup guidelines note that the expectation is that usage of
> goto and cleanup helpers is never mixed in the same function to avoid
> resource leaks or double-free bugs during future maintenance.

This should be fine if goto to error path only

Frank

>
> [ ... ]
>
> > +static const struct v4l2_async_notifier_operations formatter_notify_ops = {
> > +	.bound = csi_formatter_notify_bound,
> > +};
>
> [Severity: High]
> Could this result in a use-after-free?
>
> The driver saves the remote subdev pointer in csi_formatter_notify_bound()
> via formatter->csi_sd = sd, but there is no unbind callback in
> formatter_notify_ops to clear formatter->csi_sd.
>
> If the remote subdevice is unbound (e.g. the sensor module is removed), the
> V4L2 async framework tears down the link. Because the pointer isn't cleared,
> subsequent calls to VIDIOC_SUBDEV_ENABLE_STREAMS on the still-registered
> formatter video node will dereference the freed formatter->csi_sd pointer
> in csi_formatter_subdev_enable_streams().
>
> [ ... ]
>
> > +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;
> > +	}
> > +
> > +	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;
> > +}
>
> [Severity: Medium]
> Does mixing __free() scope declarations with goto error labels here risk
> cleanup ordering issues?
>
> The kernel guidelines specify that functions should either convert all
> resources that need a goto cleanup to scope-based cleanup, or convert none
> of them.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260716-csi_formatter-v12-0-2601d17fc4af@oss.nxp.com?part=3