Re: [PATCH v7 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 (UAF) occurs in the HFI response handler when an instance is concurrently closed.
- [High] Concurrent V4L2 ioctls can race and corrupt `inst->completion` due to dropping `inst->lock` while waiting for firmware responses.
--

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

    media: iris: add minimal GET_PROPERTY implementation
    
    AR50Lt with the Gen1 firmware requires host to read
    HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS property, otherwise it doesn't
    update internal data and fails the HFI_CMD_SESSION_LOAD_RESOURCES
    command. Implement minimal support for querying the properties from the
    firmware. It is used by one of the following patches, adding support for
    Agatti.

> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> index 7674b47ad6c49..99e82e5510abe 100644
> --- 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
>  	return 0;
>  }
>  
> +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 is a pre-existing issue, but can concurrent ioctls on the same file
descriptor corrupt inst->completion here?

The driver implements manual ioctl serialization by acquiring inst->lock.
However, iris_wait_for_session_response() drops inst->lock while waiting:

drivers/media/platform/qcom/iris/iris_utils.c:iris_wait_for_session_response() {
    ...
	mutex_unlock(&inst->lock);
	ret = wait_for_completion_timeout(done, msecs_to_jiffies(HW_RESPONSE_TIMEOUT_VALUE));
	mutex_lock(&inst->lock);
    ...
}

Doesn't dropping the lock allow a concurrent ioctl to acquire inst->lock, issue
a new command, and call reinit_completion(&inst->completion) again, thereby
wiping out the completion state for the first thread?

> +}
> +

[ ... ]

> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
> index bfd7495bf44f0..23fc7194b1e3a 100644
> --- 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);

[Severity: High]
This isn't a bug introduced by this patch, but is there a use-after-free risk
when accessing inst here if the instance is concurrently closed?

The threaded IRQ handler iris_hfi_gen1_handle_response() retrieves the instance
via iris_get_instance(), which finds inst but drops core->lock without taking
a reference count:

drivers/media/platform/qcom/iris/iris_utils.c:iris_get_instance() {
    ...
			mutex_unlock(&core->lock);
			return inst;
    ...
}

If a user-space process closes the file descriptor concurrently, iris_close()
acquires inst->lock, calls iris_remove_session(), drops the lock, and frees inst:

drivers/media/platform/qcom/iris/iris_vidc.c:iris_close() {
    ...
	iris_remove_session(inst);
	mutex_unlock(&inst->lock);
	mutex_destroy(&inst->ctx_q_lock);
	mutex_destroy(&inst->lock);
	kfree(inst->fmt_src);
	kfree(inst->fmt_dst);
	kfree(inst);
}

When the IRQ handler iris_hfi_gen1_handle_response() resumes and attempts to
acquire inst->lock or call complete(&inst->completion), could it dereference
the freed inst pointer?

>  		} else {
>  			struct hfi_msg_session_hdr_pkt *shdr;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-iris-ar50lt-v7-0-76af9dd4d1f6@oss.qualcomm.com?part=12