Re: [PATCH v8 12/18] media: iris: add minimal GET_PROPERTY implementation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Use-after-free race condition in `iris_hfi_gen1_handle_response` due to lack of instance refcounting.
- [High] Concurrent commands can corrupt `inst->completion` and falsely signal command completion due to lock dropping.
--

--- Patch [12]: [PATCH v8 12/18] media: iris: add minimal GET_PROPERTY implementation ---
commit 30243b6f486f6a7511eef1e8f6ae8cf07acc9093
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

media: iris: add minimal GET_PROPERTY implementation

This commit adds minimal support for querying properties from the Gen1
firmware in the iris driver, specifically implementing
HFI_CMD_SESSION_GET_PROPERTY. This is required for AR50Lt to read
HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS.

> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> @@ -1117,10 +1117,31 @@ static int iris_hfi_gen1_session_set_config_params(struct iris_inst *inst, u32 p

[ ... ]

> +static int iris_hfi_gen1_session_get_property(struct iris_inst *inst, u32 packet_type)
> +{
> +	struct hfi_session_get_property_pkt pkt;
> +	int ret;
> +
> +	pkt.shdr.hdr.size = sizeof(pkt);
> +	pkt.shdr.hdr.pkt_type = HFI_CMD_SESSION_GET_PROPERTY;
> +	pkt.shdr.session_id = inst->session_id;
> +	pkt.num_properties = 1;
> +	pkt.data = packet_type;
> +
> +	reinit_completion(&inst->completion);
> +
> +	ret = iris_hfi_queue_cmd_write(inst->core, &pkt, pkt.shdr.hdr.size);
> +	if (ret)
> +		return ret;
> +
> +	return iris_wait_for_session_response(inst, false);
> +}

[Severity: High]
This isn't a bug introduced by this patch, but does dropping the lock in
iris_wait_for_session_response() while waiting on a single shared completion
allow concurrent ioctls to corrupt the completion state?

A thread executing a V4L2 ioctl can lock inst->lock and issue a firmware
command. It then calls iris_wait_for_session_response(), which explicitly
unlocks inst->lock and sleeps on inst->completion:

iris_wait_for_session_response()
    mutex_unlock(&inst->lock);
    ret = wait_for_completion_timeout(done, ...);
    mutex_lock(&inst->lock);

While the first thread is waiting, another thread executing a concurrent
ioctl can acquire inst->lock, send a different firmware command, and call
reinit_completion(&inst->completion). Both threads end up sleeping on the
same completion.

When the firmware responds to the first command, the single completion is
triggered, which could incorrectly wake up the second thread prematurely.

Could this lead to corrupted driver state or commands assuming success before
the hardware has actually completed them?


> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
> @@ -652,6 +656,8 @@ static void iris_hfi_gen1_handle_response(struct iris_core *core, void *response
>  			iris_hfi_gen1_session_etb_done(inst, hdr);
>  		} else if (hdr->pkt_type == HFI_MSG_SESSION_FILL_BUFFER) {
>  			iris_hfi_gen1_session_ftb_done(inst, hdr);
> +		} else if (hdr->pkt_type == HFI_MSG_SESSION_PROPERTY_INFO) {
> +			complete(&inst->completion);
>  		} else {

[Severity: High]
This is a pre-existing issue, but is there a use-after-free race condition in
iris_hfi_gen1_handle_response() when dealing with inst?

When the hardware triggers an interrupt, the response handler retrieves the
instance pointer using iris_get_instance(). This function returns the pointer
without incrementing any reference count:

iris_get_instance()
    mutex_lock(&core->lock);
    [ ... ]
    return inst; 

If the response handler thread is preempted here, a concurrent close() on the
device file descriptor can execute iris_close(). This would lock inst->lock,
remove the instance from the core list, and free the instance memory.

When the response handler resumes, it attempts to lock the freed instance:

iris_hfi_gen1_handle_response()
    inst = iris_get_instance(...);
    [ ... ]
    mutex_lock(&inst->lock);

Can this be triggered by a user space process rapidly closing the file
descriptor while firmware responses are pending, leading to memory corruption
or a kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-iris-ar50lt-v8-0-6597c08cea6f@oss.qualcomm.com?part=12