[Public] > -----Original Message----- > From: Limonciello, Mario > Sent: Thursday, March 17, 2022 12:09 > To: Robin Murphy <robin.murphy@xxxxxxx>; andreas.noever@xxxxxxxxx; > michael.jamet@xxxxxxxxx; mika.westerberg@xxxxxxxxxxxxxxx; > YehezkelShB@xxxxxxxxx > Cc: linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx > Subject: RE: [PATCH] thunderbolt: Make iommu_dma_protection more > accurate > > [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? Here is a proposal on top of what you did for this. The idea being check the ports right when the links are made if they exist (all the new USB4 stuff) and then check all siblings on TBT3 stuff. diff --git a/drivers/thunderbolt/acpi.c b/drivers/thunderbolt/acpi.c index 79b5abf9d042..89432456dbea 100644 --- a/drivers/thunderbolt/acpi.c +++ b/drivers/thunderbolt/acpi.c @@ -14,6 +14,7 @@ static acpi_status tb_acpi_add_link(acpi_handle handle, u32 level, void *data, void **return_value) { + enum nhi_iommu_status iommu_status = IOMMU_UNKNOWN; struct fwnode_reference_args args; struct fwnode_handle *fwnode; struct tb_nhi *nhi = data; @@ -91,6 +92,8 @@ static acpi_status tb_acpi_add_link(acpi_handle handle, u32 level, void *data, if (link) { dev_dbg(&nhi->pdev->dev, "created link from %s\n", dev_name(&pdev->dev)); + if (iommu_status != IOMMU_DISABLED) + iommu_status = nhi_check_iommu_for_port(pdev); } else { dev_warn(&nhi->pdev->dev, "device link creation from %s failed\n", dev_name(&pdev->dev)); @@ -101,6 +104,7 @@ static acpi_status tb_acpi_add_link(acpi_handle handle, u32 level, void *data, out_put: fwnode_handle_put(args.fwnode); + nhi->iommu_dma_protection = (iommu_status == IOMMU_ENABLED); return AE_OK; } diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c index e12c2e266741..b5eb0cab392f 100644 --- a/drivers/thunderbolt/nhi.c +++ b/drivers/thunderbolt/nhi.c @@ -1103,10 +1103,30 @@ static void nhi_check_quirks(struct tb_nhi *nhi) nhi->quirks |= QUIRK_AUTO_CLEAR_INT; } +enum nhi_iommu_status nhi_check_iommu_for_port(struct pci_dev *pdev) +{ + if (!pci_is_pcie(pdev) || + !(pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT || + pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM)) { + return IOMMU_UNKNOWN; + } + + if (!device_iommu_mapped(&pdev->dev)) { + return IOMMU_DISABLED; + } + + if (!pdev->untrusted) { + dev_info(&pdev->dev, + "Assuming unreliable Kernel DMA protection\n"); + return IOMMU_DISABLED; + } + return IOMMU_ENABLED; +} + static void nhi_check_iommu(struct tb_nhi *nhi) { - struct pci_dev *pdev; - bool port_ok = false; + enum nhi_iommu_status iommu_status = nhi->iommu_dma_protection ? + IOMMU_ENABLED : IOMMU_UNKNOWN; /* * Check for sibling devices that look like they should be our @@ -1117,23 +1137,13 @@ static void nhi_check_iommu(struct tb_nhi *nhi) * 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; - - 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; + if (iommu_status == IOMMU_UNKNOWN) { + struct pci_dev *pdev; + for_each_pci_bridge(pdev, nhi->pdev->bus) + if (iommu_status != IOMMU_DISABLED) + iommu_status = nhi_check_iommu_for_port(pdev); } - nhi->iommu_dma_protection = port_ok; + nhi->iommu_dma_protection = (iommu_status == IOMMU_ENABLED); } static int nhi_init_msi(struct tb_nhi *nhi) diff --git a/drivers/thunderbolt/nhi.h b/drivers/thunderbolt/nhi.h index 69083aab2736..1622d49b1763 100644 --- a/drivers/thunderbolt/nhi.h +++ b/drivers/thunderbolt/nhi.h @@ -11,6 +11,13 @@ #include <linux/thunderbolt.h> +enum nhi_iommu_status { + IOMMU_UNKNOWN, + IOMMU_DISABLED, + IOMMU_ENABLED, +}; +enum nhi_iommu_status nhi_check_iommu_for_port(struct pci_dev *pdev); + enum nhi_fw_mode { NHI_FW_SAFE_MODE, NHI_FW_AUTH_MODE, > > > + 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
