Re: [PATCH v3 2/2] media: i2c: Add Samsung S5K3T2 image sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] media: i2c: s5k3t2: PM usage counter underflow due to incorrect pm_runtime_get_if_active() check
- [High] media: i2c: s5k3t2: Array filter ignores index leading to NULL pointer dereference
--

commit 3fc6dedbde02c5967afbd9449d7a6ccc791af98e
Author: Armandas Kvietkus <armandas.kvietkus@proton.me>
Subject: media: i2c: Add Samsung S5K3T2 image sensor driver

This commit adds a driver for the Samsung S5K3T2 20 Mpixel CMOS image sensor.
It supports three modes and manages power using runtime PM with autosuspend.

> diff --git a/drivers/media/i2c/s5k3t2.c b/drivers/media/i2c/s5k3t2.c
> new file mode 100644
> index 0000000000000..5958312177753
> --- /dev/null
> +++ b/drivers/media/i2c/s5k3t2.c
[ ... ]
> @@ -0,0 +1,1360 @@
[ ... ]
> +static int s5k3t2_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
[ ... ]
> +	if (!pm_runtime_get_if_active(s5k3t2->dev))
> +		return 0;

[Severity: High]
Does this code handle negative error codes correctly?

If pm_runtime_get_if_active() returns a negative error code (such as
-EINVAL when runtime PM is disabled, which can happen during driver unbind
or system suspend phases), !(-EINVAL) evaluates to 0.

This bypasses the early return and allows execution to proceed to access
the hardware when the device is not active.

> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_ANALOGUE_GAIN:
[ ... ]
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	pm_runtime_put(s5k3t2->dev);

[Severity: High]
Can this cause a PM usage counter underflow?

If pm_runtime_get_if_active() returned a negative error code earlier,
the usage counter was not incremented. Calling pm_runtime_put() here
would unconditionally decrement the counter, leading to an underflow.

> +
> +	return ret;
> +}
[ ... ]
> +static int s5k3t2_init_controls(struct s5k3t2 *s5k3t2)
> +{
> +	struct v4l2_ctrl_handler *ctrl_hdlr = &s5k3t2->ctrl_handler;
> +	const struct s5k3t2_mode *mode = s5k3t2->mode;
> +	s64 hblank, vblank, exposure_max;
> +	struct v4l2_fwnode_device_properties props;
> +	int ret;
> +
> +	v4l2_ctrl_handler_init(ctrl_hdlr, 11);
> +
> +	s5k3t2->link_freq =
> +		v4l2_ctrl_new_int_menu(ctrl_hdlr, &s5k3t2_ctrl_ops,
> +				       V4L2_CID_LINK_FREQ,
> +				       ARRAY_SIZE(s5k3t2_link_freq_menu) - 1,
> +				       mode->link_freq_index,
> +				       s5k3t2_link_freq_menu);
> +	if (s5k3t2->link_freq)
> +		s5k3t2->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +	s5k3t2->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &s5k3t2_ctrl_ops,
> +					       V4L2_CID_PIXEL_RATE,
> +					       S5K3T2_PIXEL_RATE,
> +					       S5K3T2_PIXEL_RATE, 1,
> +					       S5K3T2_PIXEL_RATE);
> +
> +	hblank = mode->hts - mode->width;

[Severity: High]
Can mode be NULL here?

If v4l2_find_nearest_size_conditional() returns NULL during probe,
mode is NULL and this assignment will trigger a NULL pointer dereference.
This occurs because s5k3t2_filter_by_link_freq() always evaluates the
first element of the modes array, leading to a failure to match any mode
if the board doesn't use the link frequency of the first element.

> +	s5k3t2->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &s5k3t2_ctrl_ops,
> +					   V4L2_CID_HBLANK, hblank,
> +					   hblank, 1, hblank);
[ ... ]
> +static bool s5k3t2_filter_by_link_freq(const void *array, size_t index,
> +				       const void *context)
> +{
> +	const struct s5k3t2_mode *mode = array;
> +	const struct s5k3t2 *s5k3t2 = context;
> +
> +	return s5k3t2->link_freq_bitmap & BIT(mode->link_freq_index);

[Severity: High]
Does this filter ignore the index parameter?

By assigning array to mode directly, it only ever checks the first element
of the array instead of indexing into it.

If the first element's link frequency is missing from the device tree
configuration, this function will always return false for every index,
causing v4l2_find_nearest_size_conditional() to return NULL.

> +}
[ ... ]
> +static int s5k3t2_probe(struct i2c_client *client)
> +{
[ ... ]
> +	s5k3t2->mode =
> +		v4l2_find_nearest_size_conditional(s5k3t2_supported_modes,
> +						   ARRAY_SIZE(s5k3t2_supported_modes),
> +						   width, height,
> +						   S5K3T2_DEFAULT_WIDTH,
> +						   S5K3T2_DEFAULT_HEIGHT,
> +						   s5k3t2_filter_by_link_freq,
> +						   s5k3t2);
> +	ret = s5k3t2_init_controls(s5k3t2);

[Severity: High]
If v4l2_find_nearest_size_conditional() returns NULL, s5k3t2->mode will be
set to NULL, and s5k3t2_init_controls() will dereference it.

Is it possible to add a check for NULL here?

> +	if (ret) {
> +		dev_err_probe(s5k3t2->dev, ret, "failed to init controls\n");
> +		goto power_off;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-upstream-s5k3t2-v3-0-a5c58dfcec29@proton.me?part=2