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

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] mira016_init_state() mutates global driver state when called for TRY states, corrupting active frame timings and creating a data race.
- [High] Out-of-bounds array access in analogue gain lookup table due to off-by-one maximum limit.
- [High] Control handler may access freed active state during driver removal due to incorrect cleanup ordering.
- [Medium] Error codes from I2C writes are silently overwritten and ignored in multiple configuration functions.
- [Medium] Active V4L2 state is modified before rejecting the operation if the device is currently streaming.
- [Medium] Severe precision loss in pixel rate calculation due to early integer division.
- [Medium] Hardware minimum vertical blanking is violated due to integer division rounding down.
--

commit 3cceafc720bbde80fa31e38aaa08374b0f450a04
Author: Jacopo Mondi [off-list ref]

media: i2c: mira016: Add driver for Mira016

Add driver for the ams OSRAM Mira016 sensor.

[ ... ]

+static inline u32 mira016_nsec_to_lines(struct mira016 *mira016, u32 nsec)
+{
+	return nsec / (mira016_trow_psec(mira016) / HZ_PER_KHZ);
+}
+
+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 early integer division cause severe precision loss? The formula
h_tot * HZ_PER_MHZ / trow_psec * HZ_PER_MHZ is evaluated left-to-right.
The division by trow_psec truncates the intermediate result to an integer
before it is scaled back up by HZ_PER_MHZ, discarding all sub-megahertz
precision.

+}
+
+static inline u32 mira016_calc_min_vblank(struct mira016 *mira016, u32 y_tot)
+{
+	/*
+	 * See 3.15.2 Frame Rate, equation 4.
+	 *
+	 * TODO: The minimum frame duration has to be expanded if embedded data
+	 * are used.
+	 */
+	u32 trow_nsec = mira016_trow_psec(mira016) / HZ_PER_KHZ;
+	u32 min_duration_nsec =  trow_nsec * (y_tot + 35) + 50 * HZ_PER_KHZ;
+
+	return mira016_nsec_to_lines(mira016, min_duration_nsec) - y_tot;

[Severity: Medium]
Can this result in a vertical blanking duration that is strictly shorter than
the hardware's minimum requirement? mira016_calc_min_vblank() computes a
minimum duration in nanoseconds and converts it to lines using
mira016_nsec_to_lines(). That helper uses integer division, which rounds
down. If the required minimum translates to a fractional number of lines,
the truncated result could violate the minimum duration.

+}

[ ... ]

+static int mira016_calc_row_length(struct mira016 *mira016,
+				   struct v4l2_subdev_state *state)
+{
+	struct v4l2_mbus_framefmt *fmt = v4l2_subdev_state_get_format(state, 0);
+	u32 phy_timings = mira016_calc_phy_timings(mira016, fmt);
+	u32 adc_timings = mira016_calc_adc_timings(mira016, fmt);
+
+	mira016->timings.row_length = max(phy_timings, adc_timings);

[Severity: High]
Does this corrupt the active streaming configuration?

When userspace opens the V4L2 subdevice node, the V4L2 core allocates a new
TRY state and calls the driver's init_state callback. The mira016_init_state()
callback unconditionally calls mira016_calc_row_length(), which reads the
format from the newly created TRY state and directly overwrites the
driver-wide global variable mira016->timings.row_length.

This could reset the active device row length to match the default 8-bit
format even if the camera is currently streaming in a 10-bit or 12-bit format.

+
+	return 0;
+}
+
+static int mira016_set_pad_format(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_state *state,
+				  struct v4l2_subdev_format *fmt)
+{
+	struct mira016 *mira016 = to_mira016(sd);
+	struct v4l2_rect *crop;
+	u32 pixel_rate;
+	u32 min_vblank;
+	u32 gain_max;
+	int ret;
+
+	mira016_update_pad_format(mira016, &fmt->format, fmt->format.code);
+	*v4l2_subdev_state_get_format(state, 0) = fmt->format;
+
+	crop = v4l2_subdev_state_get_crop(state, 0);
+	crop->width = fmt->format.width;
+	crop->height = fmt->format.height;
+	crop->left = MIRA016_PIXEL_ARRAY_LEFT;
+	crop->top = MIRA016_PIXEL_ARRAY_TOP;
+
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
+		return 0;
+
+	/*
+	 * Update the row length: changing the image format implies changing the
+	 * row_length parameter, which changes the line duration and the pixel
+	 * rate consequentially. Also, changing the image format changes the
+	 * analogue gain limits.
+	 *
+	 * Do not allow to change image format while the subdevice is streaming.
+	 *
+	 * TODO: row length depends on binning, update it also in the
+	 * implementation of set_selection.
+	 */
+	if (v4l2_subdev_is_streaming(sd))
+		return -EBUSY;

[Severity: Medium]
Does this permanently alter the active state before validating if it is safe
to do so?

The subdevice state's format and crop configuration are immediately
overwritten with user-supplied values. Only after these changes does the
function check if the device is streaming and return -EBUSY. This leaves the
internal software state desynchronized from the actual hardware configuration.

+
+	switch (fmt->format.code) {
+	case MEDIA_BUS_FMT_Y8_1X8:
+		gain_max = ARRAY_SIZE(mira016_gain_lut_8bit);
+		break;
+	case MEDIA_BUS_FMT_Y10_1X10:
+		gain_max = ARRAY_SIZE(mira016_gain_lut_10bit);
+		break;
+	case MEDIA_BUS_FMT_Y12_1X12:
+	default:
+		/*
+		 * TODO: Clarify how to handle 12 bit 2x fixed gain which
+		 * changes the line timings while streaming. Only allow 1x
+		 * for the time being.
+		 */
+		gain_max = 1;
+		break;
+	}
+
+	ret = __v4l2_ctrl_modify_range(mira016->gain, 1, gain_max, 1, 1);

[Severity: High]
Does this set up an out-of-bounds array access? The V4L2 control's maximum
value is set to ARRAY_SIZE(...) rather than ARRAY_SIZE(...) - 1. When
mira016_write_analogue_gain() is called with this maximum value, it uses the
1-based maximum directly as a 0-based array index, reading past the end of
the gain lookup table.

[ ... ]

+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_CLKGEN_CP_DIV_REG,
+		  mira016->pll.cp_div, &ret);
+	cci_write(mira016->regmap, MIRA016_OTP_GRANULARITY_REG,
+		  mira016->pll.otp_gran, &ret);
+	cci_write(mira016->regmap, MIRA016_GRAN_TG_REG, mira016->pll.gran_tg, &ret);
+	cci_write(mira016->regmap, MIRA016_LUT_DEL008_REG,
+		  mira016->pll.lut_del, &ret);
+	cci_write(mira016->regmap, MIRA016_CLKGEN_TX_ESC_H_REG,
+		  mira016->pll.esc_h, &ret);
+	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);
+
+	ret = read_poll_timeout(cci_read, ret,
+				((ret < 0 || (val & MIRA016_PLL_LOCKED))),
+				1000, 1000, false, mira016->regmap,
+				MIRA016_PLL_LOCK_REG, &val, NULL);

[Severity: Medium]
Does this overwrite the accumulated error code in ret? The read_poll_timeout
assigns the result of cci_read directly to ret, replacing any prior errors
accumulated during the cci_write() sequence.

[ ... ]

+static int mira016_configure_roi(struct mira016 *mira016,
+				 struct v4l2_subdev_state *state)
+{
+	int ret = 0;
+
+	/*
+	 * Write horizontal configuration to context 0 first, then repeat for
+	 * context 1.
+	 */
+	cci_write(mira016->regmap, MIRA016_RW_CONTEXT_REG, 0, &ret);
+	cci_write(mira016->regmap, MIRA016_BANK_SEL_REG, 1, &ret);
+	ret = mira016_configure_horizontal_roi(mira016, state);

[Severity: Medium]
Is the error code from the preceding cci_write() calls silently lost here?
cci_write() accumulates errors into ret, but the assignment
ret = mira016_configure_horizontal_roi(...) completely overwrites ret,
discarding prior failures.

+	if (ret)
+		return ret;

[ ... ]

+static int mira016_init_state(struct v4l2_subdev *sd,
+			      struct v4l2_subdev_state *state)
+{
+	struct mira016 *mira016 = to_mira016(sd);
+	struct v4l2_subdev_format fmt = {
+		.which = V4L2_SUBDEV_FORMAT_TRY,
+		.pad = 0,
+		.format = {
+			.code = MEDIA_BUS_FMT_Y8_1X8,
+			.width = MIRA016_PIXEL_ARRAY_WIDTH,
+			.height = MIRA016_PIXEL_ARRAY_HEIGHT
+		},
+	};
+
+	mira016_set_pad_format(sd, state, &fmt);
+	mira016_calc_row_length(mira016, state);

[Severity: High]
As mentioned earlier in mira016_calc_row_length(), this call from
mira016_init_state() mutates the driver-wide global state when userspace
allocates a new TRY state.

+
+	return 0;
+}

[ ... ]

+static int mira016_write_analogue_gain(struct mira016 *mira016,
+				       struct v4l2_subdev_state *state,
+				       u32 gain)
+{
+	const struct mira016_gain_lut *lut;
+	struct v4l2_mbus_framefmt *format;
+	int ret = 0;
+
+	format = v4l2_subdev_state_get_format(state, 0);
+	switch (format->code) {
+	case MEDIA_BUS_FMT_Y8_1X8:
+		lut = &mira016_gain_lut_8bit[gain];

[Severity: High]
If gain matches the maximum array size limit established in
mira016_set_pad_format(), this will read memory past the end of the gain
lookup table.

[ ... ]

+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);

[Severity: High]
Could this sequence lead to a use-after-free or NULL pointer dereference
during driver removal?

In mira016_remove(), v4l2_subdev_cleanup() is called before
v4l2_ctrl_handler_free(). The v4l2_subdev_cleanup() frees the subdevice's
active state, but the control handler remains alive and accessible.

If a concurrent ioctl sets a control, it invokes mira016_set_ctrl(), which
calls v4l2_subdev_get_locked_active_state() and dereferences the already
freed state object.

+	crop = v4l2_subdev_state_get_crop(state, 0);

[ ... ]

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

[Severity: High]
As mentioned above, the active_state is destroyed by v4l2_subdev_cleanup()
while the ctrl_handler is still active and capable of receiving ioctls.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-mira016-v1-0-af0013ef070d@ideasonboard.com?part=2