Re: [PATCH v11 2/2] media: i2c: add Himax HM1246 image sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Incorrect handling of negative error codes from pm_runtime_get_if_active() leads to usage counter underflow and invalid hardware access.
- [High] Integer overflow in vco_out calculation and logical flaw completely break PLL configuration for high link frequencies.
- [High] Use-After-Free race condition during device removal due to incorrect cleanup ordering.
- [Medium] Active subdev format code is not updated when modifying HFLIP or VFLIP controls.
--

commit afbc7b4c289cadb0abe3caa02a219adf435617bf
Author: Matthias Fend <matthias.fend@emfend.at>
Subject: media: i2c: add Himax HM1246 image sensor driver

This commit adds a V4L2 sub-device driver for the Himax HM1246 image sensor,
supporting raw mode operations through an I2C interface.

> diff --git a/drivers/media/i2c/hm1246.c b/drivers/media/i2c/hm1246.c
> new file mode 100644
> index 0000000000000..f87ccb9b5a560
> --- /dev/null
> +++ b/drivers/media/i2c/hm1246.c
> @@ -0,0 +1,1285 @@

[ ... ]

> +static int hm1246_calc_pll(u32 xclk, u32 link_freq, u32 clocks_per_pixel,
> +			   u8 *pll1, u8 *pll2, u8 *pll3)
> +{

[ ... ]

> +	/* Determine an appropriate post divider. */
> +	for (post_div_index = 0; post_div_index < ARRAY_SIZE(post_div_table);
> +	     post_div_index++) {
> +		vco_out = pclk * pclk_div_table[pclk_div_index] *
> +			  post_div_table[post_div_index];
> +
> +		if (vco_out >= HM1246_PLL_VCO_MIN &&
> +		    vco_out <= HM1246_PLL_VCO_MAX)
> +			break;
> +	}

[Severity: High]
Does this calculation overflow the 32-bit vco_out variable for high link
frequencies?

For a maximum allowed link frequency of 96MHz, pclk would be 192MHz. The
product of pclk (192,000,000) * pclk_div (6) * post_div (8) is 9,216,000,000,
which overflows the 32-bit u32 variable. This overflowed garbage value could
falsely fall within the valid VCO bounds, causing the loop to succeed and
the configuration to ultimately fail later with -EINVAL.

[ ... ]

> +static int hm1246_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct hm1246 *hm1246 =
> +		container_of_const(ctrl->handler, struct hm1246, ctrls);
> +	struct v4l2_subdev_state *state;
> +	const struct v4l2_mbus_framefmt *format;
> +	u32 val;
> +	bool needs_cmu_update = true;
> +	int ret;
> +
> +	state = v4l2_subdev_get_locked_active_state(&hm1246->sd);
> +	format = v4l2_subdev_state_get_format(state, 0);
> +
> +	if (ctrl->id == V4L2_CID_VBLANK) {
> +		s64 exposure_max;
> +
> +		exposure_max =
> +			format->height + ctrl->val - HM1246_COARSE_INTG_MARGIN;
> +		ret = __v4l2_ctrl_modify_range(hm1246->exposure_ctrl,
> +					       hm1246->exposure_ctrl->minimum,
> +					       exposure_max,
> +					       hm1246->exposure_ctrl->step,
> +					       exposure_max);
> +
> +		if (ret) {
> +			dev_err(hm1246->dev, "exposure ctrl range update failed\n");
> +			return ret;
> +		}
> +	}
> +
> +	if (!pm_runtime_get_if_active(hm1246->dev))
> +		return 0;

[Severity: High]
Could this condition mishandle negative error codes returned from the
Runtime PM core?

The function pm_runtime_get_if_active() returns 1 if active, 0 if suspended,
and a negative error code (like -EINVAL) if disabled. If it returns a
negative error code, this check is bypassed. This might allow the driver
to attempt hardware register accesses on unpowered hardware. In addition,
since the usage count wasn't incremented on error, the subsequent call to
pm_runtime_put() at the end of the function could cause a usage counter
underflow. Checking for (ret <= 0) might be safer here.

> +
> +	ret = 0;
> +	switch (ctrl->id) {
> +	case V4L2_CID_EXPOSURE:
> +		cci_write(hm1246->regmap, HM1246_COARSE_INTG_REG, ctrl->val,
> +			  &ret);
> +		break;
> +
> +	case V4L2_CID_ANALOGUE_GAIN:
> +		cci_write(hm1246->regmap, HM1246_ANALOG_GLOBAL_GAIN_REG,
> +			  ctrl->val, &ret);
> +		break;
> +
> +	case V4L2_CID_VBLANK:
> +		val = format->height + ctrl->val;
> +		cci_write(hm1246->regmap, HM1246_FRAME_LENGTH_LINES_REG, val,
> +			  &ret);
> +		break;
> +
> +	case V4L2_CID_HFLIP:
> +	case V4L2_CID_VFLIP:
> +		val = 0;
> +		if (hm1246->hflip_ctrl->val)
> +			val |= HM1246_IMAGE_ORIENTATION_HFLIP;
> +		if (hm1246->vflip_ctrl->val)
> +			val |= HM1246_IMAGE_ORIENTATION_VFLIP;
> +
> +		cci_write(hm1246->regmap, HM1246_IMAGE_ORIENTATION_REG, val,
> +			  &ret);
> +		break;

[Severity: Medium]
Should the active subdev format code be updated here to reflect the new
pixel layout?

Since V4L2_CID_HFLIP and V4L2_CID_VFLIP change the sensor's Bayer color
pattern and are registered with V4L2_CTRL_FLAG_MODIFY_LAYOUT, leaving the
active state's pixel format un-updated could cause subsequent
VIDIOC_SUBDEV_G_FMT ioctls to return stale format information.

[ ... ]

> +static void hm1246_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct hm1246 *hm1246 = to_hm1246(sd);
> +
> +	v4l2_async_unregister_subdev(&hm1246->sd);
> +	v4l2_subdev_cleanup(sd);
> +	media_entity_cleanup(&hm1246->sd.entity);
> +	v4l2_ctrl_handler_free(&hm1246->ctrls);

[Severity: High]
Could the order of these cleanup calls lead to a use-after-free?

Calling v4l2_subdev_cleanup() frees the subdevice's active state memory
before v4l2_ctrl_handler_free() destroys the controls. If a concurrent
VIDIOC_S_CTRL ioctl is executing hm1246_set_ctrl(), it could safely
dereference the active state via v4l2_subdev_get_locked_active_state()
since the control handler is still active. If the state is freed underneath
it during removal, this would cause a use-after-free.

> +
> +	pm_runtime_disable(&client->dev);
> +	if (!pm_runtime_status_suspended(&client->dev)) {
> +		hm1246_power_off(hm1246->dev);
> +		pm_runtime_set_suspended(&client->dev);
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622-hm1246-v11-0-c62ef9516ab1@emfend.at?part=2