Re: [PATCH v5 10/16] 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:
- [Critical] Use-After-Free due to missing reference counting in instance lookup.
- [High] Unsafe release of instance lock during firmware wait allows concurrent IOCTLs to clobber completion state.
--

commit d766418e0dabcbcbb8dc9a3f16b8a80a51ec61f4
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 firmware
to satisfy Gen1 firmware requirements for AR50Lt. It implements the
session_get_property operation and handles the resulting property info
messages.

> 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 the newly added function utilizes a pattern
that might be unsafe.

In iris_wait_for_session_response(), inst->lock is explicitly dropped while
waiting for inst->completion. Because the driver does not appear to use
vdev->lock to serialize V4L2 ioctls, dropping inst->lock could expose the
driver to concurrent ioctls.

Could a second thread acquire inst->lock, issue another firmware command,
and call reinit_completion(&inst->completion), erasing the state the first
thread is waiting on? 

If so, when the firmware finally responds, complete(&inst->completion) might
arbitrarily wake up the wrong thread and desynchronize the driver state.

> 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: Critical]
This isn't a bug introduced by this patch, but there appears to be a
critical structural flaw with how the instance is handled here.

Earlier in iris_hfi_gen1_handle_response(), iris_get_instance() looks up an
iris_inst from the core->instances list while holding core->lock, but returns
the raw pointer without incrementing any reference count, and then drops
core->lock.

The response handler then subsequently attempts to acquire
mutex_lock(&inst->lock). If a concurrent thread executing iris_close()
acquires core->lock via iris_remove_session(), removes the instance from
the list, and then calls kfree(inst), could the IRQ handler end up
executing a locking operation on freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616-iris-ar50lt-v5-0-583b42770b6a@oss.qualcomm.com?part=10