Re: [PATCH v7 10/12] media: iris: Add platform data for glymur

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] `iris_glymur_deinit_cb_devs()` unregisters and frees `core->fw_dev` but fails to set the pointer to `NULL`, leaving a dangling pointer in the `core` structure.
- [High] The address of a local stack variable (`fw_fid`) is stored in a permanent device structure field (`dev->platform_data`), leading to a dangling pointer and Use-After-Free once the initialization function returns.
- [High] Silent cancellation of IOMMU deferred probe leading to improper fallback.
--

commit 22223f505726fa5c0ee432b64a8712aa1e89468b
Author: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>

media: iris: Add platform data for glymur

On glymur platform, the iris core shares most properties with the
iris core on the SM8550 platform. The major difference is that glymur
integrates two codec cores (vcodec0 and vcodec1), while SM8550 has only
one. Add glymur specific platform data, reusing SM8550 definitions
wherever applicable.

> diff --git a/drivers/media/platform/qcom/iris/iris_platform_glymur.c b/drivers/media/platform/qcom/iris/iris_platform_glymur.c
> new file mode 100644
> index 0000000000000..c42ac99870ede
> --- /dev/null
> +++ b/drivers/media/platform/qcom/iris/iris_platform_glymur.c
> @@ -0,0 +1,106 @@

[ ... ]

> +int iris_glymur_init_cb_devs(struct iris_core *core)
> +{
> +	u64 dma_mask = core->iris_platform_data->dma_mask;
> +	const u32 fw_fid = IOMMU_FID_IRIS_FIRMWARE;
> +	struct device *dev;
> +
> +	dev = iris_vpu_bus_create_device(core->dev, "iris-firmware", dma_mask, &fw_fid);

[Severity: High]
Is it safe to pass the address of a local stack variable to device creation?

Looking at iris_vpu_bus_create_device(), it assigns the passed pointer
directly to dev->platform_data:

iris_vpu_bus_create_device()
    ...
    dev->platform_data = (void *)iommu_fid;

Since fw_fid is allocated on the stack in iris_glymur_init_cb_devs(),
accessing dev->platform_data after this function returns might lead to a
use-after-free.

> +	if (IS_ERR(dev))
> +		return PTR_ERR(dev);
> +
> +	if (device_iommu_mapped(dev))
> +		core->fw_dev = dev;
> +	else
> +		device_unregister(dev);
> +
> +	return 0;

[Severity: High]
Is it intentional to return success (0) here when the IOMMU mapping fails?

If device_iommu_mapped(dev) is false, the device is unregistered but the
function still returns 0. Without an error being propagated, the firmware
loading logic might incorrectly fall back to using core->dev instead of the
required secure firmware domain. Could this lead to TrustZone load failures or
IOMMU translation faults?

> +}
> +
> +void iris_glymur_deinit_cb_devs(struct iris_core *core)
> +{
> +	if (core->fw_dev)
> +		device_unregister(core->fw_dev);

[Severity: Critical]
Could this unregistration leave a dangling pointer in core->fw_dev?

Since core->fw_dev is not set to NULL after calling device_unregister(),
subsequent initialization attempts might dereference the stale pointer. For
example, if a later call to iris_glymur_init_cb_devs() fails to map the IOMMU
and returns 0, core->fw_dev would remain unchanged. The core might then
proceed to iris_fw_load() and pass the stale pointer to
devm_qcom_scm_pas_context_alloc(), causing a use-after-free.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-glymur-v7-0-afaa55d11fe0@oss.qualcomm.com?part=10