Re: [PATCH v3 2/2] media: i2c: imx678: Add driver for Sony IMX678

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Incorrect check of `pm_runtime_get_if_in_use()` in `imx678_set_ctrl()` causes usage count underflow and unsafe hardware access.
- [High] Crop selection boundaries destroy hardware alignment and can exceed the active pixel area.
- [High] Freeing the V4L2 active state before destroying controls in `remove()` and probe error path creates a Use-After-Free vulnerability.
- [Medium] Incorrect upper bound calculation for crop rectangle `rect.top` restricts the cropping range.
- [Medium] I2C read errors during module identification are masked, breaking probe deferral.
- [Medium] Unbalanced power off in the probe error path can cause a double disable of clocks and regulators.
- [Low] The `dev_pm_ops` structure is assigned without the `pm_ptr()` wrapper, causing dead code when CONFIG_PM is disabled.
--

commit bd147d0e8951d7c1d3269dbbc351c621e16146eb
Author: Jai Luthra <jai.luthra@ideasonboard.com>

media: i2c: imx678: Add driver for Sony IMX678

Add a V4L2 subdev driver for the Sony IMX678 image sensor supporting
monochrome and color variants, configurable resolutions, crop, exposure,
and gain control.

> diff --git a/drivers/media/i2c/imx678.c b/drivers/media/i2c/imx678.c
> new file mode 100644
> index 0000000000000..5ac710b7e38b6
> --- /dev/null
> +++ b/drivers/media/i2c/imx678.c

[ ... ]

> +static int imx678_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct imx678 *imx678 = container_of(ctrl->handler, struct imx678, ctrl_handler);
> +	struct v4l2_subdev_state *state;
> +	struct i2c_client *client = v4l2_get_subdevdata(&imx678->sd);
> +	const struct v4l2_mbus_framefmt *format;
> +	int ret = 0;
> +
> +	state = v4l2_subdev_get_locked_active_state(&imx678->sd);
> +	format = imx678_state_format(state);

[ ... ]

> +	/*
> +	 * Applying V4L2 control value only happens
> +	 * when power is up for streaming
> +	 */
> +	if (pm_runtime_get_if_in_use(&client->dev) == 0)
> +		return 0;

[Severity: High]
Does this correctly handle the case where pm_runtime_get_if_in_use() returns
a negative error code?

If it returns a negative value (such as -EINVAL when runtime PM is disabled),
this condition evaluates to false. The driver will then proceed to access the
hardware and call pm_runtime_put(). Since the usage count was not incremented
by the failed get, will the put operation cause a reference count underflow
warning and perform an unsafe hardware access?

[ ... ]

> +static int imx678_set_selection(struct v4l2_subdev *sd,
> +				struct v4l2_subdev_state *sd_state,
> +				struct v4l2_subdev_selection *sel)
> +{

[ ... ]

> +	/* Align left, top to 4 */
> +	rect.left = clamp_t(s32, ALIGN(sel->r.left, IMX678_CROP_HST_ALIGN),
> +			    imx678_active_area.left,
> +			    imx678_active_area.width - IMX678_PIXEL_ARRAY_MIN_WIDTH);
> +	rect.top = clamp_t(s32, ALIGN(sel->r.top, IMX678_CROP_VST_ALIGN),
> +			   imx678_active_area.top,
> +			   imx678_active_area.height - IMX678_PIXEL_ARRAY_MIN_HEIGHT);

[Severity: Medium]
Is this upper bound calculation for rect.top correct given the active area
offset?

Because the active area starts at an offset of imx678_active_area.top (20),
imx678_active_area.height - IMX678_PIXEL_ARRAY_MIN_HEIGHT evaluates to
2180 - 956 = 1224. Shouldn't the valid maximum actually be 20 + 2180 - 956,
which is 1244?

It appears the current formula restricts the crop rectangle from reaching
the bottom 20 lines of the sensor's active array.

> +	/* Align width to 16 and height to 4 */
> +	rect.width = clamp_t(u32, ALIGN(sel->r.width, IMX678_CROP_HWIDTH_ALIGN),
> +			     IMX678_PIXEL_ARRAY_MIN_WIDTH, imx678_active_area.width);
> +	rect.height = clamp_t(u32, ALIGN(sel->r.height, IMX678_CROP_VWIDTH_ALIGN),
> +			      IMX678_PIXEL_ARRAY_MIN_HEIGHT, imx678_active_area.height);
> +
> +	rect.width = min_t(u32, rect.width, imx678_native_area.width - rect.left);

[Severity: High]
Can this min_t() calculation break the hardware alignment and cause the
crop rectangle to exceed the active pixel area?

For example, if rect.left is 4 and rect.width is 3856,
min_t(3856, 3857 - 4) evaluates to 3853. This truncates the width to an
unaligned value since 3853 is not a multiple of 16.

Also, since the active area width is 3856, will an end position of
4 + 3853 = 3857 exceed the sensor's active width and cause it to read
optical black pixels?

[ ... ]

> +static int imx678_identify_model(struct imx678 *imx678)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&imx678->sd);
> +	const struct imx678_model_info *info;
> +	enum imx678_type detected;
> +	int ret = 0;
> +	u64 val = 0;
> +
> +	info = of_device_get_match_data(&client->dev);
> +
> +	/*
> +	 * This sensor's ID registers become accessible 80ms after coming out
> +	 * of STANDBY mode.
> +	 */
> +	cci_write(imx678->cci, IMX678_REG_MODE_SELECT, 0, &ret);
> +	fsleep(IMX678_MODULE_ID_DELAY);
> +
> +	cci_read(imx678->cci, IMX678_REG_MODULE_ID, &val, &ret);
> +
> +	if (val != IMX678_ID) {

[Severity: Medium]
Should the return value (ret) be checked before evaluating the read value?

If the I2C read fails and returns an error like -EPROBE_DEFER or
-EREMOTEIO, val remains 0. The code will then evaluate 0 != IMX678_ID,
assume the wrong chip is present, and return -ENXIO. This might mask
critical errors and break probe deferral.

> +		dev_err(&client->dev,
> +			"Chip ID mismatch: %x!=%llx\n", IMX678_ID, val);
> +		return -ENXIO;
> +	}

[ ... ]

> +static int imx678_probe(struct i2c_client *client)
> +{

[ ... ]

> +error_subdev_cleanup:
> +	v4l2_subdev_cleanup(&imx678->sd);
> +
> +error_media_entity:
> +	media_entity_cleanup(&imx678->sd.entity);
> +
> +error_handler_free:
> +	imx678_free_controls(imx678);

[Severity: High]
Can calling v4l2_subdev_cleanup() before imx678_free_controls() cause a
use-after-free if a concurrent ioctl is running?

v4l2_subdev_cleanup() frees the memory holding the subdevice's active
state. If a concurrent VIDIOC_S_CTRL ioctl is executing, imx678_set_ctrl()
could dereference the freed active_state to access format parameters.

Calling imx678_free_controls() prior to v4l2_subdev_cleanup() acts as a
synchronization barrier because it acquires the control handler lock,
ensuring any executing set_ctrl operations complete before the state is
freed.

> +
> +error_pm_runtime:
> +	pm_runtime_disable(&client->dev);
> +	pm_runtime_set_suspended(&client->dev);
> +
> +error_power_off:
> +	imx678_power_off(&client->dev);

[Severity: Medium]
Can this error path result in an unbalanced power off?

If the device had already been suspended in the background by the PM core
(since there is no get_sync preventing it), pm_runtime_set_suspended()
forces the status to suspended, but then imx678_power_off() is called
unconditionally.

Does this need to check !pm_runtime_status_suspended(&client->dev) before
powering off, similar to how it is handled in the remove() function, to
prevent a double disable of clocks and regulators?

> +
> +	return ret;
> +}
> +
> +static void imx678_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct imx678 *imx678 = to_imx678(sd);
> +
> +	v4l2_async_unregister_subdev(sd);
> +	v4l2_subdev_cleanup(sd);
> +	media_entity_cleanup(&sd->entity);
> +	imx678_free_controls(imx678);

[Severity: High]
Similarly to the probe error path, does calling v4l2_subdev_cleanup() before
imx678_free_controls() create a use-after-free window here during device
removal?

[ ... ]

> +static struct i2c_driver imx678_i2c_driver = {
> +	.driver = {
> +		.name = "imx678",
> +		.of_match_table = imx678_of_match,
> +		.pm = &imx678_pm_ops,

[Severity: Low]
Should the pm operations pointer be wrapped with pm_ptr()?

Without the wrapper, the imx678_pm_ops structure and its referenced
callbacks (like imx678_power_on and imx678_power_off) will be statically
linked into the kernel even when CONFIG_PM is disabled, producing dead
code.

> +	},
> +	.probe = imx678_probe,
> +	.remove = imx678_remove,
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520-imx678-v3-0-8b5f9676486e@ideasonboard.com?part=2