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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Stack buffer overflow in max_des_get_frame_desc_state when populating fd->entry
- [High] Architectural flaw in max_des_ser_attach_addr assumes any newly bound I2C client is the serializer
- [High] NULL pointer dereference due to missing checks for optional callback operations in max_des_ops
- [High] I2C multiplexer link selection is hijacked during multi-step initialization due to bypassed bus locking
- [High] TOCTOU Use-After-Free race on source->sd pointer when a subdevice is unbound
- [Medium] V4L2_CID_TEST_PATTERN change is ignored during active streaming
- [Medium] Dynamic devm allocations (devm_kcalloc/devm_kfree) during stream lifecycle cause performance overhead
- [Medium] S_FMT and S_FRAME_INTERVAL ioctls violate V4L2 API by returning -EINVAL for unsupported formats
--

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

media: i2c: add Maxim GMSL2/3 deserializer framework

These drivers are meant to be used as a common framework for Maxim GMSL2/3
deserializer. This framework enables support for the following new features
across all the 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
> index 0000000000000..8b6bed6e05f6a
> --- /dev/null
> +++ b/drivers/media/i2c/maxim-serdes/max_des.c
> @@ -0,0 +1,3346 @@
[ ... ]
> +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)
> +{
[ ... ]
> +	hw->source = max_des_get_link_source(priv, link);
> +	if (!hw->source->sd)
> +		return 0;
> +
> +	ret = v4l2_subdev_call(hw->source->sd, pad, get_frame_desc,
> +			       hw->source->pad, &fd);

[Severity: High]
Is there a race condition between reading the `hw->source->sd` pointer here
and the `max_des_notify_unbind` callback?

If `max_des_notify_unbind` asynchronously clears `source->sd = NULL` after
the NULL check above but before `v4l2_subdev_call` executes, this could
result in a use-after-free.

[ ... ]
> +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;
[ ... ]
> +		ret = des->ops->set_pipe_stream_id(des, hw.pipe, stream_id);

[Severity: High]
Is it possible that `des->ops->set_pipe_stream_id` is unassigned?

The `log_status` callback correctly checks `if (des->ops->set_pipe_stream_id)`
before printing it, but this execution path calls the callback
unconditionally. If a chip driver does not implement it, this will result
in a NULL pointer dereference.

[ ... ]
> +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)
> +{
> +	struct max_des *des = priv->des;
> +	struct max_des_remap *remaps;
> +	unsigned int num_remaps;
> +	int ret;
> +
> +	if (!des->ops->set_pipe_remap)
> +		return 0;
> +
> +	remaps = devm_kcalloc(priv->dev, des->info->num_remaps_per_pipe,
> +			      sizeof(*remaps), GFP_KERNEL);

[Severity: Medium]
Could this dynamic allocation with `devm_kcalloc()` during stream start/stop
cause performance overhead?

Because devres operations traverse a linked list when freeing memory
with `devm_kfree()`, allocating and freeing device-managed memory during
the stream lifecycle hotpath introduces overhead compared to standard
`kcalloc()`.

[ ... ]
> +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;
> +
> +	if (des->ops->select_links) {
> +		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 concurrent I2C accesses to another MUX channel hijack the active link
here?

The parent I2C bus lock is acquired and released internally by each
distinct transaction. Since it is released between `select_links()` and
`max_ser_wait_for_multiple()`, another thread might trigger the MUX select
callback and redirect the initialization commands to the wrong link.

[ ... ]
> +static int max_des_ser_attach_addr(struct max_des_priv *priv, u32 chan_id,
> +				   u16 addr, u16 alias)
> +{
> +	struct max_des *des = priv->des;
> +	struct max_des_link *link = &des->links[chan_id];
> +	unsigned int mask;
> +	int i, min, max;
> +	int ret = -ENOENT;
> +	int err;
> +
> +	max_des_ser_find_version_range(des, &min, &max);
> +
> +	if (link->ser_xlate.en) {
> +		dev_err(priv->dev, "Serializer for link %u already bound\n",
> +			link->index);
> +		return -EINVAL;
> +	}
> +
> +	for (i = max; i >= min; i--) {
> +		if (!(des->info->versions & BIT(i)))
> +			continue;
> +
> +		if (des->ops->set_link_version) {
> +			ret = des->ops->set_link_version(des, link, i);
> +			if (ret)
> +				goto out_select_links;
> +		}
> +
> +		ret = max_des_init_link_ser_xlate(priv, link, priv->client->adapter,
> +						  addr, alias);

[Severity: High]
Does this code verify the identity of the device before sending GMSL
reset sequences?

`max_des_i2c_mux_bus_notifier_call()` unconditionally passes the address of
any newly bound I2C client to this function. If another device, such as a
sensor, binds to the adapter before the actual serializer, it might receive
these proprietary reset commands instead.

[ ... ]
> +static int max_des_set_tpg_fmt(struct v4l2_subdev *sd,
> +			       struct v4l2_subdev_state *state,
> +			       struct v4l2_subdev_format *format)
> +{
> +	struct v4l2_mbus_framefmt *fmt = &format->format;
> +	struct max_des_priv *priv = v4l2_get_subdevdata(sd);
> +	struct max_des *des = priv->des;
> +	const struct max_serdes_tpg_entry *entry;
> +	struct v4l2_fract *in;
> +
> +	if (format->stream != MAX_SERDES_TPG_STREAM)
> +		return -EINVAL;
> +
> +	entry = max_des_find_tpg_entry(des, 0, fmt->width, fmt->height,
> +				       fmt->code, 0, 0);
> +	if (!entry)
> +		return -EINVAL;

[Severity: Medium]
Should this function adjust the requested format to the closest supported
match rather than returning `-EINVAL`?

The V4L2 specification typically requires drivers to modify the requested
format to a supported one and return success during `VIDIOC_SUBDEV_S_FMT`,
so userspace can negotiate formats correctly.

[ ... ]
> +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]
Are we dropping valid user requests to dynamically change the test pattern
while streaming?

When the user updates the test pattern, it modifies the software state
`des->tpg_pattern` but does not trigger a hardware update.

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

Because `des->tpg_entry` is evaluated solely on format dimensions and
intervals, this check will bypass `des->ops->set_tpg` if only the test
pattern index was changed.

[ ... ]
> +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;
> +	fd->num_entries = 0;
> +
> +	ret = max_des_populate_remap_context(priv, &context, state);
> +	if (ret)
> +		return ret;
> +
> +	for_each_active_route(&state->routing, route) {
> +		struct max_des_route_hw hw;
> +		unsigned int dst_vc_id;
> +
> +		if (pad != route->source_pad)
> +			continue;
> +
> +		ret = max_des_route_to_hw(priv, state, route, &hw);
> +		if (ret)
> +			return ret;
> +
> +		ret = max_des_get_src_dst_vc_id(&context, hw.pipe->index, hw.phy->index,
> +						hw.entry.bus.csi2.vc, &dst_vc_id);
> +		if (ret)
> +			return ret;
> +
> +		hw.entry.bus.csi2.vc = dst_vc_id;
> +		hw.entry.stream = route->source_stream;
> +
> +		fd->entry[fd->num_entries++] = hw.entry;

[Severity: High]
Does this code overflow `fd->entry` if `num_entries` exceeds the array size?

If user-space configures more routes mapping distinct sink streams to the
same source pad than the size of the `entry` array allows, it seems
`num_entries` can overflow without an explicit bounds check here.

[ ... ]
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +static int max_des_g_register(struct v4l2_subdev *sd,
> +			      struct v4l2_dbg_register *reg)
> +{
> +	struct max_des_priv *priv = v4l2_get_subdevdata(sd);
> +	struct max_des *des = priv->des;
> +	unsigned int val;
> +	int ret;
> +
> +	ret = des->ops->reg_read(des, reg->reg, &val);

[Severity: High]
Can `des->ops->reg_read` be unassigned here?

If a chip driver omits the implementation of this optional callback,
this unconditional dereference will cause a kernel panic when accessed
via debugfs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-gmsl2-3_serdes-v17-0-002499e534e8@analog.com?part=15