RE: [PATCH] thunderbolt: Stop using iommu_present()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[Public]



> -----Original Message-----
> From: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> Sent: Wednesday, March 16, 2022 07:45
> To: Robin Murphy <robin.murphy@xxxxxxx>
> Cc: andreas.noever@xxxxxxxxx; michael.jamet@xxxxxxxxx;
> YehezkelShB@xxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx; Limonciello,
> Mario <Mario.Limonciello@xxxxxxx>; hch@xxxxxx
> 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.
> >

Suggest you include a link to the discussion(s) that spurred this too in commit message.

> > Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
> > ---
> >
> > 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...

I did check that as a result of this:
* Turning IOMMU to pass through leads to iommu_dma_protection of 0
* Leaving IOMMU enabled leads to iommu_dma_protection of 1

I suspect you'll respin this on the below comment, but if you do keep it:
Tested-by: Mario Limonciello <mario.limonciello@xxxxxxx>

> >
> >  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".

I think below is what you are looking for (on top of your patch).  This checks the NHI, and then also checks all those siblings Mika referred to.

diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
index 5f5fc5f6a09b..b17961ba1396 100644
--- a/drivers/thunderbolt/domain.c
+++ b/drivers/thunderbolt/domain.c
@@ -259,12 +259,25 @@ static ssize_t iommu_dma_protection_show(struct device *dev,
 {
        struct tb *tb = container_of(dev, struct tb, dev);
        struct iommu_domain *iod = iommu_get_domain_for_dev(&tb->nhi->pdev->dev);
+       struct device_link *link;
+       bool protected;
+
        /*
         * Kernel DMA protection is a feature where Thunderbolt security is
         * handled natively using IOMMU. It is enabled when IOMMU is
         * enabled and actively enforcing translation.
         */
-       return sprintf(buf, "%d\n", iod && iod->type != IOMMU_DOMAIN_IDENTITY);
+       protected = iod && iod->type != IOMMU_DOMAIN_IDENTITY;
+       if (protected) {
+               list_for_each_entry(link, &tb->nhi->pdev->dev.links.consumers, s_node) {
+                       if (protected && pci_pcie_type(to_pci_dev(link->consumer)) == PCI_EXP_TYPE_ROOT_PORT) {
+                               iod = iommu_get_domain_for_dev(link->consumer);
+                               if (!iod || iod->type == IOMMU_DOMAIN_IDENTITY)
+                                       protected = false;
+                       }
+               }
+       }
+       return sprintf(buf, "%d\n", protected);
 }
 static DEVICE_ATTR_RO(iommu_dma_protection);




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux