linux-kernel - Re: [PATCH] thunderbolt: Stop using iommu

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:   Wed, 16 Mar 2022 14:45:11 +0200
From:   Mika Westerberg <mika.westerberg@...ux.intel.com>
To:     Robin Murphy <robin.murphy@....com>
Cc:     andreas.noever@...il.com, michael.jamet@...el.com,
        YehezkelShB@...il.com, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org, iommu@...ts.linux-foundation.org,
        mario.limonciello@....com, hch@....de
Subject: Re: [PATCH] thunderbolt: Stop using iommu_present()

Hi Robin,

On Wed, Mar 16, 2022 at 11:25:51AM +0000, Robin Murphy wrote:
> Even if an IOMMU might be present for some PCI segment in the system,
> that doesn't necessarily mean it provides translation for the device
> we care about. Furthermore, the presence or not of one firmware flag
> doesn't imply anything about the IOMMU driver's behaviour, which may
> still depend on other firmware properties and kernel options too. What
> actually matters is whether an IOMMU is enforcing protection for our
> device - regardless of whether that stemmed from firmware policy, kernel
> config, or user control - at the point we need to decide whether to
> authorise it. We can ascertain that generically by simply looking at
> whether we're currently attached to a translation domain or not.
> 
> Signed-off-by: Robin Murphy <robin.murphy@....com>
> ---
> 
> I don't have the means to test this, but I'm at least 80% confident
> in my unpicking of the structures to retrieve the correct device...
> 
>  drivers/thunderbolt/domain.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
> index 7018d959f775..5f5fc5f6a09b 100644
> --- a/drivers/thunderbolt/domain.c
> +++ b/drivers/thunderbolt/domain.c
> @@ -257,13 +257,14 @@ static ssize_t iommu_dma_protection_show(struct device *dev,
>  					 struct device_attribute *attr,
>  					 char *buf)
>  {
> +	struct tb *tb = container_of(dev, struct tb, dev);
> +	struct iommu_domain *iod = iommu_get_domain_for_dev(&tb->nhi->pdev->dev);

I wonder if this is the correct "domain"? I mean it's typically no the
Thunderbolt controller (here tb->nhi->pdev->dev) that needs the
protection (although in discrete controllers it does get it too) but
it's the tunneled PCIe topology that we need to check here.

For instance in Intel with intergrated Thunderbolt we have topology like
this:

  Host bridge
      |
      +--- Tunneled PCIe root port #1
      +--- Tunneled PCIe root port #2
      +--- Thunderbolt host controller (the NHI above)
      +--- xHCI

and In case of discrete controllers it looks like this:

  Host bridge
      |
      +--- PCIe root port #x
                |
                |
           PCIe switch upstream port
                |
	        +--- Tunneled PCIe switch downstream port #1
	        +--- Tunneled PCIe switch downstream port #2
        	+--- Thunderbolt host controller (the NHI above)
        	+--- xHCI

What we want is to make sure the Tunneled PCIe ports get the full IOMMU
protection. In case of the discrete above it is also fine if all the
devices behind the PCIe root port get the full IOMMU protection. Note in
the integrated all the devices are "siblings".