Re: [PATCH v17 14/22] media: i2c: add Maxim GMSL2/3 serializer framework

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Global active stream state `ser->active` is incorrectly cleared based on a single pad's activity, bypassing safety checks.
- [High] Lockless TOCTOU race on remote subdevice pointer `hw->source->sd` leads to a Use-After-Free.
- [Medium] V4L2 API violation: format and frame interval setters return `-EINVAL` instead of coercing unsupported requests.
- [Medium] Error handlers in `max_ser_update_phy` hardcode the global state instead of using the provided rollback state parameter.
- [Medium] Fragile rollback logic relies on dynamic memory allocation (`devm_kcalloc`) and ignores failures.
--

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

media: i2c: add Maxim GMSL2/3 serializer framework

This commit adds a common framework for Maxim GMSL2/3 serializers. It
enables support for the Streams API, I2C ATR, automatic GMSL link version
negotiation, and various other features across the chips.

> diff --git a/drivers/media/i2c/maxim-serdes/max_ser.c b/drivers/media/i2c/maxim-serdes/max_ser.c
> new file mode 100644
> index 0000000000000..673c1b34b8e77
> --- /dev/null
> +++ b/drivers/media/i2c/maxim-serdes/max_ser.c

[ ... ]

> +static int max_ser_route_to_hw(struct max_ser_priv *priv,
> +			       struct v4l2_subdev_state *state,
> +			       struct v4l2_subdev_route *route,
> +			       struct max_ser_route_hw *hw)
> +{
> +	struct max_ser *ser = priv->ser;
> +	struct v4l2_mbus_frame_desc fd = {};
> +	struct max_ser_phy *phy;
> +	unsigned int i;
> +	int ret;
> +
> +	memset(hw, 0, sizeof(*hw));
> +
> +	hw->is_tpg = max_ser_pad_is_tpg(ser, route->sink_pad);
> +	if (hw->is_tpg)
> +		return max_ser_tpg_route_to_hw(priv, state, route, hw);
> +
> +	phy = max_ser_pad_to_phy(ser, route->sink_pad);
> +	if (!phy)
> +		return -ENOENT;
> +
> +	hw->pipe = max_ser_find_phy_pipe(ser, phy);
> +	if (!hw->pipe)
> +		return -ENOENT;
> +
> +	hw->source = max_ser_get_phy_source(priv, phy);
> +	if (!hw->source->sd)
> +		return 0;
> +
> +	ret = v4l2_subdev_call(hw->source->sd, pad, get_frame_desc,
> +			       hw->source->pad, &fd);

[Severity: High]
Is this pointer safe to access here without taking a lock? If a userspace
thread concurrently calls format or routing ioctls, and the remote sensor
module is unloaded, max_ser_notify_unbind could set source->sd = NULL. Since
there is no synchronization (like a reference count or graph lock) held
across this block, does this allow v4l2_subdev_call to dereference a freed
pointer?

[ ... ]

> +static int max_ser_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_ser_priv *priv = v4l2_get_subdevdata(sd);
> +	struct max_ser *ser = priv->ser;
> +	const struct max_serdes_tpg_entry *entry;
> +	struct v4l2_fract *in;
> +
> +	if (format->stream != MAX_SERDES_TPG_STREAM)
> +		return -EINVAL;
> +
> +	entry = max_ser_find_tpg_entry(ser, 0, fmt->width, fmt->height,
> +				       fmt->code, 0, 0);
> +	if (!entry)
> +		return -EINVAL;

[Severity: Medium]
Does this violate the V4L2 core contract by rejecting the format entirely
instead of coercing it? Typically, the driver should automatically adjust
formats to the closest match rather than returning an error when an exact
match isn't found.

[ ... ]

> +static int max_ser_set_frame_interval(struct v4l2_subdev *sd,
> +				      struct v4l2_subdev_state *state,
> +				      struct v4l2_subdev_frame_interval *fi)
> +{
> +	struct max_ser_priv *priv = v4l2_get_subdevdata(sd);
> +	struct max_ser *ser = priv->ser;
> +	const struct max_serdes_tpg_entry *entry;
> +	struct v4l2_mbus_framefmt *fmt;
> +	struct v4l2_fract *in;
> +
> +	if (!max_ser_pad_is_tpg(ser, fi->pad) ||
> +	    fi->stream != MAX_SERDES_TPG_STREAM)
> +		return -ENOTTY;
> +
> +	if (fi->which == V4L2_SUBDEV_FORMAT_ACTIVE && ser->active)
> +		return -EBUSY;
> +
> +	fmt = v4l2_subdev_state_get_format(state, fi->pad, fi->stream);
> +	if (!fmt)
> +		return -EINVAL;
> +
> +	entry = max_ser_find_tpg_entry(ser, 0, fmt->width, fmt->height,
> +				       fmt->code, fi->interval.numerator,
> +				       fi->interval.denominator);
> +	if (!entry)
> +		return -EINVAL;

[Severity: Medium]
Similar to the format setter above, should this coerce the frame interval to
the closest supported value instead of outright rejecting the request with an
error?

[ ... ]

> +static int max_ser_update_pipe(struct max_ser_priv *priv,
> +			       struct max_ser_pipe *pipe,
> +			       struct v4l2_subdev_state *state,
> +			       u64 *streams_masks)
> +{
> +	struct max_ser *ser = priv->ser;
> +	struct max_ser_pipe_mode mode = { 0 };
> +	unsigned int num_dts;
> +	unsigned int *dts;
> +	unsigned int vcs;
> +	int ret;
> +
> +	if (!ser->ops->num_dts_per_pipe)
> +		return 0;
> +
> +	dts = devm_kcalloc(priv->dev, ser->ops->num_dts_per_pipe, sizeof(*dts),
> +			   GFP_KERNEL);
> +	if (!dts)
> +		return -ENOMEM;

[Severity: Medium]
Is it safe to rely on dynamic allocation here? This is called during stream
start/stop operations, and if the allocation fails during a rollback, the
error is ignored and the hardware could be left in a corrupted state. Also,
using device-managed memory dynamically on operations triggered by userspace
might continuously leak memory onto the devres list.

[ ... ]

> +static int max_ser_update_phy(struct max_ser_priv *priv,
> +			      struct v4l2_subdev_state *state,
> +			      struct max_ser_phy *phy, u64 *streams_masks,
> +			      u64 *old_streams_masks)
> +{
> +	struct max_ser *ser = priv->ser;
> +	u32 pad = max_ser_phy_to_pad(ser, phy);
> +	bool enable_changed = !streams_masks[pad] != !old_streams_masks[pad];
> +	bool enable = !!streams_masks[pad];
> +	struct max_ser_pipe *pipe;
> +	int ret;
> +
> +	pipe = max_ser_find_phy_pipe(ser, phy);
> +	if (!pipe)
> +		return -ENOENT;
> +
> +	if (!enable && enable_changed) {
> +		ret = max_ser_phy_set_active(ser, phy, enable);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = max_ser_update_pipe(priv, pipe, state, streams_masks);
> +	if (ret)
> +		goto err_revert_phy_disable;
> +
> +	ret = max_ser_update_pipe_enable(priv, pipe, state, streams_masks);
> +	if (ret)
> +		goto err_revert_pipe_update;
> +
> +	if (enable && enable_changed) {
> +		ret = max_ser_phy_set_active(ser, phy, enable);
> +		if (ret)
> +			goto err_revert_update_pipe_enable;
> +	}
> +
> +	return 0;
> +
> +err_revert_update_pipe_enable:
> +	max_ser_update_pipe_enable(priv, pipe, state, priv->streams_masks);
> +
> +err_revert_pipe_update:
> +	max_ser_update_pipe(priv, pipe, state, priv->streams_masks);

[Severity: Medium]
Should these rollbacks use the old_streams_masks parameter instead of
hardcoding the global priv->streams_masks? If a nested rollback fails partway
(for example, when max_ser_update_phys attempts to revert a partially
successful multi-pad update), utilizing the global state here might restore
to the wrong target state instead of the actual old state.

[ ... ]

> +static int max_ser_update_streams(struct v4l2_subdev *sd,
> +				  struct v4l2_subdev_state *state,
> +				  u32 pad, u64 updated_streams_mask, bool enable)
> +{
> +	struct max_ser_priv *priv = v4l2_get_subdevdata(sd);
> +	struct max_ser *ser = priv->ser;
> +	unsigned int num_pads = max_ser_num_pads(ser);
> +	u64 *streams_masks;
> +	int ret;

[ ... ]

> +	devm_kfree(priv->dev, priv->streams_masks);
> +	priv->streams_masks = streams_masks;
> +	ser->active = !!streams_masks[pad];
> +
> +	return 0;

[Severity: High]
Does this incorrectly clear the global active stream state if a single
pad is disabled? If userspace enables a stream on one pad (setting
ser->active to true) and then disables a stream on a different pad, this
unconditionally sets ser->active to false based only on the disabled pad,
even though the first pad is still streaming. This could bypass hardware
configuration safety checks elsewhere that rely on ser->active.

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