Re: [PATCH 08/11] iommu/tegra-smmu: Defer display client translation to driver handover

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] iommu/tegra-smmu: type confusion in tegra_smmu_enable_translation() on ARM SMMU
--

commit 35f909bd1b8b88cf1f8f29602efafacdd3b66778
Author: Mikko Perttunen <mperttunen@nvidia.com>

iommu/tegra-smmu: Defer display client translation to driver handover

Bootloaders may leave display controllers scanning out a boot splash
while the kernel boots. Enabling translation for them without special
care during that time would cause translation faults. This patch
defers enabling translation until the display controller is reset.

> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 0b3fcc7011b35..2ae4da07bbc99 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
[ ... ]
> @@ -2751,6 +2752,13 @@ static int tegra_dc_init(struct host1x_client *client)
>  		return err;
>  	}
>  
> +	/*
> +	 * Enable translation now that the final domain is attached.
> +	 * The hardware was reset at probe and has not been programmed
> +	 * since.
> +	 */
> +	tegra_smmu_enable_translation(client->dev);

[Severity: Critical]
Does this code call the Tegra SMMU API unconditionally on all Tegra display
controllers? Newer SoC generations (Tegra186 and later) use the ARM SMMU
instead of the Tegra SMMU, which means this could pass a device managed by
the ARM SMMU driver into a Tegra SMMU specific function.

[ ... ]
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 67e7a7b925f0c..6636a044905f6 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
[ ... ]
> @@ -417,6 +423,49 @@ static void tegra_smmu_disable(struct tegra_smmu *smmu, unsigned int swgroup,
>  	}
>  }
>  
> +/**
> + * tegra_smmu_enable_translation - Release a client's swgroups to translation
> + * @dev: memory client device, already quiesced by its driver
> + *
> + * Enables translation for swgroups which were left in bypass at attach time
> + * because the client could still have been running from a pre-kernel
> + * configuration. Must be called after @dev has been attached to the domain it
> + * will use. Safe to call for any device; clients which were not deferred are
> + * unaffected.
> + */
> +void tegra_smmu_enable_translation(struct device *dev)
> +{
> +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> +	struct tegra_smmu *smmu = dev_iommu_priv_get(dev);

[Severity: Critical]
Is it safe to blindly retrieve the IOMMU private data via
dev_iommu_priv_get() and cast it to a struct tegra_smmu pointer without
validating that the device is actually managed by the Tegra SMMU driver?

On ARM SMMU systems, this private data actually points to a completely
different structure (like struct arm_smmu_master_cfg or
struct arm_smmu_master) that is significantly smaller.

> +	struct iommu_domain *domain;
> +	unsigned int i;
> +
> +	if (!fwspec || !smmu)
> +		return;
> +
> +	domain = iommu_get_domain_for_dev(dev);
> +	if (!domain || !(domain->type & __IOMMU_DOMAIN_PAGING))
> +		return;
> +
> +	mutex_lock(&smmu->lock);

[Severity: Critical]
Because lock is at a large offset in struct tegra_smmu, could dereferencing
this type-confused pointer to acquire the mutex access memory out of bounds
of the ARM SMMU configuration structure? This looks like it would lead to
heap corruption and a kernel oops during the display controller probe path.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915-b4-arm32-iommu-dma-cookie-v1-0-ba4f9d33a2b8@nvidia.com?part=8