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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The local stack variable `fw_fid` is passed by reference to `iris_vpu_bus_create_device()`, which stores its address in the persistent device's `platform_data` field, leading to a dangling pointer when the function returns.
- [High] The `core->fw_dev` pointer is not set to NULL after the context bank device is unregistered in `iris_glymur_deinit_cb_devs`, causing Use-After-Free and Double Free vulnerabilities.
--

commit ca3d38405759f1b57b41c987f0bd9782975ce278
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. This patch adds 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
[ ... ]
> +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 a pointer to the local stack variable fw_fid here?

Looking at iris_vpu_bus_create_device(), the pointer is assigned directly
to dev->platform_data without copying the underlying value:

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(), won't
dev->platform_data become a dangling pointer once this function returns?

If the IOMMU configuration is deferred and reprobed later via 
bus_iommu_probe(), the bus's dma_configure callback might dereference 
dev->platform_data and read stack garbage.

> +	if (IS_ERR(dev))
> +		return PTR_ERR(dev);
> +
> +	if (device_iommu_mapped(dev))
> +		core->fw_dev = dev;
> +	else
> +		device_unregister(dev);
> +
> +	return 0;
> +}
> +
> +void iris_glymur_deinit_cb_devs(struct iris_core *core)
> +{
> +	if (core->fw_dev)
> +		device_unregister(core->fw_dev);

[Severity: High]
Should core->fw_dev be set to NULL after unregistering the device?

If the driver's core initialization is invoked again and 
iris_glymur_init_cb_devs() is called, but device_iommu_mapped(dev) is false, 
the newly created device is unregistered without overwriting core->fw_dev.

This could leave a stale pointer in core->fw_dev. If the core initialization
sequence proceeds to load the firmware, could it use the stale core->fw_dev
pointer, or result in a double free if iris_glymur_deinit_cb_devs() is 
called again on the failure path?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610-glymur-v8-0-1c79b9d51fc0@oss.qualcomm.com?part=10