[PATCH] thunderbolt: Stop using iommu_present()

[PATCH] thunderbolt: Stop using iommu_present()

[Robin Murphy]

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@arm.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);
 	/*
 	 * Kernel DMA protection is a feature where Thunderbolt security is
 	 * handled natively using IOMMU. It is enabled when IOMMU is
-	 * enabled and ACPI DMAR table has DMAR_PLATFORM_OPT_IN set.
+	 * enabled and actively enforcing translation.
 	 */
-	return sprintf(buf, "%d\n",
-		       iommu_present(&pci_bus_type) && dmar_platform_optin());
+	return sprintf(buf, "%d\n", iod && iod->type != IOMMU_DOMAIN_IDENTITY);
 }
 static DEVICE_ATTR_RO(iommu_dma_protection);
 
-- 
2.28.0.dirty

[PATCH] thunderbolt: Stop using iommu_present()

[Robin Murphy]

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@arm.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);
 	/*
 	 * Kernel DMA protection is a feature where Thunderbolt security is
 	 * handled natively using IOMMU. It is enabled when IOMMU is
-	 * enabled and ACPI DMAR table has DMAR_PLATFORM_OPT_IN set.
+	 * enabled and actively enforcing translation.
 	 */
-	return sprintf(buf, "%d\n",
-		       iommu_present(&pci_bus_type) && dmar_platform_optin());
+	return sprintf(buf, "%d\n", iod && iod->type != IOMMU_DOMAIN_IDENTITY);
 }
 static DEVICE_ATTR_RO(iommu_dma_protection);
 
-- 
2.28.0.dirty

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Mika Westerberg]

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

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

[Mika Westerberg]

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@arm.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".
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Robin Murphy]

On 2022-03-16 12:45, Mika Westerberg wrote:
> 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@arm.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".

Ah, OK, I wasn't aware that the NHI isn't even the right thing in the 
first place :(

Is there an easy way to get from the struct tb to a PCI device 
representing the end of its relevant tunnel, or do we have a circular 
dependency problem where the latter won't appear until we've authorised 
it (and thus the IOMMU layer won't know about it yet either)?

Thanks,
Robin.

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

[Robin Murphy]

On 2022-03-16 12:45, Mika Westerberg wrote:
> 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@arm.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".

Ah, OK, I wasn't aware that the NHI isn't even the right thing in the 
first place :(

Is there an easy way to get from the struct tb to a PCI device 
representing the end of its relevant tunnel, or do we have a circular 
dependency problem where the latter won't appear until we've authorised 
it (and thus the IOMMU layer won't know about it yet either)?

Thanks,
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Mika Westerberg]

Hi,

On Wed, Mar 16, 2022 at 02:49:09PM +0000, Robin Murphy wrote:
> > 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".
> 
> Ah, OK, I wasn't aware that the NHI isn't even the right thing in the first
> place :(
> 
> Is there an easy way to get from the struct tb to a PCI device representing
> the end of its relevant tunnel, or do we have a circular dependency problem
> where the latter won't appear until we've authorised it (and thus the IOMMU
> layer won't know about it yet either)?

The PCIe root ports (and the PCIe downstream ports) are there already
even without "authorization".

There is a way to figure out the "tunneled" PCIe ports by looking at
certain properties and we do that already actually. The BIOS has the
following under these ports:

https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-externally-exposed-pcie-root-ports

and the ports will have dev->external_facing set to 1. Perhaps looking
at that field helps here?

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

[Mika Westerberg]

Hi,

On Wed, Mar 16, 2022 at 02:49:09PM +0000, Robin Murphy wrote:
> > 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".
> 
> Ah, OK, I wasn't aware that the NHI isn't even the right thing in the first
> place :(
> 
> Is there an easy way to get from the struct tb to a PCI device representing
> the end of its relevant tunnel, or do we have a circular dependency problem
> where the latter won't appear until we've authorised it (and thus the IOMMU
> layer won't know about it yet either)?

The PCIe root ports (and the PCIe downstream ports) are there already
even without "authorization".

There is a way to figure out the "tunneled" PCIe ports by looking at
certain properties and we do that already actually. The BIOS has the
following under these ports:

https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-externally-exposed-pcie-root-ports

and the ports will have dev->external_facing set to 1. Perhaps looking
at that field helps here?
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Limonciello, Mario]

[Public]

> On Wed, Mar 16, 2022 at 02:49:09PM +0000, Robin Murphy wrote:
> > > 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".
> >
> > Ah, OK, I wasn't aware that the NHI isn't even the right thing in the first
> > place :(
> >
> > Is there an easy way to get from the struct tb to a PCI device representing
> > the end of its relevant tunnel, or do we have a circular dependency
> problem
> > where the latter won't appear until we've authorised it (and thus the
> IOMMU
> > layer won't know about it yet either)?
> 
> The PCIe root ports (and the PCIe downstream ports) are there already
> even without "authorization".
> 
> There is a way to figure out the "tunneled" PCIe ports by looking at
> certain properties and we do that already actually. The BIOS has the
> following under these ports:
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs
> .microsoft.com%2Fen-us%2Fwindows-hardware%2Fdrivers%2Fpci%2Fdsd-
> for-pcie-root-ports%23identifying-externally-exposed-pcie-root-
> ports&data=04%7C01%7Cmario.limonciello%40amd.com%7C0465d319a
> 6684335d9c208da07710e7c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7
> C0%7C637830479402895833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
> LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&am
> p;sdata=z6hpYGpj%2B%2BVvz9d6MXiO4N66PUm4zwhOdI%2Br6l3PjhQ%3D
> &reserved=0
> 
> and the ports will have dev->external_facing set to 1. Perhaps looking
> at that field helps here?

External facing isn't a guarantee from the firmware though.  It's something we
all expect in practice, but I think it's better to look at the ones that are from
the _DSD usb4-host-interface to be safer.

Mika, you might not have seen it yet, but I sent a follow up diff in this thread
to Robin's patch.  If that looks good Robin can submit a v2 (or I'm happy to do
so as well as I confirmed it helps my original intent too).

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

[Limonciello, Mario via iommu]

[Public]

> On Wed, Mar 16, 2022 at 02:49:09PM +0000, Robin Murphy wrote:
> > > 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".
> >
> > Ah, OK, I wasn't aware that the NHI isn't even the right thing in the first
> > place :(
> >
> > Is there an easy way to get from the struct tb to a PCI device representing
> > the end of its relevant tunnel, or do we have a circular dependency
> problem
> > where the latter won't appear until we've authorised it (and thus the
> IOMMU
> > layer won't know about it yet either)?
> 
> The PCIe root ports (and the PCIe downstream ports) are there already
> even without "authorization".
> 
> There is a way to figure out the "tunneled" PCIe ports by looking at
> certain properties and we do that already actually. The BIOS has the
> following under these ports:
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs
> .microsoft.com%2Fen-us%2Fwindows-hardware%2Fdrivers%2Fpci%2Fdsd-
> for-pcie-root-ports%23identifying-externally-exposed-pcie-root-
> ports&data=04%7C01%7Cmario.limonciello%40amd.com%7C0465d319a
> 6684335d9c208da07710e7c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7
> C0%7C637830479402895833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
> LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&am
> p;sdata=z6hpYGpj%2B%2BVvz9d6MXiO4N66PUm4zwhOdI%2Br6l3PjhQ%3D
> &reserved=0
> 
> and the ports will have dev->external_facing set to 1. Perhaps looking
> at that field helps here?

External facing isn't a guarantee from the firmware though.  It's something we
all expect in practice, but I think it's better to look at the ones that are from
the _DSD usb4-host-interface to be safer.

Mika, you might not have seen it yet, but I sent a follow up diff in this thread
to Robin's patch.  If that looks good Robin can submit a v2 (or I'm happy to do
so as well as I confirmed it helps my original intent too).
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Mika Westerberg]

Hi Mario,

On Wed, Mar 16, 2022 at 05:24:38PM +0000, Limonciello, Mario wrote:
> [Public]
> 
> > On Wed, Mar 16, 2022 at 02:49:09PM +0000, Robin Murphy wrote:
> > > > 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".
> > >
> > > Ah, OK, I wasn't aware that the NHI isn't even the right thing in the first
> > > place :(
> > >
> > > Is there an easy way to get from the struct tb to a PCI device representing
> > > the end of its relevant tunnel, or do we have a circular dependency
> > problem
> > > where the latter won't appear until we've authorised it (and thus the
> > IOMMU
> > > layer won't know about it yet either)?
> > 
> > The PCIe root ports (and the PCIe downstream ports) are there already
> > even without "authorization".
> > 
> > There is a way to figure out the "tunneled" PCIe ports by looking at
> > certain properties and we do that already actually. The BIOS has the
> > following under these ports:
> > 
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs
> > .microsoft.com%2Fen-us%2Fwindows-hardware%2Fdrivers%2Fpci%2Fdsd-
> > for-pcie-root-ports%23identifying-externally-exposed-pcie-root-
> > ports&data=04%7C01%7Cmario.limonciello%40amd.com%7C0465d319a
> > 6684335d9c208da07710e7c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7
> > C0%7C637830479402895833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
> > LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&am
> > p;sdata=z6hpYGpj%2B%2BVvz9d6MXiO4N66PUm4zwhOdI%2Br6l3PjhQ%3D
> > &reserved=0
> > 
> > and the ports will have dev->external_facing set to 1. Perhaps looking
> > at that field helps here?
> 
> External facing isn't a guarantee from the firmware though.  It's something we
> all expect in practice, but I think it's better to look at the ones that are from
> the _DSD usb4-host-interface to be safer.

Right but then we have the discrete ones with the DVSEC that exposes the
tunneled ports :(

> Mika, you might not have seen it yet, but I sent a follow up diff in this thread
> to Robin's patch.  If that looks good Robin can submit a v2 (or I'm happy to do
> so as well as I confirmed it helps my original intent too).

I saw it now and I'm thinking are we making this unnecessary complex? I
mean Microsoft solely depends on the DMAR platform opt-in flag:

  https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt

We also do turn on full IOMMU mappings in that case for devices that are
marked as external facing by the same firmware that provided the DMAR
bit. If the user decides to disable IOMMU from command line for instance
then we expect she knows what she is doing.

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

[Mika Westerberg]

Hi Mario,

On Wed, Mar 16, 2022 at 05:24:38PM +0000, Limonciello, Mario wrote:
> [Public]
> 
> > On Wed, Mar 16, 2022 at 02:49:09PM +0000, Robin Murphy wrote:
> > > > 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".
> > >
> > > Ah, OK, I wasn't aware that the NHI isn't even the right thing in the first
> > > place :(
> > >
> > > Is there an easy way to get from the struct tb to a PCI device representing
> > > the end of its relevant tunnel, or do we have a circular dependency
> > problem
> > > where the latter won't appear until we've authorised it (and thus the
> > IOMMU
> > > layer won't know about it yet either)?
> > 
> > The PCIe root ports (and the PCIe downstream ports) are there already
> > even without "authorization".
> > 
> > There is a way to figure out the "tunneled" PCIe ports by looking at
> > certain properties and we do that already actually. The BIOS has the
> > following under these ports:
> > 
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs
> > .microsoft.com%2Fen-us%2Fwindows-hardware%2Fdrivers%2Fpci%2Fdsd-
> > for-pcie-root-ports%23identifying-externally-exposed-pcie-root-
> > ports&data=04%7C01%7Cmario.limonciello%40amd.com%7C0465d319a
> > 6684335d9c208da07710e7c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7
> > C0%7C637830479402895833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
> > LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&am
> > p;sdata=z6hpYGpj%2B%2BVvz9d6MXiO4N66PUm4zwhOdI%2Br6l3PjhQ%3D
> > &reserved=0
> > 
> > and the ports will have dev->external_facing set to 1. Perhaps looking
> > at that field helps here?
> 
> External facing isn't a guarantee from the firmware though.  It's something we
> all expect in practice, but I think it's better to look at the ones that are from
> the _DSD usb4-host-interface to be safer.

Right but then we have the discrete ones with the DVSEC that exposes the
tunneled ports :(

> Mika, you might not have seen it yet, but I sent a follow up diff in this thread
> to Robin's patch.  If that looks good Robin can submit a v2 (or I'm happy to do
> so as well as I confirmed it helps my original intent too).

I saw it now and I'm thinking are we making this unnecessary complex? I
mean Microsoft solely depends on the DMAR platform opt-in flag:

  https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt

We also do turn on full IOMMU mappings in that case for devices that are
marked as external facing by the same firmware that provided the DMAR
bit. If the user decides to disable IOMMU from command line for instance
then we expect she knows what she is doing.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Robin Murphy]

On 2022-03-16 17:37, Mika Westerberg wrote:
> Hi Mario,
> 
> On Wed, Mar 16, 2022 at 05:24:38PM +0000, Limonciello, Mario wrote:
>> [Public]
>>
>>> On Wed, Mar 16, 2022 at 02:49:09PM +0000, Robin Murphy wrote:
>>>>> 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".
>>>>
>>>> Ah, OK, I wasn't aware that the NHI isn't even the right thing in the first
>>>> place :(
>>>>
>>>> Is there an easy way to get from the struct tb to a PCI device representing
>>>> the end of its relevant tunnel, or do we have a circular dependency
>>> problem
>>>> where the latter won't appear until we've authorised it (and thus the
>>> IOMMU
>>>> layer won't know about it yet either)?
>>>
>>> The PCIe root ports (and the PCIe downstream ports) are there already
>>> even without "authorization".
>>>
>>> There is a way to figure out the "tunneled" PCIe ports by looking at
>>> certain properties and we do that already actually. The BIOS has the
>>> following under these ports:
>>>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs
>>> .microsoft.com%2Fen-us%2Fwindows-hardware%2Fdrivers%2Fpci%2Fdsd-
>>> for-pcie-root-ports%23identifying-externally-exposed-pcie-root-
>>> ports&data=04%7C01%7Cmario.limonciello%40amd.com%7C0465d319a
>>> 6684335d9c208da07710e7c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7
>>> C0%7C637830479402895833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
>>> LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&am
>>> p;sdata=z6hpYGpj%2B%2BVvz9d6MXiO4N66PUm4zwhOdI%2Br6l3PjhQ%3D
>>> &reserved=0
>>>
>>> and the ports will have dev->external_facing set to 1. Perhaps looking
>>> at that field helps here?
>>
>> External facing isn't a guarantee from the firmware though.  It's something we
>> all expect in practice, but I think it's better to look at the ones that are from
>> the _DSD usb4-host-interface to be safer.
> 
> Right but then we have the discrete ones with the DVSEC that exposes the
> tunneled ports :(
> 
>> Mika, you might not have seen it yet, but I sent a follow up diff in this thread
>> to Robin's patch.  If that looks good Robin can submit a v2 (or I'm happy to do
>> so as well as I confirmed it helps my original intent too).
> 
> I saw it now and I'm thinking are we making this unnecessary complex? I
> mean Microsoft solely depends on the DMAR platform opt-in flag:
> 
>    https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt
> 
> We also do turn on full IOMMU mappings in that case for devices that are
> marked as external facing by the same firmware that provided the DMAR
> bit. If the user decides to disable IOMMU from command line for instance
> then we expect she knows what she is doing.

Yeah, if external_facing is set correctly then we can safely expect the 
the IOMMU layer to do the right thing, so in that case it probably is OK 
to infer that if an IOMMU is present for the NHI then it'll be managing 
that whole bus hierarchy. What I'm really thinking about here is whether 
we can defend against a case when external_facing *isn't* set, so we 
treat the tunnelled ports as normal PCI buses, assume it's OK since 
we've got an IOMMU and everything else is getting translation domains by 
default, but then a Thunderbolt device shows up masquerading the VID:DID 
of something that gets a passthrough quirk, and thus tricks its way 
through the perceived protection.

Robin.

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

[Robin Murphy]

On 2022-03-16 17:37, Mika Westerberg wrote:
> Hi Mario,
> 
> On Wed, Mar 16, 2022 at 05:24:38PM +0000, Limonciello, Mario wrote:
>> [Public]
>>
>>> On Wed, Mar 16, 2022 at 02:49:09PM +0000, Robin Murphy wrote:
>>>>> 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".
>>>>
>>>> Ah, OK, I wasn't aware that the NHI isn't even the right thing in the first
>>>> place :(
>>>>
>>>> Is there an easy way to get from the struct tb to a PCI device representing
>>>> the end of its relevant tunnel, or do we have a circular dependency
>>> problem
>>>> where the latter won't appear until we've authorised it (and thus the
>>> IOMMU
>>>> layer won't know about it yet either)?
>>>
>>> The PCIe root ports (and the PCIe downstream ports) are there already
>>> even without "authorization".
>>>
>>> There is a way to figure out the "tunneled" PCIe ports by looking at
>>> certain properties and we do that already actually. The BIOS has the
>>> following under these ports:
>>>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs
>>> .microsoft.com%2Fen-us%2Fwindows-hardware%2Fdrivers%2Fpci%2Fdsd-
>>> for-pcie-root-ports%23identifying-externally-exposed-pcie-root-
>>> ports&data=04%7C01%7Cmario.limonciello%40amd.com%7C0465d319a
>>> 6684335d9c208da07710e7c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7
>>> C0%7C637830479402895833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
>>> LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&am
>>> p;sdata=z6hpYGpj%2B%2BVvz9d6MXiO4N66PUm4zwhOdI%2Br6l3PjhQ%3D
>>> &reserved=0
>>>
>>> and the ports will have dev->external_facing set to 1. Perhaps looking
>>> at that field helps here?
>>
>> External facing isn't a guarantee from the firmware though.  It's something we
>> all expect in practice, but I think it's better to look at the ones that are from
>> the _DSD usb4-host-interface to be safer.
> 
> Right but then we have the discrete ones with the DVSEC that exposes the
> tunneled ports :(
> 
>> Mika, you might not have seen it yet, but I sent a follow up diff in this thread
>> to Robin's patch.  If that looks good Robin can submit a v2 (or I'm happy to do
>> so as well as I confirmed it helps my original intent too).
> 
> I saw it now and I'm thinking are we making this unnecessary complex? I
> mean Microsoft solely depends on the DMAR platform opt-in flag:
> 
>    https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt
> 
> We also do turn on full IOMMU mappings in that case for devices that are
> marked as external facing by the same firmware that provided the DMAR
> bit. If the user decides to disable IOMMU from command line for instance
> then we expect she knows what she is doing.

Yeah, if external_facing is set correctly then we can safely expect the 
the IOMMU layer to do the right thing, so in that case it probably is OK 
to infer that if an IOMMU is present for the NHI then it'll be managing 
that whole bus hierarchy. What I'm really thinking about here is whether 
we can defend against a case when external_facing *isn't* set, so we 
treat the tunnelled ports as normal PCI buses, assume it's OK since 
we've got an IOMMU and everything else is getting translation domains by 
default, but then a Thunderbolt device shows up masquerading the VID:DID 
of something that gets a passthrough quirk, and thus tricks its way 
through the perceived protection.

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Limonciello, Mario]

[Public]

> >>>
> >>> There is a way to figure out the "tunneled" PCIe ports by looking at
> >>> certain properties and we do that already actually. The BIOS has the
> >>> following under these ports:
> >>>
> >>>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs
> >>> .microsoft.com%2Fen-us%2Fwindows-
> hardware%2Fdrivers%2Fpci%2Fdsd-
> >>> for-pcie-root-ports%23identifying-externally-exposed-pcie-root-
> >>>
> ports&data=04%7C01%7Cmario.limonciello%40amd.com%7C0465d319a
> >>>
> 6684335d9c208da07710e7c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7
> >>>
> C0%7C637830479402895833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
> >>>
> LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&am
> >>>
> p;sdata=z6hpYGpj%2B%2BVvz9d6MXiO4N66PUm4zwhOdI%2Br6l3PjhQ%3D
> >>> &reserved=0
> >>>
> >>> and the ports will have dev->external_facing set to 1. Perhaps looking
> >>> at that field helps here?
> >>
> >> External facing isn't a guarantee from the firmware though.  It's
> something we
> >> all expect in practice, but I think it's better to look at the ones that are
> from
> >> the _DSD usb4-host-interface to be safer.
> >
> > Right but then we have the discrete ones with the DVSEC that exposes the
> > tunneled ports :(
> >

Can the USB4 CM make the device links in the DVSEC case perhaps too?  I would
think we want that anyway to control device suspend ordering.

If I had something discrete to try I'd dust off the DVSEC patch I wrote before to
try it, but alas all I have is integrated stuff on my hand.

> >> Mika, you might not have seen it yet, but I sent a follow up diff in this
> thread
> >> to Robin's patch.  If that looks good Robin can submit a v2 (or I'm happy to
> do
> >> so as well as I confirmed it helps my original intent too).
> >
> > I saw it now and I'm thinking are we making this unnecessary complex? I
> > mean Microsoft solely depends on the DMAR platform opt-in flag:
> >
> >
> 

I think Microsoft doesn't allow you to turn off the IOMMU though or put it in
passthrough through on the kernel command line.

> > We also do turn on full IOMMU mappings in that case for devices that are
> > marked as external facing by the same firmware that provided the DMAR
> > bit. If the user decides to disable IOMMU from command line for instance
> > then we expect she knows what she is doing.
> 
> Yeah, if external_facing is set correctly then we can safely expect the
> the IOMMU layer to do the right thing, so in that case it probably is OK
> to infer that if an IOMMU is present for the NHI then it'll be managing
> that whole bus hierarchy. What I'm really thinking about here is whether
> we can defend against a case when external_facing *isn't* set, so we
> treat the tunnelled ports as normal PCI buses, assume it's OK since
> we've got an IOMMU and everything else is getting translation domains by
> default, but then a Thunderbolt device shows up masquerading the VID:DID
> of something that gets a passthrough quirk, and thus tricks its way
> through the perceived protection.
> 
> Robin.

Unless it happened after 5.17-rc8 looking at the code I think that's Intel
specific behavior though at the moment (has_external_pci).  I don't see it
in a generic layer.

In addition to the point Robin said about firmware not setting external facing
if the IOMMU was disabled on command line then iommu_dma_protection
would be showing the wrong values meaning userspace may choose to
authorize the device automatically in a potentially unsafe scenario.

Even if the user "knew what they were doing", I would expect that we still
do our best to protect them from themselves and not advertise something
that will cause automatic authorization.

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

[Limonciello, Mario via iommu]

[Public]

> >>>
> >>> There is a way to figure out the "tunneled" PCIe ports by looking at
> >>> certain properties and we do that already actually. The BIOS has the
> >>> following under these ports:
> >>>
> >>>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs
> >>> .microsoft.com%2Fen-us%2Fwindows-
> hardware%2Fdrivers%2Fpci%2Fdsd-
> >>> for-pcie-root-ports%23identifying-externally-exposed-pcie-root-
> >>>
> ports&data=04%7C01%7Cmario.limonciello%40amd.com%7C0465d319a
> >>>
> 6684335d9c208da07710e7c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7
> >>>
> C0%7C637830479402895833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
> >>>
> LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&am
> >>>
> p;sdata=z6hpYGpj%2B%2BVvz9d6MXiO4N66PUm4zwhOdI%2Br6l3PjhQ%3D
> >>> &reserved=0
> >>>
> >>> and the ports will have dev->external_facing set to 1. Perhaps looking
> >>> at that field helps here?
> >>
> >> External facing isn't a guarantee from the firmware though.  It's
> something we
> >> all expect in practice, but I think it's better to look at the ones that are
> from
> >> the _DSD usb4-host-interface to be safer.
> >
> > Right but then we have the discrete ones with the DVSEC that exposes the
> > tunneled ports :(
> >

Can the USB4 CM make the device links in the DVSEC case perhaps too?  I would
think we want that anyway to control device suspend ordering.

If I had something discrete to try I'd dust off the DVSEC patch I wrote before to
try it, but alas all I have is integrated stuff on my hand.

> >> Mika, you might not have seen it yet, but I sent a follow up diff in this
> thread
> >> to Robin's patch.  If that looks good Robin can submit a v2 (or I'm happy to
> do
> >> so as well as I confirmed it helps my original intent too).
> >
> > I saw it now and I'm thinking are we making this unnecessary complex? I
> > mean Microsoft solely depends on the DMAR platform opt-in flag:
> >
> >
> 

I think Microsoft doesn't allow you to turn off the IOMMU though or put it in
passthrough through on the kernel command line.

> > We also do turn on full IOMMU mappings in that case for devices that are
> > marked as external facing by the same firmware that provided the DMAR
> > bit. If the user decides to disable IOMMU from command line for instance
> > then we expect she knows what she is doing.
> 
> Yeah, if external_facing is set correctly then we can safely expect the
> the IOMMU layer to do the right thing, so in that case it probably is OK
> to infer that if an IOMMU is present for the NHI then it'll be managing
> that whole bus hierarchy. What I'm really thinking about here is whether
> we can defend against a case when external_facing *isn't* set, so we
> treat the tunnelled ports as normal PCI buses, assume it's OK since
> we've got an IOMMU and everything else is getting translation domains by
> default, but then a Thunderbolt device shows up masquerading the VID:DID
> of something that gets a passthrough quirk, and thus tricks its way
> through the perceived protection.
> 
> Robin.

Unless it happened after 5.17-rc8 looking at the code I think that's Intel
specific behavior though at the moment (has_external_pci).  I don't see it
in a generic layer.

In addition to the point Robin said about firmware not setting external facing
if the IOMMU was disabled on command line then iommu_dma_protection
would be showing the wrong values meaning userspace may choose to
authorize the device automatically in a potentially unsafe scenario.

Even if the user "knew what they were doing", I would expect that we still
do our best to protect them from themselves and not advertise something
that will cause automatic authorization.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Limonciello, Mario
> Sent: Wednesday, March 16, 2022 12:54
> To: Robin Murphy <robin.murphy@arm.com>; Mika Westerberg
> <mika.westerberg@linux.intel.com>
> Cc: michael.jamet@intel.com; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org; YehezkelShB@gmail.com; iommu@lists.linux-
> foundation.org; andreas.noever@gmail.com; hch@lst.de
> Subject: RE: [PATCH] thunderbolt: Stop using iommu_present()
> 
> [Public]
> 
> > >>>
> > >>> There is a way to figure out the "tunneled" PCIe ports by looking at
> > >>> certain properties and we do that already actually. The BIOS has the
> > >>> following under these ports:
> > >>>
> > >>>
> >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs
> > >>> .microsoft.com%2Fen-us%2Fwindows-
> > hardware%2Fdrivers%2Fpci%2Fdsd-
> > >>> for-pcie-root-ports%23identifying-externally-exposed-pcie-root-
> > >>>
> >
> ports&amp;data=04%7C01%7Cmario.limonciello%40amd.com%7C0465d319a
> > >>>
> >
> 6684335d9c208da07710e7c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7
> > >>>
> >
> C0%7C637830479402895833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
> > >>>
> >
> LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&am
> > >>>
> >
> p;sdata=z6hpYGpj%2B%2BVvz9d6MXiO4N66PUm4zwhOdI%2Br6l3PjhQ%3D
> > >>> &amp;reserved=0
> > >>>
> > >>> and the ports will have dev->external_facing set to 1. Perhaps looking
> > >>> at that field helps here?
> > >>
> > >> External facing isn't a guarantee from the firmware though.  It's
> > something we
> > >> all expect in practice, but I think it's better to look at the ones that are
> > from
> > >> the _DSD usb4-host-interface to be safer.
> > >
> > > Right but then we have the discrete ones with the DVSEC that exposes
> the
> > > tunneled ports :(
> > >
> 
> Can the USB4 CM make the device links in the DVSEC case perhaps too?  I
> would
> think we want that anyway to control device suspend ordering.
> 
> If I had something discrete to try I'd dust off the DVSEC patch I wrote before
> to
> try it, but alas all I have is integrated stuff on my hand.
> 
> > >> Mika, you might not have seen it yet, but I sent a follow up diff in this
> > thread
> > >> to Robin's patch.  If that looks good Robin can submit a v2 (or I'm happy
> to
> > do
> > >> so as well as I confirmed it helps my original intent too).
> > >
> > > I saw it now and I'm thinking are we making this unnecessary complex? I
> > > mean Microsoft solely depends on the DMAR platform opt-in flag:
> > >
> > >
> >
> 
> I think Microsoft doesn't allow you to turn off the IOMMU though or put it in
> passthrough through on the kernel command line.
> 
> > > We also do turn on full IOMMU mappings in that case for devices that are
> > > marked as external facing by the same firmware that provided the DMAR
> > > bit. If the user decides to disable IOMMU from command line for instance
> > > then we expect she knows what she is doing.
> >
> > Yeah, if external_facing is set correctly then we can safely expect the
> > the IOMMU layer to do the right thing, so in that case it probably is OK
> > to infer that if an IOMMU is present for the NHI then it'll be managing
> > that whole bus hierarchy. What I'm really thinking about here is whether
> > we can defend against a case when external_facing *isn't* set, so we
> > treat the tunnelled ports as normal PCI buses, assume it's OK since
> > we've got an IOMMU and everything else is getting translation domains by
> > default, but then a Thunderbolt device shows up masquerading the
> VID:DID
> > of something that gets a passthrough quirk, and thus tricks its way
> > through the perceived protection.
> >
> > Robin.
> 
> Unless it happened after 5.17-rc8 looking at the code I think that's Intel
> specific behavior though at the moment (has_external_pci).  I don't see it
> in a generic layer.

Oh it's via dev_is_untrusted.  A few layers through external facing translates
to untrusted and then dev_use_swiotlb is used, got it.

> 
> In addition to the point Robin said about firmware not setting external facing
> if the IOMMU was disabled on command line then iommu_dma_protection
> would be showing the wrong values meaning userspace may choose to
> authorize the device automatically in a potentially unsafe scenario.
> 
> Even if the user "knew what they were doing", I would expect that we still
> do our best to protect them from themselves and not advertise something
> that will cause automatic authorization.

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

[Limonciello, Mario via iommu]

[Public]



> -----Original Message-----
> From: Limonciello, Mario
> Sent: Wednesday, March 16, 2022 12:54
> To: Robin Murphy <robin.murphy@arm.com>; Mika Westerberg
> <mika.westerberg@linux.intel.com>
> Cc: michael.jamet@intel.com; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org; YehezkelShB@gmail.com; iommu@lists.linux-
> foundation.org; andreas.noever@gmail.com; hch@lst.de
> Subject: RE: [PATCH] thunderbolt: Stop using iommu_present()
> 
> [Public]
> 
> > >>>
> > >>> There is a way to figure out the "tunneled" PCIe ports by looking at
> > >>> certain properties and we do that already actually. The BIOS has the
> > >>> following under these ports:
> > >>>
> > >>>
> >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs
> > >>> .microsoft.com%2Fen-us%2Fwindows-
> > hardware%2Fdrivers%2Fpci%2Fdsd-
> > >>> for-pcie-root-ports%23identifying-externally-exposed-pcie-root-
> > >>>
> >
> ports&amp;data=04%7C01%7Cmario.limonciello%40amd.com%7C0465d319a
> > >>>
> >
> 6684335d9c208da07710e7c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7
> > >>>
> >
> C0%7C637830479402895833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
> > >>>
> >
> LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&am
> > >>>
> >
> p;sdata=z6hpYGpj%2B%2BVvz9d6MXiO4N66PUm4zwhOdI%2Br6l3PjhQ%3D
> > >>> &amp;reserved=0
> > >>>
> > >>> and the ports will have dev->external_facing set to 1. Perhaps looking
> > >>> at that field helps here?
> > >>
> > >> External facing isn't a guarantee from the firmware though.  It's
> > something we
> > >> all expect in practice, but I think it's better to look at the ones that are
> > from
> > >> the _DSD usb4-host-interface to be safer.
> > >
> > > Right but then we have the discrete ones with the DVSEC that exposes
> the
> > > tunneled ports :(
> > >
> 
> Can the USB4 CM make the device links in the DVSEC case perhaps too?  I
> would
> think we want that anyway to control device suspend ordering.
> 
> If I had something discrete to try I'd dust off the DVSEC patch I wrote before
> to
> try it, but alas all I have is integrated stuff on my hand.
> 
> > >> Mika, you might not have seen it yet, but I sent a follow up diff in this
> > thread
> > >> to Robin's patch.  If that looks good Robin can submit a v2 (or I'm happy
> to
> > do
> > >> so as well as I confirmed it helps my original intent too).
> > >
> > > I saw it now and I'm thinking are we making this unnecessary complex? I
> > > mean Microsoft solely depends on the DMAR platform opt-in flag:
> > >
> > >
> >
> 
> I think Microsoft doesn't allow you to turn off the IOMMU though or put it in
> passthrough through on the kernel command line.
> 
> > > We also do turn on full IOMMU mappings in that case for devices that are
> > > marked as external facing by the same firmware that provided the DMAR
> > > bit. If the user decides to disable IOMMU from command line for instance
> > > then we expect she knows what she is doing.
> >
> > Yeah, if external_facing is set correctly then we can safely expect the
> > the IOMMU layer to do the right thing, so in that case it probably is OK
> > to infer that if an IOMMU is present for the NHI then it'll be managing
> > that whole bus hierarchy. What I'm really thinking about here is whether
> > we can defend against a case when external_facing *isn't* set, so we
> > treat the tunnelled ports as normal PCI buses, assume it's OK since
> > we've got an IOMMU and everything else is getting translation domains by
> > default, but then a Thunderbolt device shows up masquerading the
> VID:DID
> > of something that gets a passthrough quirk, and thus tricks its way
> > through the perceived protection.
> >
> > Robin.
> 
> Unless it happened after 5.17-rc8 looking at the code I think that's Intel
> specific behavior though at the moment (has_external_pci).  I don't see it
> in a generic layer.

Oh it's via dev_is_untrusted.  A few layers through external facing translates
to untrusted and then dev_use_swiotlb is used, got it.

> 
> In addition to the point Robin said about firmware not setting external facing
> if the IOMMU was disabled on command line then iommu_dma_protection
> would be showing the wrong values meaning userspace may choose to
> authorize the device automatically in a potentially unsafe scenario.
> 
> Even if the user "knew what they were doing", I would expect that we still
> do our best to protect them from themselves and not advertise something
> that will cause automatic authorization.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Robin Murphy]

On 2022-03-16 17:53, Limonciello, Mario wrote:
> [Public]
> 
>>>>>
>>>>> There is a way to figure out the "tunneled" PCIe ports by looking at
>>>>> certain properties and we do that already actually. The BIOS has the
>>>>> following under these ports:
>>>>>
>>>>>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs
>>>>> .microsoft.com%2Fen-us%2Fwindows-
>> hardware%2Fdrivers%2Fpci%2Fdsd-
>>>>> for-pcie-root-ports%23identifying-externally-exposed-pcie-root-
>>>>>
>> ports&data=04%7C01%7Cmario.limonciello%40amd.com%7C0465d319a
>>>>>
>> 6684335d9c208da07710e7c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7
>>>>>
>> C0%7C637830479402895833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
>>>>>
>> LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&am
>>>>>
>> p;sdata=z6hpYGpj%2B%2BVvz9d6MXiO4N66PUm4zwhOdI%2Br6l3PjhQ%3D
>>>>> &reserved=0
>>>>>
>>>>> and the ports will have dev->external_facing set to 1. Perhaps looking
>>>>> at that field helps here?
>>>>
>>>> External facing isn't a guarantee from the firmware though.  It's
>> something we
>>>> all expect in practice, but I think it's better to look at the ones that are
>> from
>>>> the _DSD usb4-host-interface to be safer.
>>>
>>> Right but then we have the discrete ones with the DVSEC that exposes the
>>> tunneled ports :(
>>>
> 
> Can the USB4 CM make the device links in the DVSEC case perhaps too?  I would
> think we want that anyway to control device suspend ordering.
> 
> If I had something discrete to try I'd dust off the DVSEC patch I wrote before to
> try it, but alas all I have is integrated stuff on my hand.
> 
>>>> Mika, you might not have seen it yet, but I sent a follow up diff in this
>> thread
>>>> to Robin's patch.  If that looks good Robin can submit a v2 (or I'm happy to
>> do
>>>> so as well as I confirmed it helps my original intent too).
>>>
>>> I saw it now and I'm thinking are we making this unnecessary complex? I
>>> mean Microsoft solely depends on the DMAR platform opt-in flag:
>>>
>>>
>>
> 
> I think Microsoft doesn't allow you to turn off the IOMMU though or put it in
> passthrough through on the kernel command line.
> 
>>> We also do turn on full IOMMU mappings in that case for devices that are
>>> marked as external facing by the same firmware that provided the DMAR
>>> bit. If the user decides to disable IOMMU from command line for instance
>>> then we expect she knows what she is doing.
>>
>> Yeah, if external_facing is set correctly then we can safely expect the
>> the IOMMU layer to do the right thing, so in that case it probably is OK
>> to infer that if an IOMMU is present for the NHI then it'll be managing
>> that whole bus hierarchy. What I'm really thinking about here is whether
>> we can defend against a case when external_facing *isn't* set, so we
>> treat the tunnelled ports as normal PCI buses, assume it's OK since
>> we've got an IOMMU and everything else is getting translation domains by
>> default, but then a Thunderbolt device shows up masquerading the VID:DID
>> of something that gets a passthrough quirk, and thus tricks its way
>> through the perceived protection.
>>
>> Robin.
> 
> Unless it happened after 5.17-rc8 looking at the code I think that's Intel
> specific behavior though at the moment (has_external_pci).  I don't see it
> in a generic layer.

Ah, it's not necessarily the most obvious thing - 
pci_dev->external_facing gets propagated through to pci_dev->untrusted 
by set_pcie_untrusted(), and it's that that's then checked by 
iommu_get_def_domain_type() to enforce a translation domain regardless 
of default passthrough or quirks. It's then further checked by 
iommu-dma's dev_is_untrusted() to enforce bounce-buffering to avoid data 
leakage in sub-page mappings too.

> In addition to the point Robin said about firmware not setting external facing
> if the IOMMU was disabled on command line then iommu_dma_protection
> would be showing the wrong values meaning userspace may choose to
> authorize the device automatically in a potentially unsafe scenario.
> 
> Even if the user "knew what they were doing", I would expect that we still
> do our best to protect them from themselves and not advertise something
> that will cause automatic authorization.

Might it be reasonable for the Thunderbolt core to check early on if any 
tunnelled ports are not marked as external facing, and if so just tell 
the user that iommu_dma_protection is off the table and anything they 
authorise is at their own risk?

Robin.

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

[Robin Murphy]

On 2022-03-16 17:53, Limonciello, Mario wrote:
> [Public]
> 
>>>>>
>>>>> There is a way to figure out the "tunneled" PCIe ports by looking at
>>>>> certain properties and we do that already actually. The BIOS has the
>>>>> following under these ports:
>>>>>
>>>>>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs
>>>>> .microsoft.com%2Fen-us%2Fwindows-
>> hardware%2Fdrivers%2Fpci%2Fdsd-
>>>>> for-pcie-root-ports%23identifying-externally-exposed-pcie-root-
>>>>>
>> ports&data=04%7C01%7Cmario.limonciello%40amd.com%7C0465d319a
>>>>>
>> 6684335d9c208da07710e7c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7
>>>>>
>> C0%7C637830479402895833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
>>>>>
>> LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&am
>>>>>
>> p;sdata=z6hpYGpj%2B%2BVvz9d6MXiO4N66PUm4zwhOdI%2Br6l3PjhQ%3D
>>>>> &reserved=0
>>>>>
>>>>> and the ports will have dev->external_facing set to 1. Perhaps looking
>>>>> at that field helps here?
>>>>
>>>> External facing isn't a guarantee from the firmware though.  It's
>> something we
>>>> all expect in practice, but I think it's better to look at the ones that are
>> from
>>>> the _DSD usb4-host-interface to be safer.
>>>
>>> Right but then we have the discrete ones with the DVSEC that exposes the
>>> tunneled ports :(
>>>
> 
> Can the USB4 CM make the device links in the DVSEC case perhaps too?  I would
> think we want that anyway to control device suspend ordering.
> 
> If I had something discrete to try I'd dust off the DVSEC patch I wrote before to
> try it, but alas all I have is integrated stuff on my hand.
> 
>>>> Mika, you might not have seen it yet, but I sent a follow up diff in this
>> thread
>>>> to Robin's patch.  If that looks good Robin can submit a v2 (or I'm happy to
>> do
>>>> so as well as I confirmed it helps my original intent too).
>>>
>>> I saw it now and I'm thinking are we making this unnecessary complex? I
>>> mean Microsoft solely depends on the DMAR platform opt-in flag:
>>>
>>>
>>
> 
> I think Microsoft doesn't allow you to turn off the IOMMU though or put it in
> passthrough through on the kernel command line.
> 
>>> We also do turn on full IOMMU mappings in that case for devices that are
>>> marked as external facing by the same firmware that provided the DMAR
>>> bit. If the user decides to disable IOMMU from command line for instance
>>> then we expect she knows what she is doing.
>>
>> Yeah, if external_facing is set correctly then we can safely expect the
>> the IOMMU layer to do the right thing, so in that case it probably is OK
>> to infer that if an IOMMU is present for the NHI then it'll be managing
>> that whole bus hierarchy. What I'm really thinking about here is whether
>> we can defend against a case when external_facing *isn't* set, so we
>> treat the tunnelled ports as normal PCI buses, assume it's OK since
>> we've got an IOMMU and everything else is getting translation domains by
>> default, but then a Thunderbolt device shows up masquerading the VID:DID
>> of something that gets a passthrough quirk, and thus tricks its way
>> through the perceived protection.
>>
>> Robin.
> 
> Unless it happened after 5.17-rc8 looking at the code I think that's Intel
> specific behavior though at the moment (has_external_pci).  I don't see it
> in a generic layer.

Ah, it's not necessarily the most obvious thing - 
pci_dev->external_facing gets propagated through to pci_dev->untrusted 
by set_pcie_untrusted(), and it's that that's then checked by 
iommu_get_def_domain_type() to enforce a translation domain regardless 
of default passthrough or quirks. It's then further checked by 
iommu-dma's dev_is_untrusted() to enforce bounce-buffering to avoid data 
leakage in sub-page mappings too.

> In addition to the point Robin said about firmware not setting external facing
> if the IOMMU was disabled on command line then iommu_dma_protection
> would be showing the wrong values meaning userspace may choose to
> authorize the device automatically in a potentially unsafe scenario.
> 
> Even if the user "knew what they were doing", I would expect that we still
> do our best to protect them from themselves and not advertise something
> that will cause automatic authorization.

Might it be reasonable for the Thunderbolt core to check early on if any 
tunnelled ports are not marked as external facing, and if so just tell 
the user that iommu_dma_protection is off the table and anything they 
authorise is at their own risk?

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Limonciello, Mario]

[Public]

> > Can the USB4 CM make the device links in the DVSEC case perhaps too?  I
> would
> > think we want that anyway to control device suspend ordering.
> >
> > If I had something discrete to try I'd dust off the DVSEC patch I wrote
> before to
> > try it, but alas all I have is integrated stuff on my hand.
> >
> >>>> Mika, you might not have seen it yet, but I sent a follow up diff in this
> >> thread
> >>>> to Robin's patch.  If that looks good Robin can submit a v2 (or I'm happy
> to
> >> do
> >>>> so as well as I confirmed it helps my original intent too).
> >>>
> >>> I saw it now and I'm thinking are we making this unnecessary complex? I
> >>> mean Microsoft solely depends on the DMAR platform opt-in flag:
> >>>
> >>>
> >>
> >
> > I think Microsoft doesn't allow you to turn off the IOMMU though or put it
> in
> > passthrough through on the kernel command line.
> >
> >>> We also do turn on full IOMMU mappings in that case for devices that
> are
> >>> marked as external facing by the same firmware that provided the
> DMAR
> >>> bit. If the user decides to disable IOMMU from command line for
> instance
> >>> then we expect she knows what she is doing.
> >>
> >> Yeah, if external_facing is set correctly then we can safely expect the
> >> the IOMMU layer to do the right thing, so in that case it probably is OK
> >> to infer that if an IOMMU is present for the NHI then it'll be managing
> >> that whole bus hierarchy. What I'm really thinking about here is whether
> >> we can defend against a case when external_facing *isn't* set, so we
> >> treat the tunnelled ports as normal PCI buses, assume it's OK since
> >> we've got an IOMMU and everything else is getting translation domains
> by
> >> default, but then a Thunderbolt device shows up masquerading the
> VID:DID
> >> of something that gets a passthrough quirk, and thus tricks its way
> >> through the perceived protection.
> >>
> >> Robin.
> >
> > Unless it happened after 5.17-rc8 looking at the code I think that's Intel
> > specific behavior though at the moment (has_external_pci).  I don't see it
> > in a generic layer.
> 
> Ah, it's not necessarily the most obvious thing -
> pci_dev->external_facing gets propagated through to pci_dev->untrusted
> by set_pcie_untrusted(), and it's that that's then checked by
> iommu_get_def_domain_type() to enforce a translation domain regardless
> of default passthrough or quirks. It's then further checked by
> iommu-dma's dev_is_untrusted() to enforce bounce-buffering to avoid data
> leakage in sub-page mappings too.
> 

Ah thanks for explaining it, that was immediately obvious to me.

> > In addition to the point Robin said about firmware not setting external
> facing
> > if the IOMMU was disabled on command line then iommu_dma_protection
> > would be showing the wrong values meaning userspace may choose to
> > authorize the device automatically in a potentially unsafe scenario.
> >
> > Even if the user "knew what they were doing", I would expect that we still
> > do our best to protect them from themselves and not advertise something
> > that will cause automatic authorization.
> 
> Might it be reasonable for the Thunderbolt core to check early on if any
> tunnelled ports are not marked as external facing, and if so just tell
> the user that iommu_dma_protection is off the table and anything they
> authorise is at their own risk?
> 
> Robin.

How about in iommu_dma_protection_show to just check that all the device
links to the NHI are marked as untrusted?

Then if there are device links missing we solve that separately (discrete USB4
DVSEC case we just need to make those device links).

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

[Limonciello, Mario via iommu]

[Public]

> > Can the USB4 CM make the device links in the DVSEC case perhaps too?  I
> would
> > think we want that anyway to control device suspend ordering.
> >
> > If I had something discrete to try I'd dust off the DVSEC patch I wrote
> before to
> > try it, but alas all I have is integrated stuff on my hand.
> >
> >>>> Mika, you might not have seen it yet, but I sent a follow up diff in this
> >> thread
> >>>> to Robin's patch.  If that looks good Robin can submit a v2 (or I'm happy
> to
> >> do
> >>>> so as well as I confirmed it helps my original intent too).
> >>>
> >>> I saw it now and I'm thinking are we making this unnecessary complex? I
> >>> mean Microsoft solely depends on the DMAR platform opt-in flag:
> >>>
> >>>
> >>
> >
> > I think Microsoft doesn't allow you to turn off the IOMMU though or put it
> in
> > passthrough through on the kernel command line.
> >
> >>> We also do turn on full IOMMU mappings in that case for devices that
> are
> >>> marked as external facing by the same firmware that provided the
> DMAR
> >>> bit. If the user decides to disable IOMMU from command line for
> instance
> >>> then we expect she knows what she is doing.
> >>
> >> Yeah, if external_facing is set correctly then we can safely expect the
> >> the IOMMU layer to do the right thing, so in that case it probably is OK
> >> to infer that if an IOMMU is present for the NHI then it'll be managing
> >> that whole bus hierarchy. What I'm really thinking about here is whether
> >> we can defend against a case when external_facing *isn't* set, so we
> >> treat the tunnelled ports as normal PCI buses, assume it's OK since
> >> we've got an IOMMU and everything else is getting translation domains
> by
> >> default, but then a Thunderbolt device shows up masquerading the
> VID:DID
> >> of something that gets a passthrough quirk, and thus tricks its way
> >> through the perceived protection.
> >>
> >> Robin.
> >
> > Unless it happened after 5.17-rc8 looking at the code I think that's Intel
> > specific behavior though at the moment (has_external_pci).  I don't see it
> > in a generic layer.
> 
> Ah, it's not necessarily the most obvious thing -
> pci_dev->external_facing gets propagated through to pci_dev->untrusted
> by set_pcie_untrusted(), and it's that that's then checked by
> iommu_get_def_domain_type() to enforce a translation domain regardless
> of default passthrough or quirks. It's then further checked by
> iommu-dma's dev_is_untrusted() to enforce bounce-buffering to avoid data
> leakage in sub-page mappings too.
> 

Ah thanks for explaining it, that was immediately obvious to me.

> > In addition to the point Robin said about firmware not setting external
> facing
> > if the IOMMU was disabled on command line then iommu_dma_protection
> > would be showing the wrong values meaning userspace may choose to
> > authorize the device automatically in a potentially unsafe scenario.
> >
> > Even if the user "knew what they were doing", I would expect that we still
> > do our best to protect them from themselves and not advertise something
> > that will cause automatic authorization.
> 
> Might it be reasonable for the Thunderbolt core to check early on if any
> tunnelled ports are not marked as external facing, and if so just tell
> the user that iommu_dma_protection is off the table and anything they
> authorise is at their own risk?
> 
> Robin.

How about in iommu_dma_protection_show to just check that all the device
links to the NHI are marked as untrusted?

Then if there are device links missing we solve that separately (discrete USB4
DVSEC case we just need to make those device links).
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Robin Murphy]

On 2022-03-16 18:34, Limonciello, Mario wrote:
> [Public]
> 
>>> Can the USB4 CM make the device links in the DVSEC case perhaps too?  I
>> would
>>> think we want that anyway to control device suspend ordering.
>>>
>>> If I had something discrete to try I'd dust off the DVSEC patch I wrote
>> before to
>>> try it, but alas all I have is integrated stuff on my hand.
>>>
>>>>>> Mika, you might not have seen it yet, but I sent a follow up diff in this
>>>> thread
>>>>>> to Robin's patch.  If that looks good Robin can submit a v2 (or I'm happy
>> to
>>>> do
>>>>>> so as well as I confirmed it helps my original intent too).
>>>>>
>>>>> I saw it now and I'm thinking are we making this unnecessary complex? I
>>>>> mean Microsoft solely depends on the DMAR platform opt-in flag:
>>>>>
>>>>>
>>>>
>>>
>>> I think Microsoft doesn't allow you to turn off the IOMMU though or put it
>> in
>>> passthrough through on the kernel command line.
>>>
>>>>> We also do turn on full IOMMU mappings in that case for devices that
>> are
>>>>> marked as external facing by the same firmware that provided the
>> DMAR
>>>>> bit. If the user decides to disable IOMMU from command line for
>> instance
>>>>> then we expect she knows what she is doing.
>>>>
>>>> Yeah, if external_facing is set correctly then we can safely expect the
>>>> the IOMMU layer to do the right thing, so in that case it probably is OK
>>>> to infer that if an IOMMU is present for the NHI then it'll be managing
>>>> that whole bus hierarchy. What I'm really thinking about here is whether
>>>> we can defend against a case when external_facing *isn't* set, so we
>>>> treat the tunnelled ports as normal PCI buses, assume it's OK since
>>>> we've got an IOMMU and everything else is getting translation domains
>> by
>>>> default, but then a Thunderbolt device shows up masquerading the
>> VID:DID
>>>> of something that gets a passthrough quirk, and thus tricks its way
>>>> through the perceived protection.
>>>>
>>>> Robin.
>>>
>>> Unless it happened after 5.17-rc8 looking at the code I think that's Intel
>>> specific behavior though at the moment (has_external_pci).  I don't see it
>>> in a generic layer.
>>
>> Ah, it's not necessarily the most obvious thing -
>> pci_dev->external_facing gets propagated through to pci_dev->untrusted
>> by set_pcie_untrusted(), and it's that that's then checked by
>> iommu_get_def_domain_type() to enforce a translation domain regardless
>> of default passthrough or quirks. It's then further checked by
>> iommu-dma's dev_is_untrusted() to enforce bounce-buffering to avoid data
>> leakage in sub-page mappings too.
>>
> 
> Ah thanks for explaining it, that was immediately obvious to me.
> 
>>> In addition to the point Robin said about firmware not setting external
>> facing
>>> if the IOMMU was disabled on command line then iommu_dma_protection
>>> would be showing the wrong values meaning userspace may choose to
>>> authorize the device automatically in a potentially unsafe scenario.
>>>
>>> Even if the user "knew what they were doing", I would expect that we still
>>> do our best to protect them from themselves and not advertise something
>>> that will cause automatic authorization.
>>
>> Might it be reasonable for the Thunderbolt core to check early on if any
>> tunnelled ports are not marked as external facing, and if so just tell
>> the user that iommu_dma_protection is off the table and anything they
>> authorise is at their own risk?
>>
>> Robin.
> 
> How about in iommu_dma_protection_show to just check that all the device
> links to the NHI are marked as untrusted?
> 
> Then if there are device links missing we solve that separately (discrete USB4
> DVSEC case we just need to make those device links).

The feeling I'm getting from all this is that if we've got as far as 
iommu_dma_protection_show() then it's really too late to meaningfully 
mitigate bad firmware. We should be able to detect missing 
untrusted/external-facing properties as early as nhi_probe(), and if we 
could go into "continue at your own risk" mode right then *before* 
anything else happens, it all becomes a lot easier to reason about. If 
there's a strong enough impetus from Microsoft for system vendors to get 
their firmware right, hopefully we can get away with not trying too hard 
to cope with systems that haven't.

I'm inclined to send v2 of this patch effectively going back to my 
original (even simpler) cleanup, just now with much more reasoning about 
why it isn't doing more :)

Cheers,
Robin.

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

[Robin Murphy]

On 2022-03-16 18:34, Limonciello, Mario wrote:
> [Public]
> 
>>> Can the USB4 CM make the device links in the DVSEC case perhaps too?  I
>> would
>>> think we want that anyway to control device suspend ordering.
>>>
>>> If I had something discrete to try I'd dust off the DVSEC patch I wrote
>> before to
>>> try it, but alas all I have is integrated stuff on my hand.
>>>
>>>>>> Mika, you might not have seen it yet, but I sent a follow up diff in this
>>>> thread
>>>>>> to Robin's patch.  If that looks good Robin can submit a v2 (or I'm happy
>> to
>>>> do
>>>>>> so as well as I confirmed it helps my original intent too).
>>>>>
>>>>> I saw it now and I'm thinking are we making this unnecessary complex? I
>>>>> mean Microsoft solely depends on the DMAR platform opt-in flag:
>>>>>
>>>>>
>>>>
>>>
>>> I think Microsoft doesn't allow you to turn off the IOMMU though or put it
>> in
>>> passthrough through on the kernel command line.
>>>
>>>>> We also do turn on full IOMMU mappings in that case for devices that
>> are
>>>>> marked as external facing by the same firmware that provided the
>> DMAR
>>>>> bit. If the user decides to disable IOMMU from command line for
>> instance
>>>>> then we expect she knows what she is doing.
>>>>
>>>> Yeah, if external_facing is set correctly then we can safely expect the
>>>> the IOMMU layer to do the right thing, so in that case it probably is OK
>>>> to infer that if an IOMMU is present for the NHI then it'll be managing
>>>> that whole bus hierarchy. What I'm really thinking about here is whether
>>>> we can defend against a case when external_facing *isn't* set, so we
>>>> treat the tunnelled ports as normal PCI buses, assume it's OK since
>>>> we've got an IOMMU and everything else is getting translation domains
>> by
>>>> default, but then a Thunderbolt device shows up masquerading the
>> VID:DID
>>>> of something that gets a passthrough quirk, and thus tricks its way
>>>> through the perceived protection.
>>>>
>>>> Robin.
>>>
>>> Unless it happened after 5.17-rc8 looking at the code I think that's Intel
>>> specific behavior though at the moment (has_external_pci).  I don't see it
>>> in a generic layer.
>>
>> Ah, it's not necessarily the most obvious thing -
>> pci_dev->external_facing gets propagated through to pci_dev->untrusted
>> by set_pcie_untrusted(), and it's that that's then checked by
>> iommu_get_def_domain_type() to enforce a translation domain regardless
>> of default passthrough or quirks. It's then further checked by
>> iommu-dma's dev_is_untrusted() to enforce bounce-buffering to avoid data
>> leakage in sub-page mappings too.
>>
> 
> Ah thanks for explaining it, that was immediately obvious to me.
> 
>>> In addition to the point Robin said about firmware not setting external
>> facing
>>> if the IOMMU was disabled on command line then iommu_dma_protection
>>> would be showing the wrong values meaning userspace may choose to
>>> authorize the device automatically in a potentially unsafe scenario.
>>>
>>> Even if the user "knew what they were doing", I would expect that we still
>>> do our best to protect them from themselves and not advertise something
>>> that will cause automatic authorization.
>>
>> Might it be reasonable for the Thunderbolt core to check early on if any
>> tunnelled ports are not marked as external facing, and if so just tell
>> the user that iommu_dma_protection is off the table and anything they
>> authorise is at their own risk?
>>
>> Robin.
> 
> How about in iommu_dma_protection_show to just check that all the device
> links to the NHI are marked as untrusted?
> 
> Then if there are device links missing we solve that separately (discrete USB4
> DVSEC case we just need to make those device links).

The feeling I'm getting from all this is that if we've got as far as 
iommu_dma_protection_show() then it's really too late to meaningfully 
mitigate bad firmware. We should be able to detect missing 
untrusted/external-facing properties as early as nhi_probe(), and if we 
could go into "continue at your own risk" mode right then *before* 
anything else happens, it all becomes a lot easier to reason about. If 
there's a strong enough impetus from Microsoft for system vendors to get 
their firmware right, hopefully we can get away with not trying too hard 
to cope with systems that haven't.

I'm inclined to send v2 of this patch effectively going back to my 
original (even simpler) cleanup, just now with much more reasoning about 
why it isn't doing more :)

Cheers,
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Wednesday, March 16, 2022 14:18
> To: Limonciello, Mario <Mario.Limonciello@amd.com>; Mika Westerberg
> <mika.westerberg@linux.intel.com>
> Cc: michael.jamet@intel.com; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org; YehezkelShB@gmail.com; iommu@lists.linux-
> foundation.org; andreas.noever@gmail.com; hch@lst.de
> Subject: Re: [PATCH] thunderbolt: Stop using iommu_present()
> 
> On 2022-03-16 18:34, Limonciello, Mario wrote:
> > [Public]
> >
> >>> Can the USB4 CM make the device links in the DVSEC case perhaps too?
> I
> >> would
> >>> think we want that anyway to control device suspend ordering.
> >>>
> >>> If I had something discrete to try I'd dust off the DVSEC patch I wrote
> >> before to
> >>> try it, but alas all I have is integrated stuff on my hand.
> >>>
> >>>>>> Mika, you might not have seen it yet, but I sent a follow up diff in
> this
> >>>> thread
> >>>>>> to Robin's patch.  If that looks good Robin can submit a v2 (or I'm
> happy
> >> to
> >>>> do
> >>>>>> so as well as I confirmed it helps my original intent too).
> >>>>>
> >>>>> I saw it now and I'm thinking are we making this unnecessary
> complex? I
> >>>>> mean Microsoft solely depends on the DMAR platform opt-in flag:
> >>>>>
> >>>>>
> >>>>
> >>>
> >>> I think Microsoft doesn't allow you to turn off the IOMMU though or put
> it
> >> in
> >>> passthrough through on the kernel command line.
> >>>
> >>>>> We also do turn on full IOMMU mappings in that case for devices that
> >> are
> >>>>> marked as external facing by the same firmware that provided the
> >> DMAR
> >>>>> bit. If the user decides to disable IOMMU from command line for
> >> instance
> >>>>> then we expect she knows what she is doing.
> >>>>
> >>>> Yeah, if external_facing is set correctly then we can safely expect the
> >>>> the IOMMU layer to do the right thing, so in that case it probably is OK
> >>>> to infer that if an IOMMU is present for the NHI then it'll be managing
> >>>> that whole bus hierarchy. What I'm really thinking about here is
> whether
> >>>> we can defend against a case when external_facing *isn't* set, so we
> >>>> treat the tunnelled ports as normal PCI buses, assume it's OK since
> >>>> we've got an IOMMU and everything else is getting translation domains
> >> by
> >>>> default, but then a Thunderbolt device shows up masquerading the
> >> VID:DID
> >>>> of something that gets a passthrough quirk, and thus tricks its way
> >>>> through the perceived protection.
> >>>>
> >>>> Robin.
> >>>
> >>> Unless it happened after 5.17-rc8 looking at the code I think that's Intel
> >>> specific behavior though at the moment (has_external_pci).  I don't see
> it
> >>> in a generic layer.
> >>
> >> Ah, it's not necessarily the most obvious thing -
> >> pci_dev->external_facing gets propagated through to pci_dev-
> >untrusted
> >> by set_pcie_untrusted(), and it's that that's then checked by
> >> iommu_get_def_domain_type() to enforce a translation domain
> regardless
> >> of default passthrough or quirks. It's then further checked by
> >> iommu-dma's dev_is_untrusted() to enforce bounce-buffering to avoid
> data
> >> leakage in sub-page mappings too.
> >>
> >
> > Ah thanks for explaining it, that was immediately obvious to me.
> >
> >>> In addition to the point Robin said about firmware not setting external
> >> facing
> >>> if the IOMMU was disabled on command line then
> iommu_dma_protection
> >>> would be showing the wrong values meaning userspace may choose to
> >>> authorize the device automatically in a potentially unsafe scenario.
> >>>
> >>> Even if the user "knew what they were doing", I would expect that we
> still
> >>> do our best to protect them from themselves and not advertise
> something
> >>> that will cause automatic authorization.
> >>
> >> Might it be reasonable for the Thunderbolt core to check early on if any
> >> tunnelled ports are not marked as external facing, and if so just tell
> >> the user that iommu_dma_protection is off the table and anything they
> >> authorise is at their own risk?
> >>
> >> Robin.
> >
> > How about in iommu_dma_protection_show to just check that all the
> device
> > links to the NHI are marked as untrusted?
> >
> > Then if there are device links missing we solve that separately (discrete
> USB4
> > DVSEC case we just need to make those device links).
> 
> The feeling I'm getting from all this is that if we've got as far as
> iommu_dma_protection_show() then it's really too late to meaningfully
> mitigate bad firmware. We should be able to detect missing
> untrusted/external-facing properties as early as nhi_probe(), and if we
> could go into "continue at your own risk" mode right then *before*
> anything else happens, it all becomes a lot easier to reason about. If
> there's a strong enough impetus from Microsoft for system vendors to get
> their firmware right, hopefully we can get away with not trying too hard
> to cope with systems that haven't.
> 
> I'm inclined to send v2 of this patch effectively going back to my
> original (even simpler) cleanup, just now with much more reasoning about
> why it isn't doing more :)
> 

Yeah I'm fine with your patch code as it stands right now.
In that case how about a second patch in the series to dev_warn in drivers/thunderbolt/acpi.c
right when the link is made if it's not set as trusted?  That should happen right during
tb_probe as you suggest then.

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

[Limonciello, Mario via iommu]

[Public]



> -----Original Message-----
> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Wednesday, March 16, 2022 14:18
> To: Limonciello, Mario <Mario.Limonciello@amd.com>; Mika Westerberg
> <mika.westerberg@linux.intel.com>
> Cc: michael.jamet@intel.com; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org; YehezkelShB@gmail.com; iommu@lists.linux-
> foundation.org; andreas.noever@gmail.com; hch@lst.de
> Subject: Re: [PATCH] thunderbolt: Stop using iommu_present()
> 
> On 2022-03-16 18:34, Limonciello, Mario wrote:
> > [Public]
> >
> >>> Can the USB4 CM make the device links in the DVSEC case perhaps too?
> I
> >> would
> >>> think we want that anyway to control device suspend ordering.
> >>>
> >>> If I had something discrete to try I'd dust off the DVSEC patch I wrote
> >> before to
> >>> try it, but alas all I have is integrated stuff on my hand.
> >>>
> >>>>>> Mika, you might not have seen it yet, but I sent a follow up diff in
> this
> >>>> thread
> >>>>>> to Robin's patch.  If that looks good Robin can submit a v2 (or I'm
> happy
> >> to
> >>>> do
> >>>>>> so as well as I confirmed it helps my original intent too).
> >>>>>
> >>>>> I saw it now and I'm thinking are we making this unnecessary
> complex? I
> >>>>> mean Microsoft solely depends on the DMAR platform opt-in flag:
> >>>>>
> >>>>>
> >>>>
> >>>
> >>> I think Microsoft doesn't allow you to turn off the IOMMU though or put
> it
> >> in
> >>> passthrough through on the kernel command line.
> >>>
> >>>>> We also do turn on full IOMMU mappings in that case for devices that
> >> are
> >>>>> marked as external facing by the same firmware that provided the
> >> DMAR
> >>>>> bit. If the user decides to disable IOMMU from command line for
> >> instance
> >>>>> then we expect she knows what she is doing.
> >>>>
> >>>> Yeah, if external_facing is set correctly then we can safely expect the
> >>>> the IOMMU layer to do the right thing, so in that case it probably is OK
> >>>> to infer that if an IOMMU is present for the NHI then it'll be managing
> >>>> that whole bus hierarchy. What I'm really thinking about here is
> whether
> >>>> we can defend against a case when external_facing *isn't* set, so we
> >>>> treat the tunnelled ports as normal PCI buses, assume it's OK since
> >>>> we've got an IOMMU and everything else is getting translation domains
> >> by
> >>>> default, but then a Thunderbolt device shows up masquerading the
> >> VID:DID
> >>>> of something that gets a passthrough quirk, and thus tricks its way
> >>>> through the perceived protection.
> >>>>
> >>>> Robin.
> >>>
> >>> Unless it happened after 5.17-rc8 looking at the code I think that's Intel
> >>> specific behavior though at the moment (has_external_pci).  I don't see
> it
> >>> in a generic layer.
> >>
> >> Ah, it's not necessarily the most obvious thing -
> >> pci_dev->external_facing gets propagated through to pci_dev-
> >untrusted
> >> by set_pcie_untrusted(), and it's that that's then checked by
> >> iommu_get_def_domain_type() to enforce a translation domain
> regardless
> >> of default passthrough or quirks. It's then further checked by
> >> iommu-dma's dev_is_untrusted() to enforce bounce-buffering to avoid
> data
> >> leakage in sub-page mappings too.
> >>
> >
> > Ah thanks for explaining it, that was immediately obvious to me.
> >
> >>> In addition to the point Robin said about firmware not setting external
> >> facing
> >>> if the IOMMU was disabled on command line then
> iommu_dma_protection
> >>> would be showing the wrong values meaning userspace may choose to
> >>> authorize the device automatically in a potentially unsafe scenario.
> >>>
> >>> Even if the user "knew what they were doing", I would expect that we
> still
> >>> do our best to protect them from themselves and not advertise
> something
> >>> that will cause automatic authorization.
> >>
> >> Might it be reasonable for the Thunderbolt core to check early on if any
> >> tunnelled ports are not marked as external facing, and if so just tell
> >> the user that iommu_dma_protection is off the table and anything they
> >> authorise is at their own risk?
> >>
> >> Robin.
> >
> > How about in iommu_dma_protection_show to just check that all the
> device
> > links to the NHI are marked as untrusted?
> >
> > Then if there are device links missing we solve that separately (discrete
> USB4
> > DVSEC case we just need to make those device links).
> 
> The feeling I'm getting from all this is that if we've got as far as
> iommu_dma_protection_show() then it's really too late to meaningfully
> mitigate bad firmware. We should be able to detect missing
> untrusted/external-facing properties as early as nhi_probe(), and if we
> could go into "continue at your own risk" mode right then *before*
> anything else happens, it all becomes a lot easier to reason about. If
> there's a strong enough impetus from Microsoft for system vendors to get
> their firmware right, hopefully we can get away with not trying too hard
> to cope with systems that haven't.
> 
> I'm inclined to send v2 of this patch effectively going back to my
> original (even simpler) cleanup, just now with much more reasoning about
> why it isn't doing more :)
> 

Yeah I'm fine with your patch code as it stands right now.
In that case how about a second patch in the series to dev_warn in drivers/thunderbolt/acpi.c
right when the link is made if it's not set as trusted?  That should happen right during
tb_probe as you suggest then.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Mika Westerberg]

Hi Robin,

On Wed, Mar 16, 2022 at 07:17:57PM +0000, Robin Murphy wrote:
> The feeling I'm getting from all this is that if we've got as far as
> iommu_dma_protection_show() then it's really too late to meaningfully
> mitigate bad firmware.

Note, these are requirements from Microsoft in order for the system to
use the "Kernel DMA protection". Because of this, likelyhood of "bad
firmware" should be quite low since these systems ship with Windows
installed so they should get at least some soft of validation that this
actually works.

> We should be able to detect missing
> untrusted/external-facing properties as early as nhi_probe(), and if we
> could go into "continue at your own risk" mode right then *before* anything
> else happens, it all becomes a lot easier to reason about.

I think what we want is that the DMAR opt-in bit is set in the ACPI
tables and that we know the full IOMMU translation is happening for the
devices behind "external facing ports". If that's not the case the
iommu_dma_protection_show() should return 0 meaning the userspace can
ask the user whether the connected device is allowed to use DMA (e.g
PCIe is tunneled or not).

We do check for the DMAR bit in the Intel IOMMU code and we also do
check that there actually are PCIe ports marked external facing but we
could issue warning there if that's not the case. Similarly if the user
explicitly disabled the IOMMU translation. This can be done inside a new
IOMMU API that does something like the below pseudo-code:

#if IOMMU_ENABLED
bool iommu_dma_protected(struct device *dev)
{
	if (dmar_platform_optin() /* or the AMD equivalent */) {
		if (!iommu_present(...)) /* whatever is needed to check that the full translation is enabled */
			dev_warn(dev, "IOMMU protection disabled!");
		/*
		 * Look for the external facing ports. Should be at
		 * least 1 or issue warning.
		 */
		 ...

		return true;
	}

	return false;
}
#else
static inline bool iommu_dma_protected(struct device *dev)
{
	return false;
}
#endif

Then we can make iommu_dma_protection_show() to call this function.

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

[Mika Westerberg]

Hi Robin,

On Wed, Mar 16, 2022 at 07:17:57PM +0000, Robin Murphy wrote:
> The feeling I'm getting from all this is that if we've got as far as
> iommu_dma_protection_show() then it's really too late to meaningfully
> mitigate bad firmware.

Note, these are requirements from Microsoft in order for the system to
use the "Kernel DMA protection". Because of this, likelyhood of "bad
firmware" should be quite low since these systems ship with Windows
installed so they should get at least some soft of validation that this
actually works.

> We should be able to detect missing
> untrusted/external-facing properties as early as nhi_probe(), and if we
> could go into "continue at your own risk" mode right then *before* anything
> else happens, it all becomes a lot easier to reason about.

I think what we want is that the DMAR opt-in bit is set in the ACPI
tables and that we know the full IOMMU translation is happening for the
devices behind "external facing ports". If that's not the case the
iommu_dma_protection_show() should return 0 meaning the userspace can
ask the user whether the connected device is allowed to use DMA (e.g
PCIe is tunneled or not).

We do check for the DMAR bit in the Intel IOMMU code and we also do
check that there actually are PCIe ports marked external facing but we
could issue warning there if that's not the case. Similarly if the user
explicitly disabled the IOMMU translation. This can be done inside a new
IOMMU API that does something like the below pseudo-code:

#if IOMMU_ENABLED
bool iommu_dma_protected(struct device *dev)
{
	if (dmar_platform_optin() /* or the AMD equivalent */) {
		if (!iommu_present(...)) /* whatever is needed to check that the full translation is enabled */
			dev_warn(dev, "IOMMU protection disabled!");
		/*
		 * Look for the external facing ports. Should be at
		 * least 1 or issue warning.
		 */
		 ...

		return true;
	}

	return false;
}
#else
static inline bool iommu_dma_protected(struct device *dev)
{
	return false;
}
#endif

Then we can make iommu_dma_protection_show() to call this function.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Robin Murphy]

On 2022-03-17 08:08, Mika Westerberg wrote:
> Hi Robin,
> 
> On Wed, Mar 16, 2022 at 07:17:57PM +0000, Robin Murphy wrote:
>> The feeling I'm getting from all this is that if we've got as far as
>> iommu_dma_protection_show() then it's really too late to meaningfully
>> mitigate bad firmware.
> 
> Note, these are requirements from Microsoft in order for the system to
> use the "Kernel DMA protection". Because of this, likelyhood of "bad
> firmware" should be quite low since these systems ship with Windows
> installed so they should get at least some soft of validation that this
> actually works.
> 
>> We should be able to detect missing
>> untrusted/external-facing properties as early as nhi_probe(), and if we
>> could go into "continue at your own risk" mode right then *before* anything
>> else happens, it all becomes a lot easier to reason about.
> 
> I think what we want is that the DMAR opt-in bit is set in the ACPI
> tables and that we know the full IOMMU translation is happening for the
> devices behind "external facing ports". If that's not the case the
> iommu_dma_protection_show() should return 0 meaning the userspace can
> ask the user whether the connected device is allowed to use DMA (e.g
> PCIe is tunneled or not).

Ah, if it's safe to just say "no protection" in the case that we don't 
know for sure, that's even better. Clearly I hadn't quite grasped that 
aspect of the usage model, thanks for the nudge!

> We do check for the DMAR bit in the Intel IOMMU code and we also do
> check that there actually are PCIe ports marked external facing but we
> could issue warning there if that's not the case. Similarly if the user
> explicitly disabled the IOMMU translation. This can be done inside a new
> IOMMU API that does something like the below pseudo-code:
> 
> #if IOMMU_ENABLED
> bool iommu_dma_protected(struct device *dev)
> {
> 	if (dmar_platform_optin() /* or the AMD equivalent */) {
> 		if (!iommu_present(...)) /* whatever is needed to check that the full translation is enabled */
> 			dev_warn(dev, "IOMMU protection disabled!");
> 		/*
> 		 * Look for the external facing ports. Should be at
> 		 * least 1 or issue warning.
> 		 */
> 		 ...
> 
> 		return true;
> 	}
> 
> 	return false;
> }
> #else
> static inline bool iommu_dma_protected(struct device *dev)
> {
> 	return false;
> }
> #endif
> 
> Then we can make iommu_dma_protection_show() to call this function.

The problem that I've been trying to nail down here is that 
dmar_platform_optin() really doesn't mean much for us - I don't know how 
  Windows' IOMMU drivers work, but there's every chance it's not the 
same way as ours. The only material effect that dmar_platform_optin() 
has for us is to prevent the user from disabling the IOMMU driver 
altogether, and thus ensure that iommu_present() is true. Whether or not 
we can actually trust the IOMMU driver to provide reliable protection 
depends entirely on whether it knows the PCIe ports are external-facing. 
If not, we can only *definitely* know what the IOMMU driver will do for 
a given endpoint once that endpoint has appeared behind the port and 
iommu_probe_device() has decided what its default domain should be, and 
as far as I now understand, that's not an option for Thunderbolt since 
it can only happen *after* the tunnel has been authorised and created.

Much as I'm tempted to de-scope back to my IOMMU API cleanup and run 
away from the rest of the issue, I think I can crib enough from the 
existing code to attempt a reasonable complete fix, so let me give that 
a go...

Thanks,
Robin.

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

[Robin Murphy]

On 2022-03-17 08:08, Mika Westerberg wrote:
> Hi Robin,
> 
> On Wed, Mar 16, 2022 at 07:17:57PM +0000, Robin Murphy wrote:
>> The feeling I'm getting from all this is that if we've got as far as
>> iommu_dma_protection_show() then it's really too late to meaningfully
>> mitigate bad firmware.
> 
> Note, these are requirements from Microsoft in order for the system to
> use the "Kernel DMA protection". Because of this, likelyhood of "bad
> firmware" should be quite low since these systems ship with Windows
> installed so they should get at least some soft of validation that this
> actually works.
> 
>> We should be able to detect missing
>> untrusted/external-facing properties as early as nhi_probe(), and if we
>> could go into "continue at your own risk" mode right then *before* anything
>> else happens, it all becomes a lot easier to reason about.
> 
> I think what we want is that the DMAR opt-in bit is set in the ACPI
> tables and that we know the full IOMMU translation is happening for the
> devices behind "external facing ports". If that's not the case the
> iommu_dma_protection_show() should return 0 meaning the userspace can
> ask the user whether the connected device is allowed to use DMA (e.g
> PCIe is tunneled or not).

Ah, if it's safe to just say "no protection" in the case that we don't 
know for sure, that's even better. Clearly I hadn't quite grasped that 
aspect of the usage model, thanks for the nudge!

> We do check for the DMAR bit in the Intel IOMMU code and we also do
> check that there actually are PCIe ports marked external facing but we
> could issue warning there if that's not the case. Similarly if the user
> explicitly disabled the IOMMU translation. This can be done inside a new
> IOMMU API that does something like the below pseudo-code:
> 
> #if IOMMU_ENABLED
> bool iommu_dma_protected(struct device *dev)
> {
> 	if (dmar_platform_optin() /* or the AMD equivalent */) {
> 		if (!iommu_present(...)) /* whatever is needed to check that the full translation is enabled */
> 			dev_warn(dev, "IOMMU protection disabled!");
> 		/*
> 		 * Look for the external facing ports. Should be at
> 		 * least 1 or issue warning.
> 		 */
> 		 ...
> 
> 		return true;
> 	}
> 
> 	return false;
> }
> #else
> static inline bool iommu_dma_protected(struct device *dev)
> {
> 	return false;
> }
> #endif
> 
> Then we can make iommu_dma_protection_show() to call this function.

The problem that I've been trying to nail down here is that 
dmar_platform_optin() really doesn't mean much for us - I don't know how 
  Windows' IOMMU drivers work, but there's every chance it's not the 
same way as ours. The only material effect that dmar_platform_optin() 
has for us is to prevent the user from disabling the IOMMU driver 
altogether, and thus ensure that iommu_present() is true. Whether or not 
we can actually trust the IOMMU driver to provide reliable protection 
depends entirely on whether it knows the PCIe ports are external-facing. 
If not, we can only *definitely* know what the IOMMU driver will do for 
a given endpoint once that endpoint has appeared behind the port and 
iommu_probe_device() has decided what its default domain should be, and 
as far as I now understand, that's not an option for Thunderbolt since 
it can only happen *after* the tunnel has been authorised and created.

Much as I'm tempted to de-scope back to my IOMMU API cleanup and run 
away from the rest of the issue, I think I can crib enough from the 
existing code to attempt a reasonable complete fix, so let me give that 
a go...

Thanks,
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Mika Westerberg]

Hi Robin,

On Thu, Mar 17, 2022 at 01:42:56PM +0000, Robin Murphy wrote:
> On 2022-03-17 08:08, Mika Westerberg wrote:
> > Hi Robin,
> > 
> > On Wed, Mar 16, 2022 at 07:17:57PM +0000, Robin Murphy wrote:
> > > The feeling I'm getting from all this is that if we've got as far as
> > > iommu_dma_protection_show() then it's really too late to meaningfully
> > > mitigate bad firmware.
> > 
> > Note, these are requirements from Microsoft in order for the system to
> > use the "Kernel DMA protection". Because of this, likelyhood of "bad
> > firmware" should be quite low since these systems ship with Windows
> > installed so they should get at least some soft of validation that this
> > actually works.
> > 
> > > We should be able to detect missing
> > > untrusted/external-facing properties as early as nhi_probe(), and if we
> > > could go into "continue at your own risk" mode right then *before* anything
> > > else happens, it all becomes a lot easier to reason about.
> > 
> > I think what we want is that the DMAR opt-in bit is set in the ACPI
> > tables and that we know the full IOMMU translation is happening for the
> > devices behind "external facing ports". If that's not the case the
> > iommu_dma_protection_show() should return 0 meaning the userspace can
> > ask the user whether the connected device is allowed to use DMA (e.g
> > PCIe is tunneled or not).
> 
> Ah, if it's safe to just say "no protection" in the case that we don't know
> for sure, that's even better. Clearly I hadn't quite grasped that aspect of
> the usage model, thanks for the nudge!

There is some documentation here too, hope it is helpful:

https://docs.kernel.org/admin-guide/thunderbolt.html

> > We do check for the DMAR bit in the Intel IOMMU code and we also do
> > check that there actually are PCIe ports marked external facing but we
> > could issue warning there if that's not the case. Similarly if the user
> > explicitly disabled the IOMMU translation. This can be done inside a new
> > IOMMU API that does something like the below pseudo-code:
> > 
> > #if IOMMU_ENABLED
> > bool iommu_dma_protected(struct device *dev)
> > {
> > 	if (dmar_platform_optin() /* or the AMD equivalent */) {
> > 		if (!iommu_present(...)) /* whatever is needed to check that the full translation is enabled */
> > 			dev_warn(dev, "IOMMU protection disabled!");
> > 		/*
> > 		 * Look for the external facing ports. Should be at
> > 		 * least 1 or issue warning.
> > 		 */
> > 		 ...
> > 
> > 		return true;
> > 	}
> > 
> > 	return false;
> > }
> > #else
> > static inline bool iommu_dma_protected(struct device *dev)
> > {
> > 	return false;
> > }
> > #endif
> > 
> > Then we can make iommu_dma_protection_show() to call this function.
> 
> The problem that I've been trying to nail down here is that
> dmar_platform_optin() really doesn't mean much for us - I don't know how
> Windows' IOMMU drivers work, but there's every chance it's not the same way
> as ours. The only material effect that dmar_platform_optin() has for us is
> to prevent the user from disabling the IOMMU driver altogether, and thus
> ensure that iommu_present() is true. Whether or not we can actually trust
> the IOMMU driver to provide reliable protection depends entirely on whether
> it knows the PCIe ports are external-facing. If not, we can only
> *definitely* know what the IOMMU driver will do for a given endpoint once
> that endpoint has appeared behind the port and iommu_probe_device() has
> decided what its default domain should be, and as far as I now understand,
> that's not an option for Thunderbolt since it can only happen *after* the
> tunnel has been authorised and created.

That's correct. We do know the PCIe root/downstream ports (the external
facing ones) that host the tunneled PCIe topology but rest will appear
dynamically after the connection manager established the protocol
tunnel.

> Much as I'm tempted to de-scope back to my IOMMU API cleanup and run away
> from the rest of the issue, I think I can crib enough from the existing code
> to attempt a reasonable complete fix, so let me give that a go...

Sure ;-)

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

[Mika Westerberg]

Hi Robin,

On Thu, Mar 17, 2022 at 01:42:56PM +0000, Robin Murphy wrote:
> On 2022-03-17 08:08, Mika Westerberg wrote:
> > Hi Robin,
> > 
> > On Wed, Mar 16, 2022 at 07:17:57PM +0000, Robin Murphy wrote:
> > > The feeling I'm getting from all this is that if we've got as far as
> > > iommu_dma_protection_show() then it's really too late to meaningfully
> > > mitigate bad firmware.
> > 
> > Note, these are requirements from Microsoft in order for the system to
> > use the "Kernel DMA protection". Because of this, likelyhood of "bad
> > firmware" should be quite low since these systems ship with Windows
> > installed so they should get at least some soft of validation that this
> > actually works.
> > 
> > > We should be able to detect missing
> > > untrusted/external-facing properties as early as nhi_probe(), and if we
> > > could go into "continue at your own risk" mode right then *before* anything
> > > else happens, it all becomes a lot easier to reason about.
> > 
> > I think what we want is that the DMAR opt-in bit is set in the ACPI
> > tables and that we know the full IOMMU translation is happening for the
> > devices behind "external facing ports". If that's not the case the
> > iommu_dma_protection_show() should return 0 meaning the userspace can
> > ask the user whether the connected device is allowed to use DMA (e.g
> > PCIe is tunneled or not).
> 
> Ah, if it's safe to just say "no protection" in the case that we don't know
> for sure, that's even better. Clearly I hadn't quite grasped that aspect of
> the usage model, thanks for the nudge!

There is some documentation here too, hope it is helpful:

https://docs.kernel.org/admin-guide/thunderbolt.html

> > We do check for the DMAR bit in the Intel IOMMU code and we also do
> > check that there actually are PCIe ports marked external facing but we
> > could issue warning there if that's not the case. Similarly if the user
> > explicitly disabled the IOMMU translation. This can be done inside a new
> > IOMMU API that does something like the below pseudo-code:
> > 
> > #if IOMMU_ENABLED
> > bool iommu_dma_protected(struct device *dev)
> > {
> > 	if (dmar_platform_optin() /* or the AMD equivalent */) {
> > 		if (!iommu_present(...)) /* whatever is needed to check that the full translation is enabled */
> > 			dev_warn(dev, "IOMMU protection disabled!");
> > 		/*
> > 		 * Look for the external facing ports. Should be at
> > 		 * least 1 or issue warning.
> > 		 */
> > 		 ...
> > 
> > 		return true;
> > 	}
> > 
> > 	return false;
> > }
> > #else
> > static inline bool iommu_dma_protected(struct device *dev)
> > {
> > 	return false;
> > }
> > #endif
> > 
> > Then we can make iommu_dma_protection_show() to call this function.
> 
> The problem that I've been trying to nail down here is that
> dmar_platform_optin() really doesn't mean much for us - I don't know how
> Windows' IOMMU drivers work, but there's every chance it's not the same way
> as ours. The only material effect that dmar_platform_optin() has for us is
> to prevent the user from disabling the IOMMU driver altogether, and thus
> ensure that iommu_present() is true. Whether or not we can actually trust
> the IOMMU driver to provide reliable protection depends entirely on whether
> it knows the PCIe ports are external-facing. If not, we can only
> *definitely* know what the IOMMU driver will do for a given endpoint once
> that endpoint has appeared behind the port and iommu_probe_device() has
> decided what its default domain should be, and as far as I now understand,
> that's not an option for Thunderbolt since it can only happen *after* the
> tunnel has been authorised and created.

That's correct. We do know the PCIe root/downstream ports (the external
facing ones) that host the tunneled PCIe topology but rest will appear
dynamically after the connection manager established the protocol
tunnel.

> Much as I'm tempted to de-scope back to my IOMMU API cleanup and run away
> from the rest of the issue, I think I can crib enough from the existing code
> to attempt a reasonable complete fix, so let me give that a go...

Sure ;-)
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Mika Westerberg]

Hi Mario,

On Wed, Mar 16, 2022 at 06:34:51PM +0000, Limonciello, Mario wrote:
> > Might it be reasonable for the Thunderbolt core to check early on if any
> > tunnelled ports are not marked as external facing, and if so just tell
> > the user that iommu_dma_protection is off the table and anything they
> > authorise is at their own risk?
> > 
> > Robin.
> 
> How about in iommu_dma_protection_show to just check that all the device
> links to the NHI are marked as untrusted?

Actually this does not work either because we have pre-USB4 systems out
there that are using firmware based connection manager and do not set
the "device links" (as it is only needed for USB4 software based
connection manager systems).

So only thing we can use is the ->external_facing (and ->untrusted) as
those exists in all these systems (well assuming the BIOS provided them
but this is Microsoft requirement in the same way with the DMAR bit).

[For those who are not familiar with the connection manager, it is the
 software or firmware that actually creates the tunnels over the
 Thunderbolt/USB4 fabric. In Intel systems up to Alder Lake it used to be
 firmware based, and from Alder Lake and beyond it is software based
 meaning that the Linux Thunderbolt driver creates the tunnels. Apple
 systems have been software based from the beginnning.]

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

[Mika Westerberg]

Hi Mario,

On Wed, Mar 16, 2022 at 06:34:51PM +0000, Limonciello, Mario wrote:
> > Might it be reasonable for the Thunderbolt core to check early on if any
> > tunnelled ports are not marked as external facing, and if so just tell
> > the user that iommu_dma_protection is off the table and anything they
> > authorise is at their own risk?
> > 
> > Robin.
> 
> How about in iommu_dma_protection_show to just check that all the device
> links to the NHI are marked as untrusted?

Actually this does not work either because we have pre-USB4 systems out
there that are using firmware based connection manager and do not set
the "device links" (as it is only needed for USB4 software based
connection manager systems).

So only thing we can use is the ->external_facing (and ->untrusted) as
those exists in all these systems (well assuming the BIOS provided them
but this is Microsoft requirement in the same way with the DMAR bit).

[For those who are not familiar with the connection manager, it is the
 software or firmware that actually creates the tunnels over the
 Thunderbolt/USB4 fabric. In Intel systems up to Alder Lake it used to be
 firmware based, and from Alder Lake and beyond it is software based
 meaning that the Linux Thunderbolt driver creates the tunnels. Apple
 systems have been software based from the beginnning.]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Mika Westerberg <mika.westerberg@linux.intel.com>
> Sent: Wednesday, March 16, 2022 07:45
> To: Robin Murphy <robin.murphy@arm.com>
> Cc: andreas.noever@gmail.com; michael.jamet@intel.com;
> YehezkelShB@gmail.com; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org; iommu@lists.linux-foundation.org; Limonciello,
> Mario <Mario.Limonciello@amd.com>; hch@lst.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.
> >

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

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

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@amd.com>

> >
> >  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);

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

[Limonciello, Mario via iommu]

[Public]



> -----Original Message-----
> From: Mika Westerberg <mika.westerberg@linux.intel.com>
> Sent: Wednesday, March 16, 2022 07:45
> To: Robin Murphy <robin.murphy@arm.com>
> Cc: andreas.noever@gmail.com; michael.jamet@intel.com;
> YehezkelShB@gmail.com; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org; iommu@lists.linux-foundation.org; Limonciello,
> Mario <Mario.Limonciello@amd.com>; hch@lst.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.
> >

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

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

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@amd.com>

> >
> >  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);
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu