Re: [PATCH v6 13/18] media: iris: update buffer requirements based on received info

[sashiko-bot@kernel.org]

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

New issues:
- [High] A Time-of-Check-to-Time-of-Use (TOCTOU) vulnerability during shared memory packet retrieval allows an attacker-controlled size to cause an out-of-bounds read of kernel memory.
- [Medium] Unchecked `num_properties > 1` in `iris_hfi_gen1_session_property_info` will cause parsing failures or state corruption.
- [Medium] Incorrect assignment of `req->count_actual` to `inst->fw_min_count` breaks minimum buffer calculations.
- [Low] Returning `-EINVAL` from enum type `enum iris_buffer_type` violates type safety.

Pre-existing issues:
- [High] Pre-existing race condition in firmware response handling leads to Use-After-Free of `inst` on firmware timeout.
--

--- Patch [13]: [PATCH v6 13/18] media: iris: update buffer requirements based on received info ---
commit 040bc4bd721ac68f0b5e1bd2726f98e4064f7387
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..896953ea62ea4 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
> @@ -533,6 +533,79 @@ static void iris_hfi_gen1_session_ftb_done(struct iris_inst *inst, void *packet)
>  	dev_err(core->dev, "error in ftb done\n");
>  }
>  
> +static enum iris_buffer_type iris_hfi_gen1_buf_type(struct iris_inst *inst, u32 type)
> +{
> +	switch (type) {
> +	case HFI_BUFFER_INPUT:

[ ... ]

> +	case HFI_BUFFER_INTERNAL_PERSIST:
> +		return BUF_ARP;
> +	default:
> +		return -EINVAL;

[Severity: Low]
Does returning -EINVAL violate type safety for the unsigned enum
iris_buffer_type?

Since the enum constants are typically represented as unsigned, this negative
value causes an implicit cast to a large unsigned integer.

> +	}
> +}
> +
> +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;
> +
> +		/* on relevant platforms hold_count and min_count are swapped */
> +		inst->buffers[type].min_count = req->hold_count;
> +		inst->buffers[type].size = req->size;
> +
> +		if (type == BUF_OUTPUT)
> +			inst->fw_min_count = req->count_actual;

[Severity: Medium]
The code tracks the strict minimum buffer count required by the hardware.
Since hold_count and count_min are swapped on these platforms, does
assigning req->count_actual here artificially inflate the minimum required
buffer count?

This seems to contradict logic elsewhere in the driver that relies on
count_min.

> +	}
> +}
> +
> +static void iris_hfi_gen1_session_property_info(struct iris_inst *inst, void *packet)
> +{
> +	struct hfi_msg_session_property_info_pkt *pkt = packet;
> +
> +	if (!pkt->num_properties) {
> +		dev_err(inst->core->dev, "error, no properties\n");
> +		goto out;
> +	}
> +
> +	switch (pkt->property) {
> +	case HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS:
> +		iris_hfi_gen1_session_buffer_requirements(inst, pkt->data,
> +							  pkt->shdr.hdr.size - sizeof(*pkt));

[Severity: Medium]
If the firmware sends an HFI_MSG_SESSION_PROPERTY_INFO packet with multiple
properties, does this code incorrectly assume the entire remaining payload
belongs to the first property?

Passing the full size could cause the loop in
iris_hfi_gen1_session_buffer_requirements() to misinterpret subsequent
property IDs and data as buffer constraints.

[Severity: High]
Could an out-of-bounds read occur if the firmware races to forge
pkt->shdr.hdr.size in shared memory?

If the size field is reread after the initial bounds check during memcpy,
trusting this size to calculate the payload length might cause the loop in
iris_hfi_gen1_session_buffer_requirements() to read beyond the bounds of
kernel memory.

> +		break;
> +	default:
> +		dev_warn(inst->core->dev, "unknown property id: %x\n", pkt->property);
> +	}
> +
> +out:
> +	complete(&inst->completion);
> +}
> +

[ ... ]

> @@ -657,7 +730,7 @@ static void iris_hfi_gen1_handle_response(struct iris_core *core, void *response
>  		} 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);
> +			iris_hfi_gen1_session_property_info(inst, hdr);
>  		} else {

[Severity: High]
This is a pre-existing issue, but can this lead to a Use-After-Free if a
video device close operation runs concurrently due to a firmware timeout?

If iris_get_instance() returns a raw pointer without incrementing a refcount:

iris_get_instance() {
    ...
    return inst;
}

And iris_close() concurrently removes and frees the memory:

iris_close() {
    ...
    iris_remove_session(inst);
    mutex_unlock(&inst->lock);
    ...
    kfree(inst);
}

Will iris_hfi_gen1_handle_response() subsequently acquire inst->lock on a
freed object before calling iris_hfi_gen1_session_property_info()?

>  			struct hfi_msg_session_hdr_pkt *shdr;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-iris-ar50lt-v6-0-374f0a46c23b@oss.qualcomm.com?part=13