Re: [PATCH v3 01/14] media: imx678: Rework into generic Starvis 2 sensor driver.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] media: UAF of active_state due to unsafe cleanup ordering in remove()
- [Low] media: sony-starvis2: sensor left active after identify
--

commit baf6081a4a9848a2866e87a317711ee36642f79e
Author: Dave Stevenson <dave.stevenson@raspberrypi.com>

    media: imx678: Rework into generic Starvis 2 sensor driver.
    
    The register layout and programming model for multiple of the
    Sony Starvis2 sensors is identical, so prepare for supporting
    those by renaming all common parts of the driver to a starvis2_
    prefix, and renaming the driver.

> diff --git a/drivers/media/i2c/imx678.c b/drivers/media/i2c/sony-starvis2.c
> index 8413fd44d6575..b4733990d03a7 100644
> --- a/drivers/media/i2c/imx678.c
> +++ b/drivers/media/i2c/sony-starvis2.c

[ ... ]

> @@ -1078,10 +1103,10 @@ static int imx678_identify_model(struct imx678 *imx678)
>  	 * This sensor's ID registers become accessible 80ms after coming out
>  	 * of STANDBY mode.
>  	 */
> -	cci_write(imx678->cci, IMX678_REG_MODE_SELECT, 0, &ret);
> -	fsleep(IMX678_MODULE_ID_DELAY);
> +	cci_write(starvis2->cci, STARVIS2_REG_MODE_SELECT, 0, &ret);
> +	fsleep(STARVIS2_MODULE_ID_DELAY);

[Severity: Low]
In starvis2_identify_model(), the sensor is woken from standby by writing 0 to
STARVIS2_REG_MODE_SELECT to read the chip ID. However, the driver never writes
STARVIS2_MODE_STANDBY (1) back to the register before returning.

While pm_runtime_idle() in probe() will normally power off the regulators
entirely, if runtime PM is disabled in the kernel (CONFIG_PM=n) or the
device's power state is forced 'on' by userspace, the regulators will remain
on.

Without reverting to standby mode, the sensor could remain active indefinitely
after probe, unnecessarily consuming power. Should a write to return to
standby mode be added before the function returns?

[ ... ]

> @@ -1380,45 +1407,45 @@ static int imx678_probe(struct i2c_client *client)

[ ... ]

> -static void imx678_remove(struct i2c_client *client)
> +static void starvis2_remove(struct i2c_client *client)
>  {
>  	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> -	struct imx678 *imx678 = to_imx678(sd);
> +	struct starvis2 *starvis2 = to_starvis2(sd);
>  
>  	v4l2_async_unregister_subdev(sd);
>  	v4l2_subdev_cleanup(sd);
>  	media_entity_cleanup(&sd->entity);
> -	v4l2_ctrl_handler_free(imx678->sd.ctrl_handler);
> +	v4l2_ctrl_handler_free(starvis2->sd.ctrl_handler);

[Severity: High]
Does this cleanup ordering create a race condition that could lead to a
use-after-free?

v4l2_async_unregister_subdev() unregisters the device node but does not
block currently executing ioctls on open file descriptors. Then,
v4l2_subdev_cleanup() unconditionally frees sd->active_state.

However, v4l2_ctrl_handler_free() (which synchronizes with running ioctls
by taking the handler lock) is called after v4l2_subdev_cleanup().

If a user opens the V4L2 subdevice node and concurrently calls an ioctl like
VIDIOC_S_CTRL, starvis2_set_ctrl() could dereference the freed active_state.

Should v4l2_ctrl_handler_free() be called before v4l2_subdev_cleanup() to
ensure proper synchronization and protect the active state from being freed
while ioctls are still using it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-media-starvis2-v3-0-c958362b5d06@raspberrypi.com?part=1