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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] max_serdes_get_streams_masks inadvertently clears unaffected pad masks and the source pad mask due to incorrect array initialization.
- [High] max_serdes_get_streams_masks uses devm_kcalloc for dynamic stream mask allocation in a runtime operation, leading to a memory leak.
- [High] Tautological pointer check in max_serdes_xlate_enable_disable_streams leads to a potential NULL pointer dereference.
--

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

This commit adds a common framework for Maxim GMSL2/3 serializers and
deserializers, enabling support for Streams API, GMSL link negotiation,
and other shared functionality across the chips.

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

[ ... ]

> +int max_serdes_xlate_enable_disable_streams(struct max_serdes_source *sources,
> +					    u32 source_sink_pad_offset,
> +					    const struct v4l2_subdev_state *state,
> +					    u32 pad, u64 updated_streams_mask,
> +					    u32 sink_pad_start, u32 num_sink_pads,
> +					    bool enable)
> +{
> +	u32 failed_sink_pad;
> +	int ret;
> +	u32 i;
> +
> +	for (i = sink_pad_start; i < sink_pad_start + num_sink_pads; i++) {
> +		u64 matched_streams_mask = updated_streams_mask;
> +		u64 updated_sink_streams_mask;
> +		struct max_serdes_source *source;
> +
> +		updated_sink_streams_mask =
> +			v4l2_subdev_state_xlate_streams(state, pad, i,
> +							&matched_streams_mask);
> +		if (!updated_sink_streams_mask)
> +			continue;
> +
> +		source = &sources[i + source_sink_pad_offset];
> +		if (!source)
> +			continue;
> +
> +		if (enable)
> +			ret = v4l2_subdev_enable_streams(source->sd, source->pad,
> +							 updated_sink_streams_mask);

[Severity: High]
Will this check actually prevent a NULL pointer dereference?

Since source is assigned the address of an element in the sources
array, the pointer itself will never be NULL. If the corresponding
sink pad is not linked to a remote subdevice, the unpopulated element
might have source->sd set to NULL.

If userspace configures an active route to an unconnected sink pad
and starts the stream, will v4l2_subdev_enable_streams() dereference
the NULL source->sd pointer and panic the kernel?

[ ... ]

> +int max_serdes_get_streams_masks(struct device *dev,
> +				 const struct v4l2_subdev_state *state,
> +				 u32 pad, u64 updated_streams_mask,
> +				 u32 num_pads, u64 *old_streams_masks,
> +				 u64 **new_streams_masks, bool enable)
> +{
> +	u64 *streams_masks;
> +	unsigned int i;
> +
> +	streams_masks = devm_kcalloc(dev, num_pads, sizeof(*streams_masks), GFP_KERNEL);

[Severity: High]
Could this lead to a memory leak during runtime operations?

This function appears to be called during stream enable and disable
operations, which userspace can trigger repeatedly via V4L2 ioctls.

Because devm_kcalloc allocations are bound to the device's lifetime,
will continuous stream toggling exhaust memory and lead to an OOM
condition since the allocations are only freed on driver detach?

> +	if (!streams_masks)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_pads; i++) {
> +		u64 matched_streams_mask = updated_streams_mask;
> +		u64 updated_sink_streams_mask;
> +
> +		updated_sink_streams_mask =
> +			v4l2_subdev_state_xlate_streams(state, pad, i,
> +							&matched_streams_mask);
> +		if (!updated_sink_streams_mask)
> +			continue;
> +
> +		streams_masks[i] = old_streams_masks[i];
> +		if (enable)
> +			streams_masks[i] |= updated_sink_streams_mask;
> +		else
> +			streams_masks[i] &= ~updated_sink_streams_mask;
> +	}
> +
> +	if (enable)
> +		streams_masks[pad] |= updated_streams_mask;
> +	else
> +		streams_masks[pad] &= ~updated_streams_mask;

[Severity: High]
Does this logic accidentally clear the state of unaffected pads and the
source pad?

The streams_masks array is zero-initialized by devm_kcalloc. Inside the
loop, if updated_sink_streams_mask is 0, the continue statement skips
the assignment of streams_masks[i] = old_streams_masks[i]. This leaves
unaffected pads with a cleared state of 0.

Additionally, when updating the target pad itself after the loop, the
operation directly applies updated_streams_mask without inheriting its
prior state from old_streams_masks[pad].

Will this cause the tracking to forget previously enabled streams and
break stream state management?

[ ... ]

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