linux-kernel - Re: [PATCH v1 1/2] iommu/tegra-smmu: Defer attachment of display clients

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 8 Apr 2021 14:40:26 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Joerg Roedel <joro@...tes.org>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Krishna Reddy <vdumpa@...dia.com>,
        Nicolin Chen <nicoleotsuka@...il.com>,
        Will Deacon <will@...nel.org>,
        iommu@...ts.linux-foundation.org, linux-tegra@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/2] iommu/tegra-smmu: Defer attachment of display
 clients

On Mon, Mar 29, 2021 at 02:32:55AM +0300, Dmitry Osipenko wrote:
> All consumer-grade Android and Chromebook devices show a splash screen
> on boot and then display is left enabled when kernel is booted. This
> behaviour is unacceptable in a case of implicit IOMMU domains to which
> devices are attached during kernel boot since devices, like display
> controller, may perform DMA at that time. We can work around this problem
> by deferring the enable of SMMU translation for a specific devices,
> like a display controller, until the first IOMMU mapping is created,
> which works good enough in practice because by that time h/w is already
> stopped.
> 
> Signed-off-by: Dmitry Osipenko <digetx@...il.com>
> ---
>  drivers/iommu/tegra-smmu.c | 71 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 71 insertions(+)

In general I do see why we would want to enable this. However, I think
this is a bad idea because it's going to proliferate the bad practice of
not describing things properly in device tree.

Whatever happened to the idea of creating identity mappings based on the
obscure tegra_fb_mem (or whatever it was called) command-line option? Is
that command-line not universally passed to the kernel from bootloaders
that initialize display?

That idealistic objection aside, this seems a bit over-engineered for
the hack that it is. See below.

> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 602aab98c079..af1e4b5adb27 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -60,6 +60,8 @@ struct tegra_smmu_as {
>  	dma_addr_t pd_dma;
>  	unsigned id;
>  	u32 attr;
> +	bool display_attached[2];
> +	bool attached_devices_need_sync;
>  };
>  
>  static struct tegra_smmu_as *to_smmu_as(struct iommu_domain *dom)
> @@ -78,6 +80,10 @@ static inline u32 smmu_readl(struct tegra_smmu *smmu, unsigned long offset)
>  	return readl(smmu->regs + offset);
>  }
>  
> +/* all Tegra SoCs use the same group IDs for displays */
> +#define SMMU_SWGROUP_DC		1
> +#define SMMU_SWGROUP_DCB	2
> +
>  #define SMMU_CONFIG 0x010
>  #define  SMMU_CONFIG_ENABLE (1 << 0)
>  
> @@ -253,6 +259,20 @@ static inline void smmu_flush(struct tegra_smmu *smmu)
>  	smmu_readl(smmu, SMMU_PTB_ASID);
>  }
>  
> +static int smmu_swgroup_to_display_id(unsigned int swgroup)
> +{
> +	switch (swgroup) {
> +	case SMMU_SWGROUP_DC:
> +		return 0;
> +
> +	case SMMU_SWGROUP_DCB:
> +		return 1;
> +
> +	default:
> +		return -1;
> +	}
> +}
> +

Why do we need to have this two-level mapping? Do we even need to care
about the specific swgroups IDs? Can we not just simply check at attach
time if the client that's being attached is a display client and then
set atteched_devices_need_sync = true?

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)