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 <[email protected]> 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/[email protected]?part=8
