Re: [PATCH v6 2/2] media: i2c: add Aptina MT9M113 1.3 Mpx SoC sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `mt9m113_start_streaming()` error path explicitly powers off the hardware but leaves the PM state as `RPM_ACTIVE`. Subsequent operations (like STREAMON retries, `s_ctrl`, or `remove`) trust the active PM state and perform illegal operations, causing clk/regulator refcount underflows and I2C timeouts.
- [High] Tying the allocation lifetime of the sensor structure to V4L2 subdev `.release` callbacks introduces a memory leak on unbound initialization and a Use-After-Free during bridge unbind.
- [High] Concurrency between `VIDIOC_S_CTRL` / `VIDIOC_STREAMON` ioctls and `mt9m113_remove()` causes Use-After-Free on devres-managed resources (e.g., `regmap`, `clk`).
--

commit f0fc2c67299ebd9a65a3dfa2e680f9a4ff64a2cb
Author: Herman van Hazendonk <github.com@herrie.org>

    media: i2c: add Aptina MT9M113 1.3 Mpx SoC sensor driver
    
    Add a V4L2 subdev driver for the Aptina MT9M113 1.3 Megapixel SoC
    image sensor (1280x1024 active pixel array) with on-chip ISP.

> diff --git a/drivers/media/i2c/mt9m113.c b/drivers/media/i2c/mt9m113.c
> --- a/drivers/media/i2c/mt9m113.c
> +++ b/drivers/media/i2c/mt9m113.c
> @@ -1876,14 +1876,8 @@ static int mt9m113_start_streaming(struct mt9m113 *sensor,
[ ... ]
>  	sensor->chip_off = true;
>  	pm_runtime_put_noidle(dev);
>  	return ret;
>  }

[Severity: High]
Does this leave the PM state as RPM_ACTIVE while the hardware is physically
powered off?

If the retry loop exhausts all attempts, the hardware is explicitly powered
down. However, because the PM state is left active, subsequent operations
like ioctls or remove() will trust the PM state and attempt hardware accesses
or call power down again.

Could this lead to clock and regulator refcount underflows in remove() or
I2C timeouts in s_ctrl() since they believe the chip is powered on?

[ ... ]

> @@ -2293,12 +2293,7 @@ static int mt9m113_ifp_registered(struct v4l2_subdev *sd)
>  {
>  	struct mt9m113 *sensor = ifp_to_mt9m113(sd);
>  	int ret;
>  
>  	ret = v4l2_device_register_subdev(sd->v4l2_dev, &sensor->pa.sd);

[ ... ]

> @@ -3246,26 +3246,12 @@ static void mt9m113_remove(struct i2c_client *client)
[ ... ]
>  	pm_runtime_disable(dev);
>  	pm_runtime_dont_use_autosuspend(dev);
>  	if (!pm_runtime_status_suspended(dev))
>  		mt9m113_power_off(sensor);

[Severity: High]
Can the sensor structure be freed before remove() is called or leaked
entirely?

The allocation lifetime of the sensor structure is tied to the V4L2 subdev
release callbacks, using an atomic release_count initialized to 2. However,
the pixel array subdev pa.sd is only registered in mt9m113_ifp_registered()
when a bridge attaches.

If the module is unloaded before a bridge binds, pa.sd is never registered,
its release callback never runs, and the sensor structure leaks.

Conversely, if a bridge unbinds, v4l2_device_unregister_subdev() invokes
the release callbacks for both subdevs, freeing the sensor structure. A
subsequent execution of remove() would then dereference the already freed
sensor when calling mt9m113_power_off() and v4l2_async_unregister_subdev().

Could this result in a use-after-free?

>  	pm_runtime_set_suspended(dev);
>  
>  	v4l2_async_unregister_subdev(&sensor->ifp.sd);

[Severity: High]
Does this remove process leave devres-managed resources vulnerable to a
use-after-free from concurrent ioctls?

By deferring the freeing of the sensor structure to the subdev release
callbacks, the control handlers can outlive remove() if an application holds
an open file descriptor.

Hardware resources like sensor->regmap and sensor->clk are managed by devres
and are automatically freed when remove() returns.

If an ioctl like VIDIOC_S_CTRL successfully calls pm_runtime_get_if_in_use()
just before remove() calls pm_runtime_disable(), will the control handler
thread attempt to use the regmap structure after devres has already destroyed
it?

>  	/* @sensor may already be freed here; do not dereference it. */
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260606-submit-media-mt9m113-v6-0-8f6d0f79f4d1@herrie.org?part=2