Re: [PATCH v5 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 6 potential issue(s) to consider:
- [High] Use-after-free of the `sensor` structure in `mt9m113_remove()`.
- [High] Use-after-free of the `link_frequencies` array via the V4L2 control handler.
- [High] Use-after-free of `devm`-managed resources (e.g., `regmap`) in `mt9m113_s_ctrl()`.
- [High] Runtime PM state desynchronization leading to clock and regulator underflows in `mt9m113_start_streaming()`.
- [Medium] Memory leak of the dynamically allocated `sensor` structure on early probe error paths.
- [Medium] V4L2 API compliance violation: incorrectly rejecting `V4L2_SUBDEV_FORMAT_TRY` queries while streaming.
--

commit fc8d6a3b86a58b31fdfee3e786e86929c5b8b724
Author: Herman van Hazendonk <github.com@herrie.org>
Subject: media: i2c: add Aptina MT9M113 1.3 Mpx SoC sensor driver

Adds a V4L2 subdev driver for the Aptina MT9M113 1.3 Megapixel SoC image 
sensor with on-chip ISP, supporting Context A and B operation modes.

> diff --git a/drivers/media/i2c/mt9m113.c b/drivers/media/i2c/mt9m113.c
> --- /dev/null
> +++ b/drivers/media/i2c/mt9m113.c
[ ... ]
> +static int mt9m113_start_streaming(struct mt9m113 *sensor,
> +				   struct v4l2_subdev_state *state)
> +{
> +	struct device *dev = &sensor->client->dev;
[ ... ]
> +		ret = mt9m113_sensor_init(sensor);
> +		if (ret) {
> +			mt9m113_power_off(sensor);
> +			break;
> +		}
> +	}
> +
> +	/*
> +	 * Every break above leaves the chip powered off (the final attempt
> +	 * branch, the power_on failure branch and the sensor_init failure
> +	 * branch each call mt9m113_power_off before breaking). Just sync the
> +	 * PM core to SUSPENDED so a later resume re-invokes power_on, and
> +	 * drop the reference taken at the top of this function.
> +	 */
> +	pm_runtime_set_suspended(dev);
> +	pm_runtime_put_noidle(dev);
> +	return ret;
> +}

[Severity: High]
Does this cause a runtime PM state desynchronization?

Calling pm_runtime_set_suspended() here while runtime PM is still enabled 
fails silently with -EAGAIN, leaving the PM core in the RPM_ACTIVE state. 
Later, when the usage counter drops to zero and auto-suspend fires, 
mt9m113_runtime_suspend() will execute and call mt9m113_power_off() a 
second time.

Could this mismatched disable lead to clk_disable_unprepare() and 
regulator_bulk_disable() underflow warnings?

[ ... ]
> +static int mt9m113_ifp_set_fmt(struct v4l2_subdev *sd,
> +			       struct v4l2_subdev_state *state,
> +			       struct v4l2_subdev_format *fmt)
> +{
> +	struct mt9m113 *sensor = ifp_to_mt9m113(sd);
[ ... ]
> +	/*
> +	 * Reject geometry/format changes while the pipeline is live.
> +	 * The sensor is only programmed in mt9m113_stream_on(); silently
> +	 * updating the active state would let userspace see one format while
> +	 * the wire still carries the previous one.
> +	 */
> +	if (sensor->streaming)
> +		return -EBUSY;

[Severity: Medium]
Is it intentional to reject V4L2_SUBDEV_FORMAT_TRY queries while streaming?

This check blocks all format changes when sensor->streaming is true, but 
V4L2_SUBDEV_FORMAT_TRY is used by userspace to safely test formatting 
parameters without affecting the active hardware pipeline. 

Could this check be narrowed to allow TRY requests during streaming?

[ ... ]
> +static int mt9m113_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct mt9m113 *sensor = container_of(ctrl->handler,
> +					       struct mt9m113, ifp.hdl);
> +	int pm_ret, ret = 0;
> +
> +	/*
> +	 * pm_runtime_get_if_in_use() returns >0 on success (HW active, kref
> +	 * incremented), 0 if the device is suspended, and -EINVAL when
> +	 * CONFIG_PM is disabled. Treat -EINVAL as "PM core is absent, HW is
> +	 * always on" and proceed without taking a kref (the matching put at
> +	 * the tail must then be skipped). Only 0 means "skip silently".
> +	 */
> +	pm_ret = pm_runtime_get_if_in_use(&sensor->client->dev);
> +	if (pm_ret == 0)
> +		return 0;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_HFLIP: {
> +		u64 mode_a, mode_b;
> +
> +		ret = mt9m113_read_mcu_var(sensor, MT9M113_SENSOR_READ_MODE_A,
> +					   &mode_a);

[Severity: High]
Can this lead to a use-after-free of devm-managed resources?

If a user-space application holds the subdevice file descriptor open and 
calls VIDIOC_S_CTRL after unbind, mt9m113_s_ctrl() will execute. Because 
pm_runtime_disable() was called in remove(), pm_runtime_get_if_in_use() 
will return -EINVAL.

Since the check is exactly pm_ret == 0, the negative error code allows the 
code to proceed and access sensor->regmap to write hardware registers. At 
this point, the devm-managed regmap has already been freed by the driver 
core.

[ ... ]
> +static int mt9m113_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct mt9m113 *sensor;
> +	int ret;
> +
> +	sensor = kzalloc(sizeof(*sensor), GFP_KERNEL);
> +	if (!sensor)
> +		return -ENOMEM;
> +
> +	sensor->client = client;
> +	/*
> +	 * Initialise to 2 so kfree(sensor) only runs after both the PA and IFP
> +	 * release callbacks have fired (i.e. all open /dev/v4l-subdev* fds have
> +	 * been closed). See mt9m113_pa_release() for the full rationale.
> +	 */
> +	atomic_set(&sensor->release_count, 2);
> +
> +	sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
> +	if (IS_ERR(sensor->regmap))
> +		return PTR_ERR(sensor->regmap);
> +
> +	ret = mt9m113_parse_dt(sensor);
> +	if (ret < 0)
> +		return ret;

[Severity: Medium]
Does this leak the dynamically allocated sensor structure?

The sensor struct is allocated with kzalloc() to tie its lifetime to the 
V4L2 subdevice character devices. However, if early probe operations like 
devm_cci_regmap_init_i2c() or mt9m113_parse_dt() fail, the driver returns 
directly. 

Could these error paths be updated to ensure kfree(sensor) is called?

[ ... ]
> +	v4l2_ctrl_handler_init(&sensor->pa.hdl, 2);
> +	{
> +		struct v4l2_ctrl *link_freq_ctrl;
> +
> +		link_freq_ctrl = v4l2_ctrl_new_int_menu(&sensor->pa.hdl, NULL,
> +				V4L2_CID_LINK_FREQ,
> +				sensor->bus_cfg.nr_of_link_frequencies - 1, 0,
> +				sensor->bus_cfg.link_frequencies);
[ ... ]
> +static void mt9m113_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct mt9m113 *sensor = ifp_to_mt9m113(sd);
> +	struct device *dev = &client->dev;
[ ... ]
> +	v4l2_async_unregister_subdev(&sensor->ifp.sd);
> +
> +	v4l2_fwnode_endpoint_free(&sensor->bus_cfg);
> +
> +	pm_runtime_disable(dev);
> +	pm_runtime_dont_use_autosuspend(dev);
> +	if (!pm_runtime_status_suspended(dev))
> +		mt9m113_power_off(sensor);
> +	pm_runtime_set_suspended(dev);
> +}

[Severity: High]
Can this cause a use-after-free of the sensor structure?

v4l2_async_unregister_subdev() detaches the subdevices, which results in 
the synchronous invocation of their release callbacks if no file 
descriptors are currently open. The driver's release callbacks decrement 
release_count and free the sensor struct. 

When v4l2_async_unregister_subdev() returns, the driver continues to 
access sensor->bus_cfg and passes the freed sensor pointer to 
mt9m113_power_off(), leading to a direct use-after-free.

[Severity: High]
Could v4l2_fwnode_endpoint_free() here also cause a use-after-free of the 
link_frequencies array via the V4L2 control handler?

During probe, the driver passes sensor->bus_cfg.link_frequencies to 
v4l2_ctrl_new_int_menu(), which stores the array pointer directly in 
ctrl->qmenu_int. 

In mt9m113_remove(), v4l2_fwnode_endpoint_free() frees the 
link_frequencies array. However, the V4L2 control handler's lifetime is 
tied to the subdevice file descriptor. If a user-space process holds the 
file descriptor open after the device is unbound, a subsequent 
VIDIOC_QUERYMENU ioctl will read from the freed qmenu_int pointer.

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