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 <[email protected]>
---
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
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 <[email protected]>
>> ---
>>
>> 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.
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.
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 <[email protected]>
> ---
>
> 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".
[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.
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.