Re: [PATCH v2 3/5] media: iris: Fix use-after-free on unbind with open sessions
From: Vishnu Reddy
Date: Tue Nov 10 2026 - 22:26:56 EST
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@xxxxxxxxxx>
>> Closes: https://lore.kernel.org/all/20260926064921.976721F000FF@xxxxxxxxxxxxxxx
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: Vishnu Reddy <busanna.reddy@xxxxxxxxxxxxxxxx>
>> ---
>> 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