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

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

On 11/10/2026 8:12 PM, Bryan O'Donoghue wrote:
> On 10/11/2026 14:09, Vishnu Reddy wrote:
>> The driver can be unbound through sysfs while a client has a video
>> session open. The core is devm allocated and freed when remove()
>> returns, so the open sessions then use freed memory.
>>
>> Only the core structure needs to outlive remove(), because it is the
>> only thing the file operations of an open session still use. Allocate
>> it with kzalloc and tie it to the reference count of its v4l2_device,
>> which every registered video device holds. It is freed when the last
>> file descriptor is closed. The other devm resources (register mapping,
>> response packet, clocks, power domains, interconnects) are only used by
>> the power on/off sequence, the clock and bandwidth votes and the
>> interrupt handler. All of them are released with the unbind, after the
>> core is shut down.
>>
>> To make sure nothing uses them afterwards, set a removing flag at the
>> start of iris_remove(). It makes core init, new sessions, command
>> writes and the clock and interconnect votes fail with -ENODEV, so a
>> client still holding a file descriptor cannot power the hardware on or
>> vote for resources that are gone. A client waiting for the firmware
>> only times out and returns an error without touching them. Runtime PM
>> is also disabled explicitly in iris_remove() instead of with devm,
>> because its callbacks use the core.
>>
>> Fixes: 38506cb7e8d2 ("media: iris: add platform driver for iris video device")
>> Reported-by: sashiko-bot <sashiko-bot@kernel.org>
>> Closes: https://lore.kernel.org/all/20260926064921.976721F000FF@smtp.kernel.org
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>
>> ---
>>   drivers/media/platform/qcom/iris/iris_core.c      | 10 +++++-
>>   drivers/media/platform/qcom/iris/iris_core.h      |  2 ++
>>   drivers/media/platform/qcom/iris/iris_hfi_queue.c | 15 +++++++-
>>   drivers/media/platform/qcom/iris/iris_power.c     | 13 +++++++
>>   drivers/media/platform/qcom/iris/iris_probe.c     | 44 ++++++++++++++++++-----
>>   drivers/media/platform/qcom/iris/iris_vidc.c      |  3 ++
>>   6 files changed, 76 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/iris/iris_core.c
>> b/drivers/media/platform/qcom/iris/iris_core.c
>> index d558475986ff0..bdac6b8c98e44 100644
>> --- a/drivers/media/platform/qcom/iris/iris_core.c
>> +++ b/drivers/media/platform/qcom/iris/iris_core.c
>> @@ -43,7 +43,10 @@ static int iris_wait_for_system_response(struct iris_core
>> *core)
>>       ret = wait_for_completion_timeout(&core->core_init_done,
>>                         msecs_to_jiffies(HW_RESPONSE_TIMEOUT_VALUE));
>>       if (!ret) {
>> -        core->state = IRIS_CORE_ERROR;
>> +        mutex_lock(&core->lock);
>> +        if (!core->removing)
>> +            core->state = IRIS_CORE_ERROR;
>> +        mutex_unlock(&core->lock);
>>           return -ETIMEDOUT;
>>       }
>>   @@ -56,6 +59,11 @@ int iris_core_init(struct iris_core *core)
>>       int ret;
>>         mutex_lock(&core->lock);
>> +    if (core->removing) {
>> +        ret = -ENODEV;
>> +        goto exit;
>> +    }
>> +
>>       if (core->state == IRIS_CORE_INIT) {
>>           ret = 0;
>>           goto exit;
>> diff --git a/drivers/media/platform/qcom/iris/iris_core.h
>> b/drivers/media/platform/qcom/iris/iris_core.h
>> index 3e4cf652bf39a..379f0666a42cb 100644
>> --- a/drivers/media/platform/qcom/iris/iris_core.h
>> +++ b/drivers/media/platform/qcom/iris/iris_core.h
>> @@ -62,6 +62,7 @@ struct qcom_ubwc_cfg_data;
>>    * @iris_firmware_desc: a pointer to the firmware-specific descriptive data
>>    * @ubwc_cfg: UBWC configuration for the platform
>>    * @state: current state of core
>> + * @removing: set when driver removal starts, blocks core init and resource
>> votes
>>    * @iface_q_table_daddr: device address for interface queue table memory
>>    * @sfr_daddr: device address for SFR (Sub System Failure Reason) register
>> memory
>>    * @iface_q_table_vaddr: virtual address for interface queue table memory
>> @@ -108,6 +109,7 @@ struct iris_core {
>>       const struct iris_firmware_desc        *iris_firmware_desc;
>>       const struct qcom_ubwc_cfg_data        *ubwc_cfg;
>>       enum iris_core_state            state;
>> +    bool                    removing;
>>       dma_addr_t                iface_q_table_daddr;
>>       dma_addr_t                sfr_daddr;
>>       void                    *iface_q_table_vaddr;
>> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_queue.c
>> b/drivers/media/platform/qcom/iris/iris_hfi_queue.c
>> index 545bbe75e1740..e0cfa5107d1d9 100644
>> --- a/drivers/media/platform/qcom/iris/iris_hfi_queue.c
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_queue.c
>> @@ -128,13 +128,26 @@ int iris_hfi_queue_cmd_write_locked(struct iris_core
>> *core, void *pkt, u32 pkt_s
>>     int iris_hfi_queue_cmd_write(struct iris_core *core, void *pkt, u32
>> pkt_size)
>>   {
>> -    int ret;
>> +    int ret = 0;
>> +
>> +    mutex_lock(&core->lock);
>> +    if (core->removing)
>> +        ret = -ENODEV;
>> +    mutex_unlock(&core->lock);
>> +    if (ret)
>> +        return ret;
>>         ret = pm_runtime_resume_and_get(core->dev);
>>       if (ret < 0)
>>           return ret;
>>         mutex_lock(&core->lock);
>> +    if (core->removing) {
>> +        mutex_unlock(&core->lock);
>> +        ret = -ENODEV;
>> +        goto exit;
>> +    }
>> +
>
> This locking scheme is looks racy, what's the point of this granularity ?

The first check is an early reject when removal is already in progress.
The second re-validates the state after resume, since iris_remove() can
start while pm_runtime_resume_and_get() is in progress. Without the second
check, a command could be issued after teardown has started. Also, core->lock
cannot be held across the entire function because iris_resume() also acquires
core->lock.

>
> ---
> bod