Re: [PATCH 3/3] media: iris: Fix use-after-free on unbind with open sessions

[Vishnu Reddy <busanna.reddy@oss.qualcomm.com>]

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@vger.kernel.org
>> Signed-off-by: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>
>> ---
>>   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@mail.gmail.com

>>   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
>
>