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

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] The set_fmt pad operation corrupts the active subdevice state if called while streaming, leading to an out-of-bounds array access.
- [High] V4L2 control handler is freed before unregistering the subdevice, allowing a use-after-free race condition.
- [High] mira016_init_state() calls set_pad_format() with V4L2_SUBDEV_FORMAT_TRY, skipping the active state control range and internal timing initialization.
- [High] Missing stabilization delay after de-asserting hardware reset and enabling clocks before accessing I2C registers.
- [High] I2C errors are silently swallowed by read_poll_timeout macro in mira016_configure_timings.
- [Medium] Missing pm_runtime_mark_last_busy() causes immediate suspend when dropping the runtime PM reference.
--

commit e3f67c157423cc82225f8c12e6de36ab28b0fded
Author: Jacopo Mondi [off-list ref]

media: i2c: mira016: Add driver for Mira016

Add driver for the ams OSRAM Mira016 sensor.

[ ... ]

+static int mira016_configure_timings(struct mira016 *mira016)
+{

[ ... ]

+	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))),
+				1000, 1000, false, mira016->regmap,
+				MIRA016_PLL_LOCK_REG, &val, NULL);
+	if (ret < 0)
+		return ret;

[Severity: High]
Does this read_poll_timeout loop unintentionally swallow I2C errors?

If cci_read() fails (for example, returning -EIO), the error is assigned
to ret, making the condition (ret < 0 || ...) evaluate to true. This
causes the loop to break successfully, and the macro evaluates to 0.

Because the return value of read_poll_timeout is stored in ret, it overwrites
the original I2C error with 0, completely defeating the check
if (ret < 0) below. This would cause the driver to silently ignore failures
during PLL configuration.

[ ... ]

+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;
+
+	/*
+	 * Use 'gain - 1' as the gain control values are indexed from 1
+	 * while the gain luts are 0-indexed.
+	 */
+	format = v4l2_subdev_state_get_format(state, 0);
+	switch (format->code) {
+	case MEDIA_BUS_FMT_Y8_1X8:
+		lut = &mira016_gain_lut_8bit[gain - 1];
+		break;
+	case MEDIA_BUS_FMT_Y10_1X10:
+		lut = &mira016_gain_lut_10bit[gain - 1];

[Severity: Critical]
Can this code trigger an out-of-bounds array read if userspace changes the
image format while the device is streaming?

Looking at mira016_set_pad_format(), the requested format is written to the
active subdevice state BEFORE the driver checks if the subdevice is currently
streaming:

	mira016_update_pad_format(mira016, &fmt->format, fmt->format.code);
	*v4l2_subdev_state_get_format(state, 0) = fmt->format;
	...
	if (v4l2_subdev_is_streaming(sd))
		return -EBUSY;

If the subdevice is streaming, it returns -EBUSY and skips updating the
analog gain control limits. The state has already been corrupted, though.

If userspace changes an 8-bit format to a 10-bit format while streaming,
the active state changes to 10-bit, but the control handler range remains
bound to the wider limits of the 8-bit format (max 83).

A subsequent call to VIDIOC_S_CTRL setting the gain to 83 would pass the
V4L2 core range check. Here in mira016_write_analogue_gain(), it would index
into the 49-element mira016_gain_lut_10bit array using 82, leading to an
out-of-bounds kernel memory read and an invalid hardware state.

[ ... ]

+static int mira016_set_pad_format(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_state *state,
+				  struct v4l2_subdev_format *fmt)
+{

[ ... ]

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

[Severity: High]
Does this early return prevent the active state from being fully initialized?

During probe, v4l2_subdev_init_finalize() calls mira016_init_state() to
populate the active state. However, mira016_init_state() constructs a
dummy format using V4L2_SUBDEV_FORMAT_TRY:

	struct v4l2_subdev_format fmt = {
		.which = V4L2_SUBDEV_FORMAT_TRY,
	...
	mira016_set_pad_format(sd, state, &fmt);

Because mira016_set_pad_format() immediately returns when it sees
V4L2_SUBDEV_FORMAT_TRY, it skips updating the analog gain control limit
and fails to set mira016->timings.row_length for the active state.

This leaves the control range incorrectly clamped to default probe limits
and breaks pixel rate calculations until userspace explicitly issues a
VIDIOC_SUBDEV_S_FMT to override it.

[ ... ]

+static int mira016_power_on(struct device *dev)
+{

[ ... ]

+	ret = clk_prepare_enable(mira016->xclk);
+	if (ret) {
+		dev_err(&client->dev, "%s: failed to enable clock\n", __func__);
+		goto reg_off;
+	}
+
+	gpiod_set_value_cansleep(mira016->reset_gpio, 0);
+
+	return 0;
+}

[Severity: High]
Should there be a delay here to allow the hardware to stabilize after
de-asserting the reset line?

Looking at mira016_probe(), the driver immediately issues I2C reads to the
sensor using mira016_identify_module() right after calling mira016_power_on():

	ret = mira016_power_on(dev);
	if (ret)
		return ret;
	...
	ret = mira016_identify_module(mira016);

Without a stabilization delay (e.g., usleep_range()) after de-asserting
reset and before sending I2C commands, this may cause sporadic or consistent
probe failures due to I2C timeouts or NACKs.

[ ... ]

+static int mira016_disable_streams(struct v4l2_subdev *sd,
+				   struct v4l2_subdev_state *state, u32 pad,
+				   u64 streams_mask)
+{
+	struct mira016 *mira016 = to_mira016(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(&mira016->sd);
+
+	mira016_stop_streaming(mira016);
+
+	__v4l2_ctrl_grab(mira016->hflip, false);
+	__v4l2_ctrl_grab(mira016->vflip, false);
+
+	pm_runtime_put_autosuspend(&client->dev);

[Severity: Medium]
Will this bypass the configured autosuspend delay of 1000ms?

The driver drops its PM reference here without first calling
pm_runtime_mark_last_busy(&client->dev).

If the streaming duration exceeds the autosuspend delay configured at probe
time, the runtime PM core will calculate the delay expiration against an old
last_busy timestamp. This will evaluate as already expired, causing the
device to immediately suspend rather than honoring the timeout.

[ ... ]

+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]
Does this sequence open a use-after-free race condition?

The control handler and its active state mutex are destroyed by
v4l2_ctrl_handler_free(), but this happens before the subdevice is
unregistered via v4l2_async_unregister_subdev().

Until the subdevice is unregistered, it remains fully visible to userspace.
If an ioctl (like VIDIOC_S_CTRL) is executed concurrently by userspace
while remove() is running, the V4L2 core will access the already-freed
control handler structures and attempt to acquire a destroyed mutex.

Unregistering the subdevice first would prevent userspace from accessing it
while it's being dismantled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-mira016-v2-0-1dcf7b3a807e@ideasonboard.com?part=2