Re: [PATCH v2 20/20] media: imx355: Remove storing cur_mode in the state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `imx355_set_stream` function and its callee `imx355_start_streaming` execute without acquiring the subdevice active state lock, leading to `lockdep_assert_held` failures and data races with controls.
- [High] Use-after-free of subdev active state in `imx355_set_ctrl()` during device unbind due to incorrect cleanup ordering.
--

--- Patch [20]: [PATCH v2 20/20] media: imx355: Remove storing cur_mode in the state ---
commit d3278174d03d9c643934fc21a9e1c67e3a90a3fb
Author: Dave Stevenson <dave.stevenson@raspberrypi.com>

media: imx355: Remove storing cur_mode in the state

All the information for the mode is now stored within
the subdev state, so configure the sensor based on that.

> diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
> index bb620ebb074f7..f31833e5d1d12 100644
> --- a/drivers/media/i2c/imx355.c
> +++ b/drivers/media/i2c/imx355.c
> @@ -594,6 +591,8 @@ static int imx355_set_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct imx355 *imx355 = container_of(ctrl->handler,
>  					     struct imx355, ctrl_handler);
> +	const struct v4l2_mbus_framefmt *format = NULL;
> +	struct v4l2_subdev_state *state;
>  	s64 max;
>  	int ret;
>  
> @@ -604,7 +603,7 @@ static int imx355_set_ctrl(struct v4l2_ctrl *ctrl)
>  	switch (ctrl->id) {
>  	case V4L2_CID_VBLANK:
>  		/* Update max exposure while meeting expected vblanking */
> -		max = imx355->cur_mode->height + ctrl->val - IMX355_EXPOSURE_OFFSET;
> +		max = format->height + ctrl->val - IMX355_EXPOSURE_OFFSET;

[Severity: High]
Can this dereference a freed active state?

If the driver is unbound (e.g. via sysfs) while a userspace process
concurrently accesses a v4l2 control, imx355_remove() is called.

In imx355_remove(), the active state is freed before the control handler:

imx355_remove()
    v4l2_subdev_cleanup(sd);
    media_entity_cleanup(&sd->entity);
    v4l2_ctrl_handler_free(sd->ctrl_handler);

This leaves a window where the concurrent ioctl can acquire the control lock,
enter imx355_set_ctrl(), retrieve the now-freed active state, and trigger a
use-after-free when reading format->height.

[ ... ]

> @@ -809,9 +804,11 @@ static int imx355_entity_init_state(struct v4l2_subdev *subdev,
>  /* Start streaming */
>  static int imx355_start_streaming(struct imx355 *imx355)
>  {
> -	const struct imx355_reg_list *reg_list;
> +	const struct v4l2_mbus_framefmt *fmt;
> +	struct v4l2_subdev_state *state;
>  	const struct imx355_mode *mode;
>  	int lane_idx = imx355->hwcfg->num_lanes == 4 ? 0 : 1;
> +	struct v4l2_rect *crop;
>  	u64 link_bitrate;
>  	u8 binning_mode;
>  	int ret = 0;
> @@ -820,25 +817,29 @@ static int imx355_start_streaming(struct imx355 *imx355)
>  	cci_multi_reg_write(imx355->regmap, imx355_global_regs,
>  			    ARRAY_SIZE(imx355_global_regs), &ret);
>  
> -	/* Apply default values of current mode */
> -	mode = imx355->cur_mode;
> -	reg_list = &mode->reg_list;
> -	cci_multi_reg_write(imx355->regmap, reg_list->regs,
> -			    reg_list->num_of_regs, &ret);
> +	/* Apply values of current mode */
> +	state = v4l2_subdev_get_locked_active_state(&imx355->sd);

[Severity: High]
Does this code execute without acquiring the subdevice active state lock?

Calling v4l2_subdev_get_locked_active_state() will trigger a lockdep failure
if the lock is not held.

This can happen when starting the video stream via userspace invokes the
.s_stream callback:

imx355_set_stream()
    imx355_start_streaming()
        state = v4l2_subdev_get_locked_active_state(&imx355->sd);

Since imx355_set_stream() doesn't acquire the lock before calling
imx355_start_streaming(), this triggers a lockdep failure.

Additionally, the missing lock leaves the subsequent calls to
__v4l2_ctrl_handler_setup() and __v4l2_ctrl_grab() unlocked, exposing a data
race with concurrent userspace control updates.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-media-imx355-v2-0-1683ec07b897@raspberrypi.com?part=20