Message-ID: <BL1PR12MB515783C0F998169D49D92A55E2129@BL1PR12MB5157.namprd12.prod.outlook.com>
Date:   Thu, 17 Mar 2022 17:09:25 +0000
From:   "Limonciello, Mario" <Mario.Limonciello@....com>
To:     Robin Murphy <robin.murphy@....com>,
        "andreas.noever@...il.com" <andreas.noever@...il.com>,
        "michael.jamet@...el.com" <michael.jamet@...el.com>,
        "mika.westerberg@...ux.intel.com" <mika.westerberg@...ux.intel.com>,
        "YehezkelShB@...il.com" <YehezkelShB@...il.com>
CC:     "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>
Subject: RE: [PATCH] thunderbolt: Make iommu_dma_protection more accurate

[Public]



> -----Original Message-----
> From: Robin Murphy <robin.murphy@....com>
> Sent: Thursday, March 17, 2022 11:17
> To: andreas.noever@...il.com; michael.jamet@...el.com;
> mika.westerberg@...ux.intel.com; YehezkelShB@...il.com
> Cc: linux-usb@...r.kernel.org; linux-kernel@...r.kernel.org;
> iommu@...ts.linux-foundation.org; linux-pci@...r.kernel.org; Limonciello,
> Mario <Mario.Limonciello@....com>
> 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@....com>
> Signed-off-by: Robin Murphy <robin.murphy@....com>
> ---
> 
> 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&amp;data=04%7C01%7Cmario.limo
> nciello%40amd.com%7C14f5afbba9624b4d0ef508da08319b2a%7C3dd8961fe4
> 884e608e11a82d994e183d%7C0%7C0%7C637831306409535091%7CUnknown
> %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> WwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=9xJ9bNT3pR3YhqOOqiJtGv94ln2
> IJSvrXllbPZjTI6M%3D&amp;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&amp;data=04%7C01%7Cmario.limonciello%40amd
> .com%7C14f5afbba9624b4d0ef508da08319b2a%7C3dd8961fe4884e608e11a8
> 2d994e183d%7C0%7C0%7C637831306409535091%7CUnknown%7CTWFpbGZs
> b3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> 0%3D%7C3000&amp;sdata=wSbpjpPQk8ulX8ifOTt%2BNMdO5svwQceQthyca
> txzScI%3D&amp;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

Powered by blists - more mailing lists