Re: [PATCH v4 11/16] media: iris: update buffer requirements based on received info

[sashiko-bot@kernel.org]

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

New issues:
- [High] The driver incorrectly assigns `req->hold_count` and `req->count_actual` from `struct hfi_buffer_requirements` instead of `req->count_min` to instance buffer counts, leading to insufficient buffer allocations.

Pre-existing issues:
- [High] Preexisting Use-After-Free (UAF) race condition due to missing reference counting in `iris_get_instance`.
- [High] Preexisting Use-After-Free (UAF) of `inst->m2m_ctx` during session teardown in `iris_close()`.
--

commit 07ef5efeab4196fd6eaca945bfba49a95c55f702
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

media: iris: update buffer requirements based on received info

Upon receiving data for HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS the
driver should update buffer sizes and counts from the received data.
Implement corresponding functionality updating buffers data. This will
be used for upcoming support of AR50Lt platforms with Gen1 firmware.

> 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 23fc7194b1e3a..ee996eb1f41fa 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c

[ ... ]

> +static void iris_hfi_gen1_session_buffer_requirements(struct iris_inst *inst,
> +						      void *data, size_t size)
> +{
> +	struct hfi_buffer_requirements *req;
> +
> +	if (!size || size % sizeof(*req))
> +		return;
> +
> +	for (req = data; size; size -= sizeof(*req), req++) {
> +		enum iris_buffer_type type = iris_hfi_gen1_buf_type(inst, req->type);
> +
> +		if (type == -EINVAL)
> +			continue;
> +
> +		inst->buffers[type].min_count = req->hold_count;

[Severity: High]
Is this assignment correct? The existing firmware semantics appear to expect
req->count_min to represent the minimum required buffers for forward progress.

Will assigning req->hold_count (which is typically lower) cause the driver to
under-allocate buffers for V4L2, leading to firmware stalls?

> +		inst->buffers[type].size = req->size;
> +
> +		if (type == BUF_OUTPUT)
> +			inst->fw_min_count = req->count_actual;

[Severity: High]
Similarly, should this be assigned to req->count_min instead of
req->count_actual to maintain consistent minimum buffer count derivations?

> +	}
> +}

[ ... ]

> @@ -657,7 +729,7 @@ static void iris_hfi_gen1_handle_response(struct iris_core *core, void *response
>  		} else if (hdr->pkt_type == HFI_MSG_SESSION_EMPTY_BUFFER) {
>  			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);

[Severity: High]
This isn't a bug introduced by this patch, but can this lead to a
use-after-free regression?

During session teardown, iris_close() releases the M2M context before
shutting down the firmware session:

iris_vidc.c:iris_close() {
    ...
    v4l2_m2m_ctx_release(inst->m2m_ctx);
    ...
}

If an HFI_MSG_SESSION_FILL_BUFFER event arrives before the session is fully
closed, the threaded IRQ handler calls iris_hfi_gen1_session_ftb_done() which
subsequently accesses the freed context in v4l2_m2m_for_each_dst_buf_safe().

Is there a way to defer freeing the M2M context until the session is safely
stopped?

>  		} else if (hdr->pkt_type == HFI_MSG_SESSION_PROPERTY_INFO) {
> -			complete(&inst->completion);
> +			iris_hfi_gen1_session_property_info(inst, hdr);

[Severity: High]
This is a pre-existing issue, but is it safe for
iris_hfi_gen1_handle_response() to use the inst pointer here without a
reference count?

When iris_get_instance() looks up the instance, it returns the pointer without
taking a reference. If a userspace thread executing iris_close() frees the
instance:

iris_vidc.c:iris_close() {
    ...
    kfree(inst);
}

Could the threaded IRQ handler concurrently attempt to lock the destroyed
inst->lock or dereference inst while handling responses, resulting in a
use-after-free?

>  		} else {
>  			struct hfi_msg_session_hdr_pkt *shdr;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-iris-ar50lt-v4-0-0abfb74d5b3c@oss.qualcomm.com?part=11