Re: [PATCH 14/17] media: rockchip: rga: put all cores into first core iommu domain

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Sharing the primary core's default IOMMU domain with secondary cores creates a severe lifetime mismatch leading to a Use-After-Free if the primary core is removed.
- [High] Incorrect error checking of `iommu_get_domain_for_dev()` using `IS_ERR()` causes a NULL pointer dereference and masks errors.
- [High] The new error paths for IOMMU attachment fail to release the PM runtime reference.
- [High] Missing `.unbind` callback leads to IOMMU domain reference leaks, teardown asymmetry, and array out-of-bounds writes upon driver rebind.
--

commit 3fbaee68845794dcb5e7c93168eef670817da976
Author: Sven Püschel <s.pueschel@pengutronix.de>

media: rockchip: rga: put all cores into first core iommu domain

Put all cores into the iommu domain of the first core to allow them to
be used by any core.

> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
> @@ -757,6 +758,19 @@ static int rga_core_bind(struct device *dev, struct device *master, void *data)
>  		  version.major, version.minor);
>  
>  	if (rga->num_cores) {
> +		/* Attach to the first cores iommu */
> +		struct iommu_domain *domain = iommu_get_domain_for_dev(rga->cores[0]->dev);
> +
> +		if (IS_ERR(domain)) {

[Severity: High]
Since iommu_get_domain_for_dev() returns NULL on failure (not an ERR_PTR),
won't this IS_ERR() check fail to catch a NULL domain?

If domain is NULL, it will be passed to iommu_attach_device(), which could
lead to a NULL pointer dereference when accessing domain->owner in
domain_iommu_ops_compatible().

> +			dev_err(core->dev, "Couldn't get domain of the first core\n");
> +			return PTR_ERR(domain);

[Severity: High]
Does this early return leak the PM runtime reference?

pm_runtime_resume_and_get(core->dev) is called earlier in this function, but
this return path doesn't call pm_runtime_put(core->dev).

> +		}
> +		ret = iommu_attach_device(domain, core->dev);

[Severity: Critical]
Does sharing the primary core's default IOMMU domain with secondary cores
create a lifetime mismatch?

The IOMMU subsystem couples the lifetime of a default domain strictly to its
original device group. If the platform device for the first core is removed,
the IOMMU core frees the default domain in iommu_deinit_device().

Secondary cores would still retain pointers to this freed domain, leading to a
use-after-free during DMA operations or unbinding.

[Severity: High]
Since we are attaching the device to the IOMMU domain here, how is it
detached when the driver is unbound?

There does not appear to be a corresponding .unbind callback in
rga_core_ops to call iommu_detach_device(). This would leave the IOMMU
group permanently attached, causing subsequent rebinds to fail with -EBUSY
or leak references.

Additionally, without an unbind callback to reset rga->num_cores, won't
re-binding the components cause rga->cores[rga->num_cores++] to continue
incrementing and overflow the array?

> +		if (ret) {
> +			dev_err(core->dev, "Couldn't attach to the domain of the first core\n");
> +			return ret;

[Severity: High]
Could this return path also leak the PM runtime reference acquired by
pm_runtime_resume_and_get(core->dev)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260606-spu-rga3multicore-v1-0-3ec2b15675f7@pengutronix.de?part=14