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

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [Critical] Stack buffer overflow in `max_des_get_frame_desc_state` via unbounded route entries
- [High] Race condition in I2C MUX selection and serializer initialization
- [High] Blind assumption of serializer identity leads to hardware corruption of other I2C devices
- [High] Synchronous driver probing bypasses serializer I2C bus notifier initialization
- [High] NULL pointer dereference in `max_des_phy_hw_data_lanes` when PHY configurations are absent
- [High] TOCTOU race in `max_des_route_to_hw` leading to stack out-of-bounds access
- [High] Hardware state machine violation due to incorrect stream disable ordering
- [High] NULL pointer dereference when invoking optional `des->ops` functions
- [Medium] Undefined behavior from `BIT_ULL(route->sink_stream)` with unbounded stream IDs
- [Medium] API violation in `max_des_set_tpg_fmt` and `max_des_set_frame_interval` returning `-EINVAL`
--

--- Patch [15]: [PATCH v15 15/22] media: i2c: add Maxim GMSL2/3 deserializer framework ---
Hi,

Thank you for the patch. I have a few questions and observations regarding
some potential issues I noticed during review.

commit 4eafc7084d0daa6783fda7c82ba0abf24d42f7dd
Author: Cosmin Tanislav <[email protected]>
Subject: media: i2c: add Maxim GMSL2/3 deserializer framework

This patch adds a common framework for Maxim GMSL2/3 deserializer drivers.
It introduces features like the Streams API support, I2C ATR, and automatic
link negotiation.

> diff --git a/drivers/media/i2c/maxim-serdes/max_des.c b/drivers/media/i2c/maxim-serdes/max_des.c
> index 0000000000000..93253068a6ada
> --- /dev/null
> +++ b/drivers/media/i2c/maxim-serdes/max_des.c
[ ... ]
> @@ -288,29 +288,29 @@ 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)
> {
[ ... ]
> 	ret = v4l2_subdev_call(hw->source->sd, pad, get_frame_desc,
> 			       hw->source->pad, &fd);
[ ... ]
> @@ -400,10 +400,10 @@ static int max_des_get_src_dst_vc_id(struct max_des_remap_context *context,
> 				     unsigned int pipe_id, unsigned int phy_id,
> 				     unsigned int src_vc_id, unsigned int *dst_vc_id)
> {
> 	if (!(context->vc_ids_masks[pipe_id][phy_id] & BIT(src_vc_id)))
> 		return -ENOENT;
> 
> 	*dst_vc_id = context->vc_ids_map[pipe_id][phy_id][src_vc_id];

[Severity: High]
Are we at risk of a time-of-check to time-of-use race here?

V4L2 locks are per-subdevice, meaning the remote subdevice's frame
descriptor can be reconfigured concurrently. If max_des_route_to_hw() calls
get_frame_desc multiple times across stream updates, a malicious or updated
src_vc_id could bypass bounds checking and lead to an out-of-bounds access
on the stack variable context->vc_ids_map.

[ ... ]
> @@ -641,20 +641,20 @@ static int max_des_get_pipe_vc_remaps(struct max_des_priv *priv,
[ ... ]
> 	for_each_active_route(&state->routing, route) {
> 		unsigned int src_vc_id, dst_vc_id;
> 		struct max_des_route_hw hw;
> 
> 		if (!(BIT_ULL(route->sink_stream) & streams_masks[route->sink_pad]))
> 			continue;

[Severity: Medium]
Is it possible for route->sink_stream to exceed the bounds of unsigned long
long?

If unprivileged userspace provides a routing table with a sink_stream value
of 64 or greater, this shift will cause undefined behavior.

[ ... ]
> @@ -1054,34 +1054,34 @@ 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));

[Severity: High]
Does this code guarantee that the select_links operation is implemented?

If a deserializer variant lacks this optional callback, calling it
unconditionally will cause a null pointer dereference and kernel panic.

> 	if (ret)
> 		return ret;
> 
> 	ret = max_ser_wait_for_multiple(adapter, addrs, ARRAY_SIZE(addrs),
> 					&current_addr);

[Severity: High]
Could concurrent traffic on other downstream MUX channels alter the hardware
MUX selection here?

Because the parent I2C bus lock isn't held between the select_links() call
and the subsequent I2C commands on the adapter, other device probing activity
could misroute these initialization commands to the wrong GMSL link.

[ ... ]
> @@ -1320,17 +1320,17 @@ static int max_des_i2c_mux_bus_notifier_call(struct notifier_block *nb,
[ ... ]
> 	for (i = 0; i < des->info->num_links; i++) {
> 		if (des->links[i].enabled &&
> 		    client->adapter == des->links[i].adapter)
> 			break;
> 	}
> 
> 	if (i == des->info->num_links)
> 		return NOTIFY_DONE;

[Severity: High]
Might this synchronous adapter registration fail to match the notifier?

When i2c_mux_add_adapter() probes child devices synchronously in
max_des_i2c_mux_init(), this notifier is triggered before link->adapter is
populated. Since client->adapter won't match the uninitialized link->adapter,
the initialization gets skipped.

> 
> 	max_des_ser_attach_addr(priv, i, client->addr, client->addr);

[Severity: High]
Can this logic incorrectly match another device probing on the bus?

It assumes any BUS_NOTIFY_BIND_DRIVER event on the downstream adapter
belongs to the serializer. If a camera sensor or PMIC happens to probe first,
max_des_ser_attach_addr() will execute GMSL initialization sequences on it,
potentially causing hardware corruption.

[ ... ]
> @@ -1357,21 +1357,21 @@ static int max_des_i2c_mux_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;
> 
> 		ret = i2c_mux_add_adapter(priv->mux, 0, i);
> 		if (ret)
> 			goto err_add_adapters;
> 
> 		link->adapter = priv->mux->adapter[priv->mux->num_adapters - 1];

This isn't a separate bug, but it is related to the previous comment. This is
where link->adapter is assigned too late to be matched by synchronous probes
triggered inside i2c_mux_add_adapter().

[ ... ]
> @@ -1379,15 +1379,15 @@ static int max_des_set_tpg_fmt(struct v4l2_subdev *sd,
[ ... ]
> 	entry = max_des_find_tpg_entry(des, 0, fmt->width, fmt->height,
> 				       fmt->code, 0, 0);
> 	if (!entry)
> 		return -EINVAL;

[Severity: Medium]
Should this format negotiation return an error instead of adjusting the
parameters?

The V4L2 Subdevice API specification requires set_fmt and set_frame_interval
to adjust requested but unsupported parameters to the closest hardware match,
rather than returning an error.

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

This isn't a separate bug, but the same API mismatch applies here for
unsupported frame intervals.

[ ... ]
> @@ -1661,16 +1661,16 @@ static int max_des_get_frame_desc_state(struct v4l2_subdev *sd,
[ ... ]
> 		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 risk of a stack buffer overflow here if userspace configures
many routes?

The v4l2_mbus_frame_desc structure contains a fixed size array bounded by
V4L2_FRAME_DESC_ENTRY_MAX. If unprivileged userspace multiplexes more active
routes to the same pad than the array can hold, this loop will write out
of bounds on the stack.

[ ... ]
> @@ -1942,9 +1942,9 @@ int max_des_phy_hw_data_lanes(struct max_des *des, struct max_des_phy *phy)
> {
> 	const struct max_serdes_phys_configs *configs = &des->info->phys_configs;
> 	const struct max_serdes_phys_config *config =
> 		&configs->configs[des->phys_config];
> 
> 	return config->lanes[phy->index];

[Severity: High]
Could configs->configs be null when evaluated here?

If a deserializer has no explicit PHY configurations
(des->info->phys_configs.num_configs == 0), max_des_find_phys_config()
returns early. Accessing the configs array will compute a null-based pointer,
causing a crash when dereferencing config->lanes.

[ ... ]
> @@ -2285,15 +2285,15 @@ static int max_des_update_streams(struct v4l2_subdev *sd,
[ ... ]
> 	ret = max_des_set_vc_remaps(priv, &context, state, streams_masks);
> 	if (ret)
> 		goto err_free_streams_masks;
> 
> 	ret = max_des_set_pipes_stream_id(priv);
> 	if (ret)
> 		goto err_free_streams_masks;
> 
> 	if (!enable) {
> 		ret = max_des_enable_disable_streams(priv, state, pad,
> 						     updated_streams_mask, enable);

[Severity: High]
Will stopping the stream in this order cause hardware protocol errors?

The driver removes the VC remap from the serializer's hardware configuration
via max_des_set_vc_remaps() before invoking max_des_enable_disable_streams()
to actually stop the downstream sensor. Altering routing while the sensor is
still actively transmitting data could drop or misroute frames.

[ ... ]
> @@ -2574,9 +2574,9 @@ 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]
Are these debugfs callbacks guaranteed to be implemented by all chip variants?

If des->ops->reg_read or reg_write are not implemented, these unconditional
invocations will trigger a null pointer dereference.

> 	if (ret)
> 		return ret;
[ ... ]
> @@ -2589,7 +2589,7 @@ static int max_des_s_register(struct v4l2_subdev *sd,
> 			      const struct v4l2_dbg_register *reg)
> {
> 	struct max_des_priv *priv = v4l2_get_subdevdata(sd);
> 	struct max_des *des = priv->des;
> 
> 	return des->ops->reg_write(des, reg->reg, reg->val);
> }

This isn't a separate bug, but the same missing check applies for reg_write
here.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15