[Public] > -----Original Message----- > From: Robin Murphy <robin.murphy@xxxxxxx> > Sent: Thursday, March 17, 2022 11:17 > To: andreas.noever@xxxxxxxxx; michael.jamet@xxxxxxxxx; > mika.westerberg@xxxxxxxxxxxxxxx; YehezkelShB@xxxxxxxxx > Cc: linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; Limonciello, > Mario <Mario.Limonciello@xxxxxxx> > Subject: [PATCH] thunderbolt: Make iommu_dma_protection more accurate > > Between me trying to get rid of iommu_present() and Mario wanting to > support the AMD equivalent of DMAR_PLATFORM_OPT_IN, scrutiny has > shown > that the iommu_dma_protection attribute is being far too optimistic. > 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(s) > we care about. Furthermore, all that DMAR_PLATFORM_OPT_IN really does > is tell us that memory was protected before the kernel was loaded, and > prevent the user from disabling the intel-iommu driver entirely. What > actually matters is whether we trust individual devices, based on the > "external facing" property that we expect firmware to describe for > Thunderbolt ports. > > Avoid false positives by looking as close as possible to the same PCI > topology that the IOMMU layer will consider once a Thunderbolt endpoint > appears. Crucially, we can't assume that IOMMU translation being enabled > for any reason is sufficient on its own; full (expensive) DMA protection > will still only be imposed on untrusted devices. > > CC: Mario Limonciello <mario.limonciello@xxxxxxx> > Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx> > --- > > This supersedes my previous attempt just trying to replace > iommu_present() at [1], further to the original discussion at [2]. > > [1] > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore. > kernel.org%2Flinux- > iommu%2FBL1PR12MB515799C0BE396377DBBEF055E2119%40BL1PR12MB515 > 7.namprd12.prod.outlook.com%2FT%2F&data=04%7C01%7Cmario.limo > nciello%40amd.com%7C14f5afbba9624b4d0ef508da08319b2a%7C3dd8961fe4 > 884e608e11a82d994e183d%7C0%7C0%7C637831306409535091%7CUnknown > %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha > WwiLCJXVCI6Mn0%3D%7C3000&sdata=9xJ9bNT3pR3YhqOOqiJtGv94ln2 > IJSvrXllbPZjTI6M%3D&reserved=0 > [2] > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore. > kernel.org%2Flinux-iommu%2F202203160844.lKviWR1Q- > lkp%40intel.com%2FT%2F&data=04%7C01%7Cmario.limonciello%40amd > .com%7C14f5afbba9624b4d0ef508da08319b2a%7C3dd8961fe4884e608e11a8 > 2d994e183d%7C0%7C0%7C637831306409535091%7CUnknown%7CTWFpbGZs > b3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn > 0%3D%7C3000&sdata=wSbpjpPQk8ulX8ifOTt%2BNMdO5svwQceQthyca > txzScI%3D&reserved=0 > > drivers/thunderbolt/domain.c | 12 +++--------- > drivers/thunderbolt/nhi.c | 35 > +++++++++++++++++++++++++++++++++++ > include/linux/thunderbolt.h | 2 ++ > 3 files changed, 40 insertions(+), 9 deletions(-) > > diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c > index 7018d959f775..d5c825e84ac8 100644 > --- a/drivers/thunderbolt/domain.c > +++ b/drivers/thunderbolt/domain.c > @@ -7,9 +7,7 @@ > */ > > #include <linux/device.h> > -#include <linux/dmar.h> > #include <linux/idr.h> > -#include <linux/iommu.h> > #include <linux/module.h> > #include <linux/pm_runtime.h> > #include <linux/slab.h> > @@ -257,13 +255,9 @@ static ssize_t iommu_dma_protection_show(struct > device *dev, > struct device_attribute *attr, > char *buf) > { > - /* > - * 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. > - */ > - return sprintf(buf, "%d\n", > - iommu_present(&pci_bus_type) && > dmar_platform_optin()); > + struct tb *tb = container_of(dev, struct tb, dev); > + > + return sprintf(buf, "%d\n", tb->nhi->iommu_dma_protection); > } > static DEVICE_ATTR_RO(iommu_dma_protection); > > diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c > index c73da0532be4..e12c2e266741 100644 > --- a/drivers/thunderbolt/nhi.c > +++ b/drivers/thunderbolt/nhi.c > @@ -14,6 +14,7 @@ > #include <linux/errno.h> > #include <linux/pci.h> > #include <linux/interrupt.h> > +#include <linux/iommu.h> > #include <linux/module.h> > #include <linux/delay.h> > #include <linux/property.h> > @@ -1102,6 +1103,39 @@ static void nhi_check_quirks(struct tb_nhi *nhi) > nhi->quirks |= QUIRK_AUTO_CLEAR_INT; > } > > +static void nhi_check_iommu(struct tb_nhi *nhi) > +{ > + struct pci_dev *pdev; > + bool port_ok = false; > + > + /* > + * Check for sibling devices that look like they should be our > + * tunnelled ports. We can reasonably assume that if an IOMMU is > + * managing the bridge it will manage any future devices beyond it > + * too. If firmware has described a port as external-facing as > + * expected then we can trust the IOMMU layer to enforce isolation; > + * otherwise even if translation is enabled for existing devices it > + * may potentially be overridden for a future tunnelled endpoint. > + */ > + for_each_pci_bridge(pdev, nhi->pdev->bus) { > + if (!pci_is_pcie(pdev) || > + !(pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT || > + pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM)) > + continue; > + Unfortunately I don't think this logic holds for the topology I see. Here is the NHI on a system I have here: $ lspci -vvv -s 64:00.5 64:00.5 USB controller: Advanced Micro Devices, Inc. [AMD] Device 162e (prog-if 40) Subsystem: Advanced Micro Devices, Inc. [AMD] Device 162e Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+ Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Latency: 0, Cache Line Size: 32 bytes Interrupt: pin A routed to IRQ 42 Region 0: Memory at b0500000 (64-bit, non-prefetchable) [size=512K] Capabilities: <access denied> Kernel driver in use: thunderbolt Kernel modules: thunderbolt The links it makes (from those _DSD) are: $ ls /sys/bus/pci/drivers/thunderbolt/0000\:64\:00.5/ | grep consumer consumer:pci:0000:00:03.1 consumer:pci:0000:64:00.3 $ lspci -s 64:00.3 64:00.3 USB controller: Advanced Micro Devices, Inc. [AMD] Device 15d6 $ lspci -s 00:03.1 00:03.1 PCI bridge: Advanced Micro Devices, Inc. [AMD] Device 14cd Looking at the topology the PCIE root port for tunneling (00:03.1) isn't actually on the same bridge. $ lspci -t -[0000:00]-+-00.0 +-00.2 +-01.0 +-01.2-[01]----00.0 +-02.0 +-02.4-[02]----00.0 +-03.0 +-03.1-[03-32]-- +-04.0 +-04.1-[33-62]-- +-08.0 +-08.1-[63]--+-00.0 | +-00.1 | +-00.2 | +-00.3 | +-00.4 | +-00.5 | +-00.6 | \-00.7 +-08.3-[64]--+-00.0 | +-00.3 | +-00.4 | +-00.5 | \-00.6 +-14.0 +-14.3 +-18.0 +-18.1 +-18.2 +-18.3 +-18.4 +-18.5 +-18.6 \-18.7 How about in this function to have two cases: * the one that looks at links * and if no links then the logic you have in place? > + if (!device_iommu_mapped(&pdev->dev)) > + return; > + > + if (!pdev->untrusted) { > + dev_info(&nhi->pdev->dev, > + "Assuming unreliable Kernel DMA > protection\n"); > + return; > + } > + port_ok = true; > + } > + nhi->iommu_dma_protection = port_ok; > +} > + > static int nhi_init_msi(struct tb_nhi *nhi) > { > struct pci_dev *pdev = nhi->pdev; > @@ -1219,6 +1253,7 @@ static int nhi_probe(struct pci_dev *pdev, const > struct pci_device_id *id) > return -ENOMEM; > > nhi_check_quirks(nhi); > + nhi_check_iommu(nhi); > > res = nhi_init_msi(nhi); > if (res) { > diff --git a/include/linux/thunderbolt.h b/include/linux/thunderbolt.h > index 124e13cb1469..7a8ad984e651 100644 > --- a/include/linux/thunderbolt.h > +++ b/include/linux/thunderbolt.h > @@ -465,6 +465,7 @@ static inline struct tb_xdomain > *tb_service_parent(struct tb_service *svc) > * @msix_ida: Used to allocate MSI-X vectors for rings > * @going_away: The host controller device is about to disappear so when > * this flag is set, avoid touching the hardware anymore. > + * @iommu_dma_protection: An IOMMU will isolate external-facing ports. > * @interrupt_work: Work scheduled to handle ring interrupt when no > * MSI-X is used. > * @hop_count: Number of rings (end point hops) supported by NHI. > @@ -479,6 +480,7 @@ struct tb_nhi { > struct tb_ring **rx_rings; > struct ida msix_ida; > bool going_away; > + bool iommu_dma_protection; > struct work_struct interrupt_work; > u32 hop_count; > unsigned long quirks; > -- > 2.28.0.dirty
