Re: [PATCH v6 2/2] media: i2c: mira016: Add driver for Mira016

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] media: use-after-free in mira016_remove() due to wrong teardown order
- [Medium] pm: usage count underflow due to improperly checking pm_runtime_get_if_in_use()
- [Medium] regulator: unbalanced regulator_disable() in mira016_power_on() error path
- [Medium] media: i2c errors are silently masked in mira016_configure_pll()
- [Low] gpio: reset line improperly de-asserted in power-on error path
- [Low] media: severe integer truncation in mira016_calc_prate()
--

commit 6cf847015f19126b3da9f83cec8a64f50c6aa2bd
Author: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

media: i2c: mira016: Add driver for Mira016

Add driver for the ams OSRAM Mira016 sensor.

> diff --git a/drivers/media/i2c/mira016.c b/drivers/media/i2c/mira016.c
> index 0000000000000..7ab42dd865593
> --- /dev/null
> +++ b/drivers/media/i2c/mira016.c
[ ... ]
> +static inline u32 mira016_calc_prate(struct mira016 *mira016, u32 h_tot)
> +{
[ ... ]
> +	u32 trow_psec = mira016_trow_psec(mira016);
> +
> +	return h_tot * HZ_PER_MHZ / trow_psec * HZ_PER_MHZ;

[Severity: Low]
Does this calculation suffer from severe integer truncation? 

The intermediate division divides a value around 400,000,000 by 5,600,000,
resulting in a small integer. Multiplying that small integer by HZ_PER_MHZ
discards all fractional precision, which might lead to an inaccurate pixel
rate and A/V sync drift.

[ ... ]
> +static int mira016_configure_pll(struct mira016 *mira016)
> +{
> +	int ret = 0;
> +	u64 val;
[ ... ]
> +	cci_write(mira016->regmap, MIRA016_CLKGEN_TX_ESC_L_REG,
> +		  mira016->pll.esc_l, &ret);
> +	cci_write(mira016->regmap, MIRA016_PLL_PD_REG, 0, &ret);
> +	if (ret)
> +		return ret;
> +
> +	return read_poll_timeout(cci_read, ret,
> +				 (ret < 0 || (val & MIRA016_PLL_LOCKED)),
> +				 500, 10000, false, mira016->regmap,
> +				 MIRA016_PLL_LOCK_REG, &val, NULL);

[Severity: Medium]
Will this silently mask I2C communication errors?

The macro read_poll_timeout() evaluates to 0 when its exit condition is met.
If cci_read() fails, it sets ret to a negative error code. This makes the
condition (ret < 0) true, causing the macro to break the loop and return 0
(success) instead of propagating the actual error.

This could allow the driver to proceed under the false assumption that the
PLL locked successfully.

[ ... ]
> +static int mira016_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct mira016 *mira016 =
> +		container_of(ctrl->handler, struct mira016, ctrl_handler);
> +	struct v4l2_subdev_state *state;
> +	struct v4l2_rect *crop;
> +	int ret = 0;
[ ... ]
> +	if (!pm_runtime_get_if_in_use(mira016->dev))
> +		return 0;

[Severity: Medium]
Can this bypass the early return if pm_runtime_get_if_in_use() returns an
error code?

The function returns a negative value like -EINVAL if Runtime PM is disabled.
In that case, (!ret) evaluates to false, allowing the driver to proceed and
access unpowered hardware.

> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_EXPOSURE:
> +		ret = mira016_write_exposure_reg(mira016, ctrl->val);
> +		break;
> +	case V4L2_CID_VBLANK:
> +		ret = mira016_write_frame_duration_reg(mira016, state, ctrl->val);
> +		break;
> +	case V4L2_CID_ANALOGUE_GAIN:
> +		ret = mira016_write_analogue_gain(mira016, state, ctrl->val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	pm_runtime_put_autosuspend(mira016->dev);

[Severity: Medium]
If the earlier pm_runtime_get_if_in_use() returned a negative error code and
bypassed the return, won't this call to pm_runtime_put_autosuspend() cause
an unbalanced usage count decrement? The count was never incremented by the
failed get call.

[ ... ]
> +static int mira016_power_on(struct device *dev)
> +{
[ ... ]
> +	ret = cci_multi_reg_write(mira016->regmap, mira016_init_sequence,
> +				  ARRAY_SIZE(mira016_init_sequence), NULL);
> +	if (!ret)
> +		ret = mira016_configure_pll(mira016);
> +	if (ret)
> +		goto err_clk_disable;
> +
> +	return 0;
> +
> +err_clk_disable:
> +	clk_disable_unprepare(mira016->xclk);
> +err_reset_assert:
> +	gpiod_set_value_cansleep(mira016->reset_gpio, 0);

[Severity: Low]
Is this writing the inverse of the expected reset value?

The probe function requests this GPIO as GPIOD_OUT_HIGH, and
mira016_power_off() writes 1 to assert reset. Writing 0 here de-asserts the
reset line, waking the sensor up instead of leaving it in a safe reset state
during an error rollback.

> +err_reg_disable:
> +	for (int i = ARRAY_SIZE(mira016_supplies) - 1; i >= 0; i--)
> +		regulator_disable(mira016->supplies[i]);

[Severity: Medium]
Does this unconditionally disable all regulators regardless of how many were
successfully enabled?

If regulator_enable() fails, this cleanup loop uses a newly scoped variable
starting from ARRAY_SIZE(mira016_supplies) - 1 and iterates down to 0,
disabling regulators that were never successfully enabled. This can cause
reference count underflows in the regulator core or accidentally unpower
shared regulators.

[ ... ]
> +static void mira016_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct mira016 *mira016 = to_mira016(sd);
> +
> +	v4l2_ctrl_handler_free(mira016->sd.ctrl_handler);
> +
> +	v4l2_async_unregister_subdev(sd);

[Severity: High]
Could freeing the control handler before unregistering the subdevice lead to
a use-after-free?

This ordering leaves a race window where the subdevice remains accessible to
userspace while its control handler and associated mutex have already been
destroyed. If concurrent ioctls (such as VIDIOC_S_CTRL) occur during driver
unbind, the V4L2 core might attempt to lock the destroyed mutex and access
freed memory.

> +	v4l2_subdev_cleanup(&mira016->sd);
> +	media_entity_cleanup(&sd->entity);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-mira016-v6-0-177ecfd6d357@ideasonboard.com?part=2