Re: [PATCH 3/3] media: iris: Fix use-after-free on unbind with open sessions
From: Vishnu Reddy
Date: Mon Nov 09 2026 - 00:10:22 EST
On 11/3/2026 5:10 PM, Bryan O'Donoghue wrote:
> On 17/10/2026 04:16, Vishnu Reddy wrote:
>> The driver unbind can be triggered through sysfs while a client has a
>> video session running. The devm resources will be freed when the unbind
>> completes. That leads to use after free for the running sessions.
>>
>> Keep the memory of the core and its device allocated until the last
>> file descriptor is closed. Stop new sessions as soon as the removal
>> starts. Make sure the interrupt thread cannot run on the core after it
>> is freed.
>
> There's too much going on in this patch in one place. The commit log is
> indicative of that too. At a minimum the IRQ stuff should be separated because
> IMO its not really a use-after-free its a race condition that already exists,
> its just more obvious when you free in this fashion.
I'll split the IRQ change as a separate patch in next revision.
>
>>
>> Fixes: 38506cb7e8d2 ("media: iris: add platform driver for iris video device")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: Vishnu Reddy <busanna.reddy@xxxxxxxxxxxxxxxx>
>> ---
>> drivers/media/platform/qcom/iris/iris_probe.c | 56 +++++++++++++++++++--------
>> 1 file changed, 40 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c
>> b/drivers/media/platform/qcom/iris/iris_probe.c
>> index 4cb678bd56f0..25876957381b 100644
>> --- a/drivers/media/platform/qcom/iris/iris_probe.c
>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c
>> @@ -243,6 +243,15 @@ static int iris_register_video_device(struct iris_core
>> *core, enum domain_type t
>> return ret;
>> }
>>
>> +static void iris_core_release(struct v4l2_device *v4l2_dev)
>> +{
>> + struct iris_core *core = container_of(v4l2_dev, struct iris_core,
>> v4l2_dev);
>> +
>> + mutex_destroy(&core->lock);
>> + put_device(core->dev);
>> + kfree(core);
>> +}
>> +
>
> What you would have here is an admixture of devm allocated stuff and
> hand-allocated, refcounted with custom v4l2_release() which might all work
> but, it's not obvious to me does work i.e. core is hand allocated while
> reg_base, response_packet etc are devm() managed.
reg_base and response_packet are safe: after iris_remove() the IRQ is
freed and the core is in IRIS_CORE_DEINIT, so nothing uses them. Only
the core structure must outlive remove(), so only that is refcounted.
>
> A much simpler solution to your problem is :
>
> static struct platform_driver iris_driver = {
> .driver = {
> .suppress_bind_attrs = true,
> ...
> },
> };
>
This was discussed in [1]. Dmitry noted it is a sign of a badly written driver,
and I agree: it removes the unbind interface to avoid the problem instead of
fixing remove().
[1]
https://lore.kernel.org/all/CAO9ioeUvV+3sME9P1bJTpDOSty=uojOKak659XCf337++vPVNg@xxxxxxxxxxxxxx
>> static void iris_remove(struct platform_device *pdev)
>> {
>> struct iris_core *core;
>> @@ -251,18 +260,20 @@ static void iris_remove(struct platform_device *pdev)
>> if (!core)
>> return;
>>
>> + video_unregister_device(core->vdev_dec);
>> + video_unregister_device(core->vdev_enc);
>> +
>> disable_delayed_work_sync(&core->sys_error_handler);
>>
>> iris_core_deinit(core);
>>
>> - video_unregister_device(core->vdev_dec);
>> - video_unregister_device(core->vdev_enc);
>> + devm_free_irq(core->dev, core->irq, core);
>
>
>
>>
>> v4l2_device_unregister(&core->v4l2_dev);
>>
>> iris_unreserve_iova_region(core->dev, core->iova_state);
>>
>> - mutex_destroy(&core->lock);
>> + v4l2_device_put(&core->v4l2_dev);
>> }
>>
>> static void iris_sys_error_handler(struct work_struct *work)
>> @@ -281,53 +292,64 @@ static int iris_probe(struct platform_device *pdev)
>> u64 dma_mask;
>> int ret;
>>
>> - core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL);
>> + core = kzalloc_obj(*core);
>> if (!core)
>> return -ENOMEM;
>> - core->dev = dev;
>> + core->dev = get_device(dev);
>> +
>> + core->v4l2_dev.release = iris_core_release;
>> + kref_init(&core->v4l2_dev.ref);
>>
>> core->state = IRIS_CORE_DEINIT;
>> mutex_init(&core->lock);
>> init_completion(&core->core_init_done);
>>
>> core->response_packet = devm_kzalloc(core->dev, IFACEQ_CORE_PKT_SIZE,
>> GFP_KERNEL);
>> - if (!core->response_packet)
>> - return -ENOMEM;
>> + if (!core->response_packet) {
>> + ret = -ENOMEM;
>> + goto err_put_core;
>> + }
>>
>> INIT_LIST_HEAD(&core->instances);
>> INIT_DELAYED_WORK(&core->sys_error_handler, iris_sys_error_handler);
>>
>> core->reg_base = devm_platform_ioremap_resource(pdev, 0);
>> - if (IS_ERR(core->reg_base))
>> - return PTR_ERR(core->reg_base);
>> + if (IS_ERR(core->reg_base)) {
>> + ret = PTR_ERR(core->reg_base);
>> + goto err_put_core;
>> + }
>>
>> core->irq = platform_get_irq(pdev, 0);
>> - if (core->irq < 0)
>> - return core->irq;
>> + if (core->irq < 0) {
>> + ret = core->irq;
>> + goto err_put_core;
>> + }
>>
>> core->iris_platform_data = of_device_get_match_data(core->dev);
>>
>> core->ubwc_cfg = qcom_ubwc_config_get_data();
>> - if (IS_ERR(core->ubwc_cfg))
>> - return PTR_ERR(core->ubwc_cfg);
>> + if (IS_ERR(core->ubwc_cfg)) {
>> + ret = PTR_ERR(core->ubwc_cfg);
>> + goto err_put_core;
>> + }
>>
>> ret = devm_request_threaded_irq(core->dev, core->irq, iris_hfi_isr,
>> iris_hfi_isr_handler,
>> IRQF_TRIGGER_HIGH | IRQF_NO_AUTOEN,
>> "iris", core);
>> if (ret)
>> - return ret;
>> + goto err_put_core;
>>
>> iris_init_ops(core);
>>
>> ret = iris_init_resources(core);
>> if (ret)
>> - return ret;
>> + goto err_put_core;
>>
>> ret = iris_reserve_iova_region(dev, &core->iova_state,
>> IRIS_NP_RESERVE_IOVA_START,
>> IRIS_NP_RESERVE_IOVA_SIZE);
>> if (ret)
>> - return ret;
>> + goto err_put_core;
>>
>> ret = v4l2_device_register(dev, &core->v4l2_dev);
>> if (ret)
>> @@ -368,6 +390,8 @@ static int iris_probe(struct platform_device *pdev)
>> v4l2_device_unregister(&core->v4l2_dev);
>> err_unresv_iova_region:
>> iris_unreserve_iova_region(dev, core->iova_state);
>> +err_put_core:
>> + v4l2_device_put(&core->v4l2_dev);
>>
>> return ret;
>> }
>>
>> --
>> 2.34.1
>
>