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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 14 potential issue(s) to consider:
- [High] Missing initialization and bounds check for `fd->num_entries` in `max_des_get_frame_desc_state`.
- [High] Stack buffer overflow in `max_des_set_pipes_stream_id` via unvalidated `stream_id`.
- [High] Checking `pipe->num_vc_remaps` instead of `pipe->vc_remaps` causes sequential devm memory leaks.
- [High] I2C bus notifier relies on an adapter array that is not yet populated during synchronous driver binding.
- [High] Uninitialized `struct v4l2_mbus_frame_desc fd` is passed to the serializer subdev.
- [High] Output struct fields are used as search constraints in `max_des_enum_frame_interval`.
- [High] Unbalanced POC regulator unwinding causes warnings and halts cleanup.
- [High] Unprotected access to `source->sd` leads to TOCTOU NULL pointer dereferences and dangling pointers.
- [High] Lockless array iteration in `max_des_log_status` causes a Use-After-Free (UAF) vulnerability.
- [High] Hardware state corruption due to an I2C multiplexing race condition during serializer binding.
- [High] Incorrect `chan_id` mapping in `max_des_i2c_mux_bus_notifier_call`.
- [Medium] Fwnode reference leak in `max_des_parse_sink_dt_endpoint`.
- [Medium] Memory leak of `streams_masks` on the error path in `max_des_update_streams`.
- [Medium] I2C ATR instances and adapters are leaked if link selection fails in `max_des_i2c_atr_init`.
--

commit 83b14a77624d98c746a5672b763eea7d8ac99877
Author: Cosmin Tanislav <demonsingur@gmail.com>
Subject: media: i2c: add Maxim GMSL2/3 deserializer framework

This commit introduces a common framework for Maxim GMSL2/3 deserializers.
It adds support for features like the full Streams API, I2C ATR, automatic
GMSL link version negotiation, and automatic stream ID selection.

> diff --git a/drivers/media/i2c/maxim-serdes/max_des.c b/drivers/media/i2c/maxim-serdes/max_des.c
> new file mode 100644
> index 0000000000000..a03101678360f
> --- /dev/null
> +++ b/drivers/media/i2c/maxim-serdes/max_des.c
> @@ -0,0 +1,3205 @@
[ ... ]
> +static int max_des_route_to_hw(struct max_des_priv *priv,
> +			       struct v4l2_subdev_state *state,
> +			       struct v4l2_subdev_route *route,
> +			       struct max_des_route_hw *hw)
> +{
> +	struct max_des *des = priv->des;
> +	struct v4l2_mbus_frame_desc fd;

[Severity: High]
Could the uninitialized fd here lead to issues?

If the subdev expects fd.num_entries to start at 0, or reads other fields,
it might operate on garbage stack data.

> +	struct max_des_link *link;
> +	unsigned int i;
> +	int ret;
[ ... ]
> +static int max_des_set_pipes_stream_id(struct max_des_priv *priv)
> +{
> +	bool stream_id_usage[MAX_SERDES_STREAMS_NUM] = { 0 };
> +	struct max_des *des = priv->des;
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < des->info->num_links; i++) {
> +		struct max_des_link_hw hw;
> +		unsigned int stream_id;
[ ... ]
> +		if (ret == -EOPNOTSUPP) {
> +			/*
> +			 * Serializer does not support setting the stream id,
> +			 * retrieve its hardcoded stream id.
> +			 */
> +			ret = max_ser_get_stream_id(hw.source->sd, &stream_id);
> +		}
> +
> +		if (ret)
> +			return ret;
> +
> +		if (stream_id_usage[stream_id] && des->info->needs_unique_stream_id) {

[Severity: High]
Is a bounds check needed for stream_id before using it to index the
stream_id_usage stack array?

A malformed or unexpectedly large value from the serializer could cause an
out-of-bounds stack write here.

> +			dev_err(priv->dev, "Duplicate stream id %u\n", stream_id);
> +			return -EINVAL;
> +		}
[ ... ]
> +static int max_des_update_pipe_vc_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)
> +{
[ ... ]
> +	ret = max_des_set_pipe_vc_remaps(priv, pipe, vc_remaps, num_vc_remaps);
> +	if (ret)
> +		goto err_free_new_vc_remaps;
> +
> +	if (pipe->num_vc_remaps)
> +		devm_kfree(priv->dev, pipe->vc_remaps);

[Severity: High]
Is there a potential memory leak if the previous state allocated memory but
populated 0 remaps?

Checking pipe->num_vc_remaps instead of the pointer pipe->vc_remaps might
bypass the free on stream updates, leaving the device-managed memory leaked.

> +
> +	pipe->vc_remaps = vc_remaps;
> +	pipe->num_vc_remaps = num_vc_remaps;
> +
> +	return 0;
[ ... ]
> +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)
> +{
> +	struct max_des *des = priv->des;
> +	u8 addrs[] = { power_up_addr, new_addr };
> +	u8 current_addr;
> +	int ret;
> +
> +	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 setting the hardware mux directly via select_links and performing I2C
transactions bypass the i2c_mux core locks?

Since the bus notifier can execute concurrently while different clients probe,
this sequence could potentially overwrite the hardware mux state and misdirect
I2C traffic to the wrong serializers.

> +	if (ret) {
> +		dev_err(priv->dev,
[ ... ]
> +static int max_des_i2c_atr_init(struct max_des_priv *priv)
> +{
[ ... ]
> +	for (i = 0; i < des->info->num_links; i++) {
> +		struct max_des_link *link = &des->links[i];
> +
> +		if (!link->enabled)
> +			continue;
> +
> +		mask |= BIT(link->index);
> +	}
> +
> +	return des->ops->select_links(des, mask);

[Severity: Medium]
If des->ops->select_links(des, mask) fails here, should it clean up the
allocated resources?

Returning the error code immediately bypasses the err_add_adapters label,
which seems like it would leak the ATR object and its adapters.

> +
> +err_add_adapters:
> +	max_des_i2c_atr_deinit(priv);
[ ... ]
> +static int max_des_i2c_mux_bus_notifier_call(struct notifier_block *nb,
> +					     unsigned long event, void *device)
> +{
[ ... ]
> +	client = i2c_verify_client(dev);
> +	if (!client)
> +		return NOTIFY_DONE;
> +
> +	for (chan_id = 0; chan_id < priv->mux->max_adapters; ++chan_id) {
> +		if (client->adapter == priv->mux->adapter[chan_id])
> +			break;
> +	}

[Severity: High]
Is it guaranteed that the priv->mux->adapter array is populated at this
point?

If a serializer binds synchronously during adapter registration, this notifier
might fire before i2c_mux_add_adapter() populates the array, causing it to
silently skip link version negotiation.

> +
> +	if (chan_id == priv->mux->max_adapters)
> +		return NOTIFY_DONE;
> +
> +	max_des_ser_attach_addr(priv, chan_id, client->addr, client->addr);

[Severity: High]
Additionally, does this chan_id indexing correctly map to the hardware links?

Because i2c_mux_add_adapter() packs adapters sequentially for enabled links,
if link 0 is disabled, link 1 might sit at array index 0. This could cause
the driver to initialize link 0's data structures for a device connected to
link 1.

> +
> +	return NOTIFY_DONE;
> +}
[ ... ]
> +static int max_des_enum_frame_interval(struct v4l2_subdev *sd,
> +				       struct v4l2_subdev_state *state,
> +				       struct v4l2_subdev_frame_interval_enum *fie)
> +{
> +	struct max_des_priv *priv = v4l2_get_subdevdata(sd);
> +	struct max_des *des = priv->des;
> +	const struct max_serdes_tpg_entry *entry;
> +
> +	if (!max_des_pad_is_tpg(des, fie->pad) ||
> +	    fie->stream != MAX_SERDES_TPG_STREAM)
> +		return -ENOTTY;
> +
> +	entry = max_des_find_tpg_entry(des, fie->index, fie->width, fie->height,
> +				       fie->code, fie->interval.denominator,
> +				       fie->interval.numerator);

[Severity: High]
Is it safe to use fie->interval.numerator and denominator as search
constraints here?

Since interval is an output parameter during
VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL, userspace might provide uninitialized
memory, which would cause the search to fail with garbage data.

> +	if (!entry)
> +		return -EINVAL;
[ ... ]
> +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]
Does this function need to hold the subdev active state lock while iterating
over pipe->remaps and pipe->vc_remaps?

Concurrent stream updates might free and reallocate these arrays under the
lock, which could lead to a use-after-free here.

> +
> +				v4l2_info(sd, "\t\tremap: from: vc: %u, dt: 0x%02x\n",
> +					  remap->from_vc, remap->from_dt);
[ ... ]
> +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)
> +{
> +	struct max_des_remap_context context = { 0 };
> +	struct max_des_priv *priv = sd_to_priv(sd);
> +	struct v4l2_subdev_route *route;
> +	int ret;
> +
> +	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;

[Severity: High]
Should fd->num_entries be explicitly initialized to 0 here before appending
new entries?

Since it appears this might be called with an uninitialized stack variable
from other places (like max_des_route_to_hw), it could result in an
out-of-bounds write to the array.

> +
> +	ret = max_des_populate_remap_context(priv, &context, state);
> +	if (ret)
[ ... ]
> +static int max_des_update_streams(struct v4l2_subdev *sd,
> +				  struct v4l2_subdev_state *state,
> +				  u32 pad, u64 updated_streams_mask, bool enable)
> +{
[ ... ]
> +	ret = max_des_set_modes(priv, &mode_context);
> +	if (ret)
> +		goto err_free_streams_masks;
> +
> +	ret = max_des_set_vc_remaps(priv, &context, state, streams_masks);
> +	if (ret)
> +		return ret;

[Severity: Medium]
If max_des_set_vc_remaps() fails, should this go to
err_free_streams_masks?

Returning the error code directly seems to skip freeing the dynamically
allocated streams_masks.

> +
> +	ret = max_des_set_pipes_stream_id(priv);
[ ... ]
> +static int max_des_notify_bound(struct v4l2_async_notifier *nf,
> +				struct v4l2_subdev *subdev,
> +				struct v4l2_async_connection *base_asc)
> +{
[ ... ]
> +	source->sd = subdev;
> +	source->pad = ret;
> +
> +	ret = media_create_pad_link(&source->sd->entity, source->pad,
> +				    &priv->sd.entity, pad,
> +				    MEDIA_LNK_FL_ENABLED |
> +				    MEDIA_LNK_FL_IMMUTABLE);
> +	if (ret) {
> +		dev_err(priv->dev, "Unable to link %s:%u -> %s:%u\n",
> +			source->sd->name, source->pad, priv->sd.name, pad);

[Severity: High]
If media_create_pad_link() fails here, should source->sd be set back to
NULL?

Leaving it populated on failure might result in a dangling pointer.

> +		return ret;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static void max_des_notify_unbind(struct v4l2_async_notifier *nf,
> +				  struct v4l2_subdev *subdev,
> +				  struct v4l2_async_connection *base_asc)
> +{
> +	struct max_serdes_asc *asc = asc_to_max(base_asc);
> +	struct max_serdes_source *source = asc->source;
> +
> +	source->sd = NULL;

[Severity: High]
Could setting source->sd = NULL here without locking race against streaming
paths like max_des_set_tunnel()?

This looks like it could lead to a time-of-check to time-of-use NULL pointer
dereference.

> +}
[ ... ]
> +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);

[Severity: High]
If regulator_enable() fails here, the function returns without disabling
the already-enabled links.

During the error path cleanup, this function is called again with
enable = false, which tries to disable the regulator that originally failed.
Could this trigger a kernel warning and halt further unwinding, leaving
genuine regulators permanently enabled?

> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static int max_des_parse_sink_dt_endpoint(struct max_des_priv *priv,
> +					  struct max_des_link *link,
> +					  struct max_serdes_source *source,
> +					  struct fwnode_handle *fwnode)
> +{
[ ... ]
> +	ep = fwnode_graph_get_endpoint_by_id(fwnode, pad, 0, 0);
> +	if (!ep)
> +		return 0;
> +
> +	source->ep_fwnode = fwnode_graph_get_remote_endpoint(ep);
> +	fwnode_handle_put(ep);
> +	if (!source->ep_fwnode) {

[Severity: Medium]
Does this code leak the fwnode handle acquired for source->ep_fwnode?

It doesn't seem to be released via fwnode_handle_put() anywhere in the
driver's teardown path or error cleanup.

> +		dev_err(priv->dev,
> +			"Failed to get remote endpoint on port %u\n", pad);
> +		return -ENODEV;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-gmsl2-3_serdes-v12-0-b26d92931196@analog.com?part=15