Re: [PATCH v7 14/18] media: iris: implement support for the Agatti platform

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Unconditional firmware query for buffer requirements during STREAMON overwrites driver-calculated sizes, breaking Gen1 decoders and causing DMA buffer overflows.

Pre-existing issues:
- [High] Completion crosstalk between a timed out `GET_PROPERTY` command and a subsequent `SESSION_STOP` command causes premature buffer freeing, leading to use-after-free.
- [Medium] The GOP_SIZE capability is missing the .hfi_id field in Gen1 encoder configurations, causing silent failures when setting the property.
--

--- Patch [14]: [PATCH v7 14/18] media: iris: implement support for the Agatti platform ---
commit 50a13c3b4c9f960eb99c4401684a9076ce008a44
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

media: iris: implement support for the Agatti platform

Port support for the AR50Lt video codec core (present for example on the
Agatti platform) to the Iris driver. Unlike more recent cores this
generation doesn't have the PIPE property (as it always has only one
pipe). Also, unlike newer platforms, buffer sizes are requested from the
firmware instead of being calculated by the driver.

> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1.c
> index ca1545d28b531..60bc1339ddd41 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1.c

[ ... ]

> +	{
> +		.cap_id = GOP_SIZE,
> +		.min = 0,
> +		.max = (1 << 16) - 1,
> +		.step_or_mask = 1,
> +		.value = 30,
> +		.set = iris_set_u32
> +	},

[Severity: Medium]
This is a pre-existing issue, but the GOP_SIZE capability is missing the
.hfi_id field here, meaning it defaults to 0.

Because of this, when iris_set_u32() invokes session_set_property() with
ID 0, the default case in iris_hfi_gen1_packet_session_set_property()
will return -EINVAL. The caller, iris_set_properties(), explicitly ignores
this return value:

    if (cap->cap_id && cap->set)
        cap->set(inst, i);

Since AR50Lt lacks the B_FRAME capability (which applies GOP size on other
platforms), does this completely prevent the GOP size from being configured
on the firmware?

[ ... ]

> 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 99e82e5510abe..c4baabbacefda 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> @@ -970,6 +970,11 @@ static int iris_hfi_gen1_set_bufsize(struct iris_inst *inst, u32 plane)
>  	struct hfi_buffer_size_actual bufsz;
>  	int ret;
>  
> +	ret = inst->hfi_session_ops->session_get_property(inst,
> +							  HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS);

[Severity: Critical]
Does this unconditional firmware query during STREAMON break existing Gen1
decoders and lead to DMA buffer overflows?

When the firmware responds to this property,
iris_hfi_gen1_session_buffer_requirements() blindly overwrites
inst->buffers[type].size with the firmware's required size:

    inst->buffers[type].size = req->size;

However, userspace allocated these V4L2 DMA buffers during REQBUFS based on
the mathematical size from iris_get_buffer_size(). During QBUF,
iris_vb2_buf_prepare() incorrectly validates against this smaller size instead
of the firmware's newly required size:

    if (vb->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE &&
        vb2_plane_size(vb, 0) < iris_get_buffer_size(inst, BUF_OUTPUT))
        return -EINVAL;

If the driver then passes the firmware's required size (via
HFI_PROPERTY_PARAM_BUFFER_SIZE_ACTUAL), could the firmware write past the end
of the physically allocated DMA memory?

Additionally, does applying this unconditional query to existing SM8250 decoders
force an unsupported state transition?

[Severity: High]
This isn't a bug introduced by this patch, but can this session_get_property()
call lead to a use-after-free if it times out?

iris_hfi_gen1_session_get_property() waits synchronously on inst->completion.
If it times out, the command is left pending in the firmware. Later, if
userspace stops the stream, iris_hfi_gen1_session_stop() reinitializes
inst->completion and waits for HFI_CMD_SESSION_STOP.

If the delayed HFI_MSG_SESSION_PROPERTY_INFO arrives during this wait,
iris_hfi_gen1_session_property_info() handles it and blindly calls:

    complete(&inst->completion);

Will this prematurely wake the SESSION_STOP wait, causing the driver to
mistakenly believe the session has stopped and free internal buffers while
the firmware is still accessing them?

> +	if (ret)
> +		return ret;
> +
>  	if (iris_split_mode_enabled(inst)) {
>  		bufsz.type = HFI_BUFFER_OUTPUT;
>  		bufsz.size = inst->core->iris_firmware_desc->get_vpu_buffer_size(inst, BUF_DPB);

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