| Message ID | b4356b228db9cb88d12db6559e28714ce26e022e.1647429348.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | thunderbolt: Stop using iommu_present() | expand |
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".
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.
[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);
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?
[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).
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.
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.
[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.
[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&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. 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.
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.
[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).
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.
[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.
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.]
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.
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.
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 ;-)
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);
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(-)