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

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

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 ?

---
bod