Re: [PATCH v5 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] pm: usage counter underflow due to incorrect pm_runtime_get_if_in_use check
- [High] media: use-after-free in mira016_remove via early control handler free
- [High] media: I2C register corruption due to overlapping 32-bit writes
- [Medium] build: missing select V4L2_FWNODE causes unmet dependencies
- [Medium] media: integer precision loss in mira016_calc_prate
- [Medium] media: I2C read errors swallowed during PLL lock polling
--

commit 3aea445f536c7012ef543a433b00f827003a9007
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/Kconfig b/drivers/media/i2c/Kconfig
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -321,6 +321,18 @@ config VIDEO_IMX678
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called imx678.
>  
> +config VIDEO_MIRA016
> +	tristate "ams OSRAM MIRA016 sensor support"
> +	depends on I2C && VIDEO_DEV
> +	depends on GPIOLIB
> +	select V4L2_CCI_I2C

[Severity: Medium]
Could this lead to kernel build failures due to unresolved symbols?

The driver uses APIs like v4l2_fwnode_endpoint_alloc_parse() in
mira016_parse_endpoint() which depend on the v4l2-fwnode module. Because
V4L2_FWNODE is not selected here, linking may fail when this module is not
enabled by other configs.

> +	help
> +	  This is a Video4Linux2 sensor driver for the ams OSRAM
> +	  MIRA016 camera.

[ ... ]

> diff --git a/drivers/media/i2c/mira016.c b/drivers/media/i2c/mira016.c
> --- /dev/null
> +++ b/drivers/media/i2c/mira016.c
> @@ -0,0 +1,2309 @@

[ ... ]

> +#define MIRA016_EXP_TIME_L_REG		CCI_REG32(0x000e)
> +#define MIRA016_EXP_TIME_S_REG		CCI_REG32(0x0012)
> +#define MIRA016_NUM_FRAMES_REG		CCI_REG32(0x0007)
> +#define MIRA016_TARGET_FRAME_TIME_REG	CCI_REG32(0x0008)

[Severity: High]
Does this sequence cause hardware register corruption?

MIRA016_NUM_FRAMES_REG (0x0007) and MIRA016_TARGET_FRAME_TIME_REG (0x0008)
are defined as 32-bit (CCI_REG32) and are written sequentially in
mira016_write_frame_duration_reg().

Because the addresses only have an offset of 1 between them, the first
write spans 0x0007-0x000a, and the second spans 0x0008-0x000b. This will
overwrite the first value and adjacent registers like CMD_REQ_1_REG.

> +
> +#define MIRA016_TIME_UNIT_REG		CCI_REG16(0x0012)
> +#define MIRA016_GLOB_TIME_REG		CCI_REG16(0x015a)

[ ... ]

> +static inline u32 mira016_calc_prate(struct mira016 *mira016, u32 h_tot)
> +{
> +	/*
> +	 * Pixel rate is calculate as the row duration divided by the total line
> +	 * length.
> +	 *
> +	 * pixel time (psec) = t_row(psec) / h_tot
> +	 * pixel rate (mbps) = 10^12 / pixel_time
> +	 *		     = 10^12 * h_tot / t_row
> +	 *		     = h_tot * 10^6 / t_row * 10^6 to avoid overflows
> +	 */
> +	u32 trow_psec = mira016_trow_psec(mira016);
> +
> +	return h_tot * HZ_PER_MHZ / trow_psec * HZ_PER_MHZ;

[Severity: Medium]
Will this evaluation lose significant precision?

The expression h_tot * HZ_PER_MHZ / trow_psec * HZ_PER_MHZ is evaluated
left-to-right. The integer division (h_tot * HZ_PER_MHZ) / trow_psec drops
all fractional sub-MHz precision before the final multiplication.

This can cause the pixel rate reported to userspace to be severely truncated
to 1 MHz boundaries, leading to framing inaccuracies.

> +}

[ ... ]

> +static int mira016_configure_timings(struct mira016 *mira016)
> +{
> +	int ret = 0;
> +	u64 val;
> +
> +	/* Configure the PLL clock tree. */
> +
> +	cci_write(mira016->regmap, MIRA016_BANK_SEL_REG, 0, &ret);
> +
> +	cci_write(mira016->regmap, MIRA016_PLL_DIV_N_REG, mira016->pll.n, &ret);
> +	cci_write(mira016->regmap, MIRA016_PLL_DIV_M_REG, mira016->pll.m, &ret);

[ ... ]

> +	cci_write(mira016->regmap, MIRA016_PLL_PD_REG, 0, &ret);
> +	if (ret)
> +		return ret;
> +
> +	ret = read_poll_timeout(cci_read, ret,
> +				(ret < 0 || (val & MIRA016_PLL_LOCKED)),
> +				500, 10000, false, mira016->regmap,
> +				MIRA016_PLL_LOCK_REG, &val, NULL);
> +	if (ret < 0)
> +		return ret;

[Severity: Medium]
Are we masking I2C read errors during PLL lock polling?

The read_poll_timeout macro evaluates its break condition (ret < 0 || ...).
If cci_read() fails, ret is negative, the condition evaluates to true, and
the macro exits returning 0. This return value overwrites ret = 0, masking
the I2C failure and potentially proceeding with uninitialized PLLs.

> +
> +	/* Configure the sensor timings. */

[ ... ]

> +static int mira016_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct mira016 *mira016 =
> +		container_of(ctrl->handler, struct mira016, ctrl_handler);
> +	struct i2c_client *client = v4l2_get_subdevdata(&mira016->sd);
> +	struct v4l2_subdev_state *state;
> +	struct v4l2_rect *crop;
> +	int ret = 0;
> +
> +	state = v4l2_subdev_get_locked_active_state(&mira016->sd);
> +	crop = v4l2_subdev_state_get_crop(state, 0);

[ ... ]

> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (!pm_runtime_get_if_in_use(&client->dev))
> +		return 0;

[Severity: High]
Could this result in a usage counter underflow and hardware access while
powered off?

When Runtime PM is disabled or not fully active, pm_runtime_get_if_in_use()
returns a negative error code like -EINVAL. The strict negation !(-EINVAL)
evaluates to false, bypassing the abort condition.

This allows the function to proceed with unpowered hardware access and
eventually call pm_runtime_put_autosuspend(), which underflows the usage
counter.

> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_EXPOSURE:

[ ... ]

> +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 this early free result in a use-after-free vulnerability?

In this teardown path, v4l2_ctrl_handler_free() is called before
v4l2_async_unregister_subdev(). The subdevice remains active and accessible
to userspace and the media core while its control handler is destroyed.

A concurrent userspace ioctl (like VIDIOC_S_CTRL) can access the freed
control handler, potentially leading to kernel memory corruption.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-mira016-v5-0-499a34ab8204@ideasonboard.com?part=2