Re: [PATCH v8 17/18] media: venus: skip QCM2290 if Iris driver is enabled

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unbalanced teardown of delayed work core->work leads to Use-After-Free.
- [Medium] Memory and DMA allocations are leaked on the venus_probe() error path.
--

--- Patch [17]: [PATCH v8 17/18] media: venus: skip QCM2290 if Iris driver is enabled ---
commit 3f4135bbccd5abb2cbdcfce435629b2b86cda1f5
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

media: venus: skip QCM2290 if Iris driver is enabled

As the Iris driver now supports the QCM2290 hardware too, there is a
race between Venus and Iris drivers on binding to the corresponding
device. Follow the approach used by other platforms and skip QCM2290 in
the Venus driver if Iris is enabled.

> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 243e342b0ae75..3c88594eb1d00 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
[ ... ]
> @@ -1169,13 +1168,16 @@ static const struct venus_resources qcm2290_res = {
>  	.enc_nodename = "video-encoder",
>  	.min_fw = &min_fw,
>  };
> +#endif
>  
>  static const struct of_device_id venus_dt_match[] = {
>  	{ .compatible = "qcom,msm8916-venus", .data = &msm8916_res, },
>  	{ .compatible = "qcom,msm8939-venus", .data = &msm8939_res, },
>  	{ .compatible = "qcom,msm8996-venus", .data = &msm8996_res, },
>  	{ .compatible = "qcom,msm8998-venus", .data = &msm8998_res, },
> +#if (!IS_ENABLED(CONFIG_VIDEO_QCOM_IRIS))
>  	{ .compatible = "qcom,qcm2290-venus", .data = &qcm2290_res, },
> +#endif
>  	{ .compatible = "qcom,sc7180-venus", .data = &sc7180_res, },
>  	{ .compatible = "qcom,sdm660-venus", .data = &sdm660_res, },
>  	{ .compatible = "qcom,sdm845-venus", .data = &sdm845_res, },

[Severity: Medium]
This is a pre-existing issue, but does the error path in venus_probe() leak
memory and DMA allocations?

Looking at venus_probe(), if devm_request_threaded_irq() fails, the code jumps
directly to err_core_put:

drivers/media/platform/qcom/venus/core.c:venus_probe() {
    ...
	ret = devm_request_threaded_irq(dev, core->irq, hfi_isr, venus_isr_thread,
					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
					"venus", core);
	if (ret)
		goto err_core_put;
    ...
err_hfi_destroy:
	hfi_destroy(core);
err_core_put:
    ...
}

This bypasses err_hfi_destroy and skips the hfi_destroy() cleanup, permanently
leaking the resources allocated by hfi_create(). Should the failure path jump
to err_hfi_destroy instead?

[Severity: Critical]
This is a pre-existing issue, but could the delayed work core->work lead to a
use-after-free?

In the venus_probe() error path, cancel_delayed_work_sync() is omitted entirely.
If the IRQ is successfully registered and fires before probe aborts, it can
queue core->work which could execute after the core object is freed.

Additionally, in venus_remove():

drivers/media/platform/qcom/venus/core.c:venus_remove() {
	cancel_delayed_work_sync(&core->work);
    ...
	hfi_destroy(core);
    ...
}

cancel_delayed_work_sync() is invoked before the IRQ is disabled in
hfi_destroy(). If an interrupt fires between these two calls, the ISR can
re-queue the work, which will subsequently execute on a freed core object. 

Could we move the cancel_delayed_work_sync() call after the IRQ is disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-iris-ar50lt-v8-0-6597c08cea6f@oss.qualcomm.com?part=17