Re: [PATCH v7 01/12] media: iris: Add iris vpu bus support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The dynamic device's `platform_data` is initialized with a pointer to a short-lived stack variable, creating a persistent dangling pointer vulnerability.
- [High] A devm allocation tied to the persistent core device causes a memory leak on every firmware reload.
--

commit 5c9139b17fe941a07d06bbded6b06a8c47d21867
Author: Vikash Garodia <vikash.garodia@oss.qualcomm.com>

media: iris: Add iris vpu bus support

On glymur platform, firmware loading needs a separate IOMMU mapping with
its own stream ID. This stream ID is defined in the device tree with the
associated firmware function ID in the iommu-map property. To create this
mapping, a separate child device is needed so the firmware memory can be
isolated in its own IOMMU context.

Introduce a new bus called iris-vpu-bus. This creates a dynamic device,
and its dma_configure() callback calls of_dma_configure_id() with the
function ID provided by the client to map the corresponding stream ID.
This sets up a dedicated IOMMU context for the child device.

> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_bus.c b/drivers/media/platform/qcom/iris/iris_vpu_bus.c
> new file mode 100644
> index 0000000000000..e3327b227332a
> --- /dev/null
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_bus.c
> @@ -0,0 +1,61 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/iris_vpu_bus.h>
> +#include <linux/of_device.h>
> +#include <linux/slab.h>
> +
> +static int iris_vpu_bus_dma_configure(struct device *dev)
> +{
> +	const u32 *iommu_fid = (const u32 *)dev_get_platdata(dev);

[Severity: High]
Can this dereference a dangling pointer if the dma_configure callback executes
asynchronously?

The iommu_fid pointer is populated in iris_vpu_bus_create_device() directly
from the caller's argument, which in iris_glymur_init_cb_devs() points to a
local stack variable. If this callback runs later (e.g., deferred probing),
it will read invalid stack memory.

> +
> +	return of_dma_configure_id(dev, dev->parent->of_node, true, iommu_fid);
> +}
[ ... ]
> +struct device *iris_vpu_bus_create_device(struct device *parent_device, const char *name,
> +					  u64 dma_mask, const u32 *iommu_fid)
> +{
> +	struct device *dev;
> +	int ret;
> +
> +	dev = kzalloc_obj(*dev);
> +	if (!dev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	dev->release = iris_vpu_bus_release_device;
> +	dev->bus = &iris_vpu_bus_type;
> +	dev->parent = parent_device;
> +	dev->coherent_dma_mask = dma_mask;
> +	dev->dma_mask = &dev->coherent_dma_mask;
> +	dev->platform_data = (void *)iommu_fid;

[Severity: High]
Does this code store a dangling pointer if the caller passes a stack variable?

The caller iris_glymur_init_cb_devs() passes the address of a local stack
variable (fw_fid) for iommu_fid:

iris_glymur_init_cb_devs() {
    ...
    const u32 fw_fid = IOMMU_FID_IRIS_FIRMWARE;
    ...
    dev = iris_vpu_bus_create_device(core->dev, "iris-firmware", dma_mask, &fw_fid);
    ...
}

Because the device structure outlives the caller's stack frame,
dev->platform_data becomes a dangling pointer immediately after
iris_glymur_init_cb_devs() returns. Should the value be copied into the
device structure instead of storing the pointer?

> +	dev_set_name(dev, "%s", name);
> +
> +	ret = device_register(dev);
> +	if (ret) {
> +		put_device(dev);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return dev;
> +}

Also, regarding an issue newly introduced in this series by commit
85711dbb43a8 ("media: iris: Enable Secure PAS support with IOMMU managed by Linux"):

> diff --git a/drivers/media/platform/qcom/iris/iris_firmware.c b/drivers/media/platform/qcom/iris/iris_firmware.c
> index 1a476146d7580..8bdc9273036ce 100644
> --- a/drivers/media/platform/qcom/iris/iris_firmware.c
> +++ b/drivers/media/platform/qcom/iris/iris_firmware.c
[ ... ]
> @@ -30,14 +34,18 @@ static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
>  	if (strlen(fw_name) >= MAX_FIRMWARE_NAME_SIZE - 4)
>  		return -EINVAL;
>  
> -	ret = of_reserved_mem_region_to_resource(dev->of_node, 0, &res);
> +	ret = of_reserved_mem_region_to_resource(core->dev->of_node, 0, &res);
>  	if (ret)
>  		return ret;
>  
>  	mem_phys = res.start;
>  	res_size = resource_size(&res);
>  
> -	ret = request_firmware(&firmware, fw_name, dev);
> +	pas_ctx = devm_qcom_scm_pas_context_alloc(fw_dev, IRIS_PAS_ID, mem_phys, res_size);

[Severity: High]
Does this devm_ allocation cause a memory leak on every firmware reload?

On platforms that do not use a dedicated firmware context bank device
(where core->fw_dev is NULL), fw_dev falls back to the long-lived core->dev.

Because firmware loading can be repeatedly triggered upon firmware crashes
(via iris_sys_error_handler()), these devm_ allocations accumulate
indefinitely on core->dev's devres list without being explicitly freed
in iris_fw_unload() or iris_core_deinit().

Will repeated firmware reloads eventually exhaust system memory?

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