Re: [PATCH v10 11/14] media: iris: Add hooks for pixel and non-pixel context banks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Dangling pointer in `vb2_queue->dev` due to synchronous unregistration of context-bank platform devices.

Pre-existing issues:
- [High] Missing cancel_delayed_work_sync() for sys_error_handler in iris_remove() leads to use-after-free.
--

--- Patch [11]: [PATCH v10 11/14] media: iris: Add hooks for pixel and non-pixel context banks ---
commit 1f1fdc377aea1a90f259a986e4bc14be351b3bc0
Author: Vikash Garodia <vikash.garodia@oss.qualcomm.com>

media: iris: Add hooks for pixel and non-pixel context banks
    
Iris platforms use separate context-bank devices for the pixel, non-pixel
firmware domains. Add platform hooks to create and destroy those
subdevices, providing a interface for their lifecycle management across
platforms.
    
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu3x.c b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> index ec35ad4f86102..bc95be22ed96d 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu3x.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> @@ -397,6 +397,52 @@ static u64 iris_vpu36_get_required_freq(struct iris_inst *inst)
>  	return max(vcodec0_freq, vcodec1_freq);
>  }
>  
> +static int iris_vpu3x_init_cb_devs(struct iris_core *core)
> +{
> +	struct device *dev;
> +
> +	dev = iris_create_cb_dev(core, "non-pixel");
> +	if (IS_ERR(dev))
> +		return PTR_ERR(dev);
> +
> +	core->np_dev = dev;
> +
> +	dev = iris_create_cb_dev(core, "pixel");
> +	if (IS_ERR(dev))
> +		goto unreg_np_dev;
> +
> +	core->p_dev = dev;
> +
> +	dev = iris_create_cb_dev(core, "video-firmware");
> +	if (IS_ERR(dev))
> +		goto unreg_p_dev;
> +
> +	core->fw_dev = dev;
> +
> +	return 0;
> +
> +unreg_p_dev:
> +	if (core->p_dev)
> +		platform_device_unregister(to_platform_device(core->p_dev));
> +	core->p_dev = NULL;
> +unreg_np_dev:
> +	if (core->np_dev)
> +		platform_device_unregister(to_platform_device(core->np_dev));
> +	core->np_dev = NULL;
> +
> +	return PTR_ERR(dev);
> +}
> +
> +static void iris_vpu3x_deinit_cb_devs(struct iris_core *core)
> +{
> +	if (core->fw_dev)
> +		platform_device_unregister(to_platform_device(core->fw_dev));
> +	if (core->p_dev)
> +		platform_device_unregister(to_platform_device(core->p_dev));
> +	if (core->np_dev)
> +		platform_device_unregister(to_platform_device(core->np_dev));
> +}

[Severity: High]
Does this code synchronously free the context-bank platform devices without
waiting for userspace V4L2 file descriptors to close?

If a userspace application holds a video device file descriptor open during
driver unbind, this function will unregister and free the context-bank
platform devices.

However, in iris_m2m_queue_init(), the context-bank devices are assigned to
vb2_queue->dev:

drivers/media/platform/qcom/iris/iris_vidc.c:iris_m2m_queue_init() {
    ...
	src_vq->dev = iris_get_cb_dev(inst, BUF_INPUT);
    ...
	dst_vq->dev = iris_get_cb_dev(inst, BUF_OUTPUT);
    ...
}

Because the V4L2 core does not hold a reference to q->dev during videobuf2
queue initialization, won't this result in a use-after-free when userspace
finally closes the file descriptor and v4l2_m2m_ctx_release() triggers
vb2_queue_release() to unmap DMA buffers?

[Severity: High]
This is a pre-existing issue, but does iris_remove() fail to cancel the
sys_error_handler delayed work before returning?

drivers/media/platform/qcom/iris/iris_probe.c:iris_remove() {
    ...
	iris_core_deinit(core);

	video_unregister_device(core->vdev_dec);
	video_unregister_device(core->vdev_enc);

	v4l2_device_unregister(&core->v4l2_dev);

	iris_deinit_cb_devs(core);

	mutex_destroy(&core->lock);
}

If a system fatal error triggers an interrupt just before or during module
removal, the sys_error_handler delayed work could be scheduled.

Since iris_remove() doesn't explicitly cancel this work before returning,
could the handler execute after the devm_kzalloc-managed core structure is
freed?

drivers/media/platform/qcom/iris/iris_probe.c:iris_sys_error_handler() {
	struct iris_core *core =
			container_of(work, struct iris_core, sys_error_handler.work);

	iris_core_deinit(core);
	iris_core_init(core);
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260726-glymur-v10-0-de451559e88b@oss.qualcomm.com?part=11