Re: [PATCH v13 2/2] media: i2c: Add driver for AMS-OSRAM Mira220

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] media: mira220: NULL pointer dereference in mira220_set_ctrl due to race with remove
- [Medium] media: mira220: missing .set_selection leaves frame windowing dead code
- [Medium] media: mira220: data race when accessing flip controls during TRY format operations
--

commit e6501c571328e79b8fc444fba484ac0380fd2bca
Author: Philippe Baetens <philippebaetens@gmail.com>

media: i2c: Add driver for AMS-OSRAM Mira220

This commit introduces the V4L2 subdev driver for the AMS-OSRAM Mira220
image sensor. It implements support for mono and RGB formats with varying
bit depths.

> diff --git a/drivers/media/i2c/mira220.c b/drivers/media/i2c/mira220.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/media/i2c/mira220.c

[ ... ]

> +#define MIRA220_FLIP_FORMAT_MASK	0x3
> +static inline unsigned int mira220_flip_format_index(struct mira220 *mira220,
> +						     unsigned int index)
> +{
> +	return (index & ~MIRA220_FLIP_FORMAT_MASK)	|
> +	       (mira220->vflip->val ? BIT(1) : 0)	|
> +	       (mira220->hflip->val ? BIT(0) : 0);
> +}

[Severity: Medium]
Is this vulnerable to a data race during V4L2_SUBDEV_FORMAT_TRY operations?

This reads the vflip and hflip control values directly without taking the
control handler lock. When this is called via pad operations like
set_pad_format() or enum_mbus_code() for a TRY request, the V4L2 core holds
the file handle's TRY state lock rather than the active state lock.

Since a concurrent VIDIOC_S_CTRL ioctl can modify these values under the
control handler lock, could this result in read tearing or unexpected
behavior from concurrent modifications?

[ ... ]

> +static int mira220_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct mira220 *mira220 =
> +		container_of(ctrl->handler, struct mira220, ctrl_handler);
> +	struct i2c_client *client = v4l2_get_subdevdata(&mira220->sd);
> +	struct v4l2_mbus_framefmt *format;
> +	struct v4l2_subdev_state *state;
> +	int ret = 0;
> +
> +	state = v4l2_subdev_get_locked_active_state(&mira220->sd);
> +	format = v4l2_subdev_state_get_format(state, 0);

[Severity: High]
Could this lead to a NULL pointer dereference during device unbind?

This function fetches the locked active state and blindly dereferences the
format pointer. If a concurrent VIDIOC_S_CTRL ioctl happens during device
removal, the active state might have already been freed by
v4l2_subdev_cleanup() before the control handler is unregistered.

Should the control handler be freed before destroying the subdev state
in mira220_remove() to prevent this race window?

[ ... ]

> +static int mira220_set_framefmt(struct mira220 *mira220,
> +				struct v4l2_subdev_state *state)
> +{

[ ... ]

> +	/*
> +	 * Program the ROI area, centered in the sensor's pixel array.
> +	 *
> +	 * TODO: Use the crop rectangle sizes once the driver is ported to the
> +	 * new RAW camera sensor model.
> +	 */
> +	top = (MIRA220_PIXEL_ARRAY_HEIGHT - format->height) / 2;
> +	left = (MIRA220_PIXEL_ARRAY_WIDTH - format->width) / 2;

[Severity: Medium]
Is the dynamic windowing calculation here effectively dead code?

The commit message mentions dynamic frame windowing support. However,
since the driver forces the format to the 1600x1400 maximum resolution in
mira220_set_pad_format() and omits the .set_selection callback in the
v4l2_subdev_pad_ops structure, userspace has no mechanism to change the
crop rectangle.

Should a .set_selection callback be added so that format->height and
format->width can actually vary?

[ ... ]

> +static const struct v4l2_subdev_pad_ops mira220_pad_ops = {
> +	.enum_mbus_code = mira220_enum_mbus_code,
> +	.get_fmt = v4l2_subdev_get_fmt,
> +	.set_fmt = mira220_set_pad_format,
> +	.get_selection = mira220_get_selection,
> +	.enum_frame_size = mira220_enum_frame_size,
> +	.enable_streams = mira220_enable_streams,
> +	.disable_streams = mira220_disable_streams,
> +};

[Severity: Medium]
This is related to the previous question: does the lack of a .set_selection
callback leave the windowing feature incomplete?

[ ... ]

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

[Severity: High]
Does freeing the control handler after cleaning up the subdev state
introduce the NULL pointer dereference race mentioned above?

v4l2_subdev_cleanup() frees the active state without taking the control
handler lock. Because the control handler is still registered, userspace
can still invoke mira220_set_ctrl(), which will try to access the now-NULL
active state format pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005-mira220-v13-0-27a041933929@ideasonboard.com?part=2