Re: [PATCH v13 15/22] media: i2c: add Maxim GMSL2/3 deserializer framework

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [Critical] Missing bounds check in `max_des_get_frame_desc_state` allows a stack buffer overflow when writing to `fd->entry`.
- [High] Fwnode endpoint reference leaks on probe error paths.
- [High] Asynchronous I2C client binding notifier incorrectly assumes the first bound client is the serializer, blocking other I2C devices.
- [High] Hardware pipes 4-7 will never be used due to a flawed 1:1 lookup mapping in `max_des_find_link_pipe`.
- [High] Bypassing I2C mux/ATR locking in `max_des_init_link_ser_xlate` leads to I2C bus corruption.
- [High] Unprotected access to `pipe->remaps` and `pipe->vc_remaps` during the `VIDIOC_LOG_STATUS` ioctl.
- [High] TOCTOU race on `hw.source->sd` allows dereferencing a NULL or freed subdev pointer during stream updates.
- [High] Invalid validation check of physical data lane indices against the active lane count in `max_des_parse_src_dt_endpoint`.
- [Medium] `max_des_set_fmt` rejects format changes for unrouted streams, violating the V4L2 subdev API contract.
- [Medium] Returning `-EINVAL` on unsupported formats and intervals violates the V4L2 specification.
- [Medium] Use of `devm_kcalloc` and `devm_kfree` in hotpaths causes severe devres list traversal overhead.
- [Medium] The `V4L2_CID_TEST_PATTERN` control does not immediately apply to the hardware, and stream restarts ignore the change.
- [Medium] Returning early on `regulator_disable` failure in `max_des_update_pocs` aborts cleanup for remaining POC supplies.
--

commit 10b6032537ce5fb1d90f4cb0131ac25ff19cabea
Author: Cosmin Tanislav <demonsingur@gmail.com>

media: i2c: add Maxim GMSL2/3 deserializer framework

This commit introduces a common framework for Maxim GMSL2/3 deserializers. It
adds support for the Streams API, I2C ATR, automatic GMSL link version
negotiation, stream ID selection, VC remapping, and multiple other hardware
features across all supported chips.

> diff --git a/drivers/media/i2c/maxim-serdes/max_des.c b/drivers/media/i2c/maxim-serdes/max_des.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/media/i2c/maxim-serdes/max_des.c
[ ... ]
> +static struct max_des_pipe *
> +max_des_find_link_pipe(struct max_des *des, struct max_des_link *link)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < des->info->num_pipes; i++) {
> +		struct max_des_pipe *pipe = &des->pipes[i];
> +
> +		if (pipe->link_id == link->index)
> +			return pipe;
> +	}
> +
> +	return NULL;
> +}

[Severity: High]
Does this force a 1:1 mapping between links and pipes? Since the assignment
uses a modulo operation elsewhere to map multiple pipes to a single link,
returning on the first match appears to prevent pipes 4-7 from ever being
selected. Can this break demultiplexing for multiple streams on the same link?

[ ... ]
> +static int max_des_set_modes(struct max_des_priv *priv,
> +			     struct max_des_mode_context *context)
> +{
[ ... ]
> +	for (i = 0; i < des->info->num_links; i++) {
> +		struct max_des_link_hw hw;
> +		u32 pipe_double_bpps = 0;
[ ... ]
> +		if (!hw.source->sd)
> +			continue;
> +
> +		pipe_double_bpps = context->pipes_double_bpps[hw.pipe->index];
> +
> +		ret = max_ser_set_double_bpps(hw.source->sd, pipe_double_bpps);

[Severity: High]
Does this access contain a time-of-check to time-of-use race?

The subdev pointer is checked for NULL, but the async notifier unbind
callback can clear this pointer concurrently without holding the state lock.

Could the pointer become NULL just before it is used?

[ ... ]
> +static int max_des_update_pipe_remaps(struct max_des_priv *priv,
> +				      struct max_des_remap_context *context,
> +				      struct max_des_pipe *pipe,
> +				      struct v4l2_subdev_state *state,
> +				      u64 *streams_masks)
> +{
[ ... ]
> +	remaps = devm_kcalloc(priv->dev, des->info->num_remaps_per_pipe,
> +			      sizeof(*remaps), GFP_KERNEL);
> +	if (!remaps)
> +		return -ENOMEM;
[ ... ]
> +	if (pipe->remaps)
> +		devm_kfree(priv->dev, pipe->remaps);
> +
> +	pipe->remaps = remaps;
> +	pipe->num_remaps = num_remaps;

[Severity: Medium]
Is it safe to use device-managed allocations in this hotpath?

Allocating and freeing memory via the devres API during stream state changes
forces a traversal of the device resource list every time. Could this cause
severe overhead and lock contention?

[ ... ]
> +static int max_des_init_link_ser_xlate(struct max_des_priv *priv,
> +				       struct max_des_link *link,
> +				       struct i2c_adapter *adapter,
> +				       u8 power_up_addr, u8 new_addr)
> +{
[ ... ]
> +	ret = des->ops->select_links(des, BIT(link->index));
> +	if (ret)
> +		return ret;
> +
> +	ret = max_ser_wait_for_multiple(adapter, addrs, ARRAY_SIZE(addrs),
> +					&current_addr);

[Severity: High]
Can this sequence cause I2C bus corruption?

The code modifies the hardware mux state directly via select_links and
performs transfers on the parent adapter without acquiring the I2C mux core
lock.

If another thread initiates a transfer on an already-bound child adapter
concurrently, could its traffic be routed to the wrong hardware link?

[ ... ]
> +static int max_des_ser_attach_addr(struct max_des_priv *priv, u32 chan_id,
> +				   u16 addr, u16 alias)
> +{
[ ... ]
> +	if (link->ser_xlate.en) {
> +		dev_err(priv->dev, "Serializer for link %u already bound\n",
> +			link->index);
> +		return -EINVAL;
> +	}

[Severity: High]
Does this logic unintentionally block other I2C devices from binding?

Once the serializer binds, this returns an error for any subsequent device
like a camera sensor on the same link. Could this prevent the ATR core from
successfully allocating aliases for those devices?

[ ... ]
> +static int max_des_get_frame_desc_state(struct v4l2_subdev *sd,
> +					struct v4l2_subdev_state *state,
> +					struct v4l2_mbus_frame_desc *fd,
> +					unsigned int pad)
> +{
[ ... ]
> +		hw.entry.bus.csi2.vc = dst_vc_id;
> +		hw.entry.stream = route->source_stream;
> +
> +		fd->entry[fd->num_entries++] = hw.entry;
> +	}

[Severity: Critical]
Is there a bounds check missing for the array access here?

The entry array has a fixed capacity, but userspace can configure a large
number of routes. If more active streams map to the same source pad than the
array can hold, could this write out of bounds on the stack?

[ ... ]
> +static int max_des_set_tpg_fmt(struct v4l2_subdev *sd,
> +			       struct v4l2_subdev_state *state,
> +			       struct v4l2_subdev_format *format)
> +{
[ ... ]
> +	entry = max_des_find_tpg_entry(des, 0, fmt->width, fmt->height,
> +				       fmt->code, 0, 0);
> +	if (!entry)
> +		return -EINVAL;

[Severity: Medium]
Does returning an error for unsupported formats violate the V4L2 API
contract? The specification mandates that unsupported requested formats
should be adjusted to the closest valid match rather than rejected.

[ ... ]
> +static int max_des_set_fmt(struct v4l2_subdev *sd,
> +			   struct v4l2_subdev_state *state,
> +			   struct v4l2_subdev_format *format)
> +{
[ ... ]
> +	fmt = v4l2_subdev_state_get_opposite_stream_format(state, format->pad,
> +							   format->stream);
> +	if (!fmt)
> +		return -EINVAL;

[Severity: Medium]
Does returning an error for unrouted streams violate the V4L2 API contract?

The specification mandates that format configuration should succeed
independently of the routing table.

[ ... ]
> +static int max_des_set_frame_interval(struct v4l2_subdev *sd,
> +				      struct v4l2_subdev_state *state,
> +				      struct v4l2_subdev_frame_interval *fi)
> +{
[ ... ]
> +	entry = max_des_find_tpg_entry(des, 0, fmt->width, fmt->height,
> +				       fmt->code, fi->interval.denominator,
> +				       fi->interval.numerator);
> +	if (!entry)
> +		return -EINVAL;

[Severity: Medium]
Similar to the format configuration, does returning an error for unsupported
intervals violate the V4L2 API contract? The specification mandates that
drivers adjust unsupported intervals instead of rejecting them.

[ ... ]
> +static int max_des_log_status(struct v4l2_subdev *sd)
> +{
[ ... ]
> +		if (des->ops->set_pipe_remap) {
> +			v4l2_info(sd, "\tremaps: %u\n", pipe->num_remaps);
> +			for (j = 0; j < pipe->num_remaps; j++) {
> +				struct max_des_remap *remap = &pipe->remaps[j];

[Severity: High]
Can this loop dereference freed memory?

The log status callback executes without holding the subdev state lock. If
another thread concurrently triggers a stream update, it might free and
replace the remaps array while it is being read here.

[ ... ]
> +static int max_des_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct max_des_priv *priv = ctrl_to_priv(ctrl->handler);
> +	struct max_des *des = priv->des;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_TEST_PATTERN:
> +		des->tpg_pattern = ctrl->val;
> +		return 0;
> +	}

[Severity: Medium]
Does this control handler fail to apply the test pattern to the hardware?
It updates the software state but does not seem to invoke the hardware
callback.

[ ... ]
> +static int max_des_update_tpg(struct max_des_priv *priv,
> +			      struct v4l2_subdev_state *state,
> +			      u64 *streams_masks)
> +{
[ ... ]
> +	if (entry == des->tpg_entry)
> +		return 0;
> +
> +	ret = des->ops->set_tpg(des, entry);

[Severity: Medium]
Does this stream restart optimization ignore pending test pattern changes?

Because the code returns early if the format has not changed, it appears a new
test pattern selected via userspace will not be applied to the hardware until
the resolution is also modified.

[ ... ]
> +static int max_des_update_pocs(struct max_des_priv *priv, bool enable)
> +{
[ ... ]
> +		if (enable)
> +			ret = regulator_enable(priv->pocs[i]);
> +		else
> +			ret = regulator_disable(priv->pocs[i]);
> +
> +		if (ret) {
> +			dev_err(priv->dev,
> +				"Failed to set POC supply to %u: %u\n",
> +				enable, ret);
> +			if (!enable)
> +				return ret;

[Severity: Medium]
Is it safe to return early when disabling regulators?

If one regulator fails to disable during driver unbind or system suspend,
this breaks out of the loop and leaves all subsequent regulators perpetually
powered on.

[ ... ]
> +static int max_des_parse_src_dt_endpoint(struct max_des_priv *priv,
> +					 struct max_des_phy *phy,
> +					 struct fwnode_handle *fwnode)
> +{
[ ... ]
> +	for (i = 0; i < mipi->num_data_lanes; i++) {
> +		if (mipi->data_lanes[i] > mipi->num_data_lanes) {
> +			dev_err(priv->dev, "Invalid data lane %u on port %u\n",
> +				mipi->data_lanes[i], pad);
> +			return -EINVAL;
> +		}
> +	}

[Severity: High]
Is this validation comparing the physical lane index against the active lane
count?

The array holds physical pin numbers, while the active count dictates the
number of used lanes. If a board routes higher numbered pins but uses fewer
total lanes, this check seems to erroneously reject valid physical
configurations.

[ ... ]
> +static int max_des_parse_dt(struct max_des_priv *priv)
> +{
[ ... ]
> +	for (i = 0; i < des->info->num_links; i++) {
> +		struct max_des_link *link = &des->links[i];
> +		struct max_serdes_source *source;
> +
> +		source = max_des_get_link_source(priv, link);
> +		source->index = i;
> +
> +		ret = max_des_parse_sink_dt_endpoint(priv, link, source, fwnode);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Do the error paths in probe leak the fwnode endpoint references?

When the parse sink endpoint function acquires a reference, it seems the
cleanup only happens on successful driver teardown. If initialization fails
in this loop or later in probe, the acquired references are not released.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604-gmsl2-3_serdes-v13-0-9d8a4919983b@analog.com?part=15