[PATCH 0/4] PCI / iommu / thunderbolt: IOMMU based DMA protection

[PATCH 0/4] PCI / iommu / thunderbolt: IOMMU based DMA protection

[Mika Westerberg]

Hi all,

Recent systems shipping with Windows 10 version 1803 or newer may be
utilizing IOMMU to prevent DMA attacks via Thunderbolt ports. This is
different from the previous security level based scheme because the
connected device cannot access system memory outside of the regions
allocated for it by the driver.

When enabled the BIOS makes sure no device can do DMA outside of RMRR
(Reserved Memory Region Record) regions. This means that during OS boot,
before it enables IOMMU, none of the connected devices can bypass DMA
protection for instance by overwriting the data structures used by the
IOMMU. The BIOS communicates support for this to the OS by setting a new
bit in ACPI DMAR table [1].

Because these systems utilize an IOMMU to block possible DMA attacks,
typically (but not always) the Thunderbolt security level is set to "none"
which means that all PCIe devices are immediately usable. This also means
that Linux needs to follow Windows 10 and enable IOMMU automatically when
running on such system otherwise connected devices can read/write system
memory pretty much without any restrictions.

Since there is a way to identify PCIe root ports that are "external facing"
we can put all internal devices to pass through (identity mapping) mode and
only external devices need to go through full IOMMU mappings.

We also make sure PCIe ATS (Address Translation Service) is not enabled for
external devices because it could be used to bypass IOMMU completely as
explained in the changelog of patch 3/4.

Finally we expose this information to userspace so tools such as bolt can
do more accurate decision whether or not authorize the connected device.

[1] https://software.intel.com/sites/default/files/managed/c5/15/vt-directed-io-spec.pdf

Lu Baolu (1):
  iommu/vt-d: Force IOMMU on for platform opt in hint

Mika Westerberg (3):
  PCI / ACPI: Identify external PCI devices
  iommu/vt-d: Do not enable ATS for external devices
  thunderbolt: Export IOMMU based DMA protection support to userspace

 .../ABI/testing/sysfs-bus-thunderbolt         |  9 +++
 Documentation/admin-guide/thunderbolt.rst     | 23 ++++++++
 drivers/acpi/property.c                       |  3 +
 drivers/iommu/dmar.c                          | 25 ++++++++
 drivers/iommu/intel-iommu.c                   | 58 ++++++++++++++++++-
 drivers/pci/pci-acpi.c                        | 13 +++++
 drivers/pci/probe.c                           | 23 ++++++++
 drivers/thunderbolt/domain.c                  | 17 ++++++
 include/linux/dmar.h                          |  8 +++
 include/linux/pci.h                           |  1 +
 10 files changed, 177 insertions(+), 3 deletions(-)

-- 
2.19.1

[PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Mika Westerberg]

Recent systems shipping Windows 10 version 1803 or later may support
IOMMU based DMA protection. This means that the platform utilizes IOMMU
to prevent DMA attacks over externally exposed PCIe root ports
(typically Thunderbolt ports)

The system BIOS marks these PCIe root ports as being externally facing
ports by implementing following ACPI _DSD under the root port in
question:

  Name (_DSD, Package () {
      ToUUID ("efcc06cc-73ac-4bc3-bff0-76143807c389"),
      Package () {
          Package () {"ExternalFacingPort", 1},
	  Package () {"UID", 0 }
      }
  })

This is documented [1].

To make it possible for IOMMU code to identify these external devices,
we look up for this property and mark all children devices (including
the root port itself) with a new flag (->is_external).

[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-externally-exposed-pcie-root-ports

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/acpi/property.c |  3 +++
 drivers/pci/pci-acpi.c  | 13 +++++++++++++
 drivers/pci/probe.c     | 23 +++++++++++++++++++++++
 include/linux/pci.h     |  1 +
 4 files changed, 40 insertions(+)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 8c7c4583b52d..4bdad32f62c8 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -31,6 +31,9 @@ static const guid_t prp_guids[] = {
 	/* Hotplug in D3 GUID: 6211e2c0-58a3-4af3-90e1-927a4e0c55a4 */
 	GUID_INIT(0x6211e2c0, 0x58a3, 0x4af3,
 		  0x90, 0xe1, 0x92, 0x7a, 0x4e, 0x0c, 0x55, 0xa4),
+	/* External facing port GUID: efcc06cc-73ac-4bc3-bff0-76143807c389 */
+	GUID_INIT(0xefcc06cc, 0x73ac, 0x4bc3,
+		  0xbf, 0xf0, 0x76, 0x14, 0x38, 0x07, 0xc3, 0x89),
 };
 
 static const guid_t ads_guid =
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 2a4aa6468579..fc193279d24d 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -789,6 +789,18 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
 	ACPI_FREE(obj);
 }
 
+static void pci_acpi_set_external(struct pci_dev *dev)
+{
+	u8 val;
+
+	if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
+		return;
+	if (device_property_read_u8(&dev->dev, "ExternalFacingPort", &val))
+		return;
+
+	dev->is_external = val == 1;
+}
+
 static void pci_acpi_setup(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
@@ -803,6 +815,7 @@ static void pci_acpi_setup(struct device *dev)
 		set_dev_node(dev, node);
 
 	pci_acpi_optimize_delay(pci_dev, adev->handle);
+	pci_acpi_set_external(pci_dev);
 
 	pci_acpi_add_pm_notifier(adev, pci_dev);
 	if (!adev->wakeup.flags.valid)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index b1c05b5054a0..f1db195a4a90 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1378,6 +1378,27 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
 	}
 }
 
+static void set_pcie_external(struct pci_dev *dev)
+{
+	struct pci_dev *parent;
+
+	/*
+	 * Walk up the device hierarchy and check for any upstream
+	 * bridge that has is_external_facing set to true. This means
+	 * the hierarchy is below PCIe port that is exposed externally
+	 * (such as Thunderbolt).
+	 */
+	parent = pci_upstream_bridge(dev);
+	while (parent) {
+		if (parent->is_external) {
+			dev->is_external = true;
+			break;
+		}
+
+		parent = pci_upstream_bridge(parent);
+	}
+}
+
 /**
  * pci_ext_cfg_is_aliased - Is ext config space just an alias of std config?
  * @dev: PCI device
@@ -1638,6 +1659,8 @@ int pci_setup_device(struct pci_dev *dev)
 	/* Need to have dev->cfg_size ready */
 	set_pcie_thunderbolt(dev);
 
+	set_pcie_external(dev);
+
 	/* "Unknown power state" */
 	dev->current_state = PCI_UNKNOWN;
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 11c71c4ecf75..e1c0e032da55 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -396,6 +396,7 @@ struct pci_dev {
 	unsigned int	is_hotplug_bridge:1;
 	unsigned int	shpc_managed:1;		/* SHPC owned by shpchp */
 	unsigned int	is_thunderbolt:1;	/* Thunderbolt controller */
+	unsigned int	is_external:1;		/* External PCIe device */
 	unsigned int	__aer_firmware_first_valid:1;
 	unsigned int	__aer_firmware_first:1;
 	unsigned int	broken_intx_masking:1;	/* INTx masking can't be used */
-- 
2.19.1

[PATCH 2/4] iommu/vt-d: Force IOMMU on for platform opt in hint

[Mika Westerberg]

From: Lu Baolu <baolu.lu@linux.intel.com>

Intel VT-d spec added a new DMA_CTRL_PLATFORM_OPT_IN_FLAG flag
in DMAR ACPI table for BIOS to report compliance about platform
initiated DMA restricted to RMRR ranges when transferring control
to the OS. The OS treats this as a hint that the IOMMU should be
enabled to prevent DMA attacks from possible malicious devices.

A use of this flag is Kernel DMA protection for Thunderbolt[1]
which in practice means that IOMMU should be enabled for PCIe
devices connected to the Thunderbolt ports. With IOMMU enabled
for these devices, all DMA operations are limited in the range
reserved for it, thus the DMA attacks are prevented. All these
devices are enumerated in the PCI/PCIe module and marked with
an is_external flag.

This forces IOMMU to be enabled if DMA_CTRL_PLATFORM_OPT_IN_FLAG
is set in DMAR ACPI table and there are PCIe devices marked as
is_external in the system. This can be turned off by adding
"intel_iommu=off" in the kernel command line, if any problems are
found.

[1] https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt

Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/iommu/dmar.c        | 25 +++++++++++++++++
 drivers/iommu/intel-iommu.c | 55 +++++++++++++++++++++++++++++++++++--
 include/linux/dmar.h        |  8 ++++++
 3 files changed, 86 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index d9c748b6f9e4..1edf2a251336 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -2042,3 +2042,28 @@ int dmar_device_remove(acpi_handle handle)
 {
 	return dmar_device_hotplug(handle, false);
 }
+
+/*
+ * dmar_platform_optin - Is %DMA_CTRL_PLATFORM_OPT_IN_FLAG set in DMAR table
+ *
+ * Returns true if the platform has %DMA_CTRL_PLATFORM_OPT_IN_FLAG set in
+ * the ACPI DMAR table. This means that the platform boot firmware has made
+ * sure no device can issue DMA outside of RMRR regions.
+ */
+bool dmar_platform_optin(void)
+{
+	struct acpi_table_dmar *dmar;
+	acpi_status status;
+	bool ret;
+
+	status = acpi_get_table(ACPI_SIG_DMAR, 0,
+				(struct acpi_table_header **)&dmar);
+	if (ACPI_FAILURE(status))
+		return false;
+
+	ret = !!(dmar->flags & DMAR_PLATFORM_OPT_IN);
+	acpi_put_table((struct acpi_table_header *)dmar);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dmar_platform_optin);
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index f3ccf025108b..ada786b05a59 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -184,6 +184,7 @@ static int rwbf_quirk;
  */
 static int force_on = 0;
 int intel_iommu_tboot_noforce;
+static int no_platform_optin;
 
 #define ROOT_ENTRY_NR (VTD_PAGE_SIZE/sizeof(struct root_entry))
 
@@ -503,6 +504,7 @@ static int __init intel_iommu_setup(char *str)
 			pr_info("IOMMU enabled\n");
 		} else if (!strncmp(str, "off", 3)) {
 			dmar_disabled = 1;
+			no_platform_optin = 1;
 			pr_info("IOMMU disabled\n");
 		} else if (!strncmp(str, "igfx_off", 8)) {
 			dmar_map_gfx = 0;
@@ -2895,6 +2897,14 @@ static int iommu_should_identity_map(struct device *dev, int startup)
 		if (device_is_rmrr_locked(dev))
 			return 0;
 
+		/*
+		 * Prevent any device marked as an external one from getting
+		 * placed into the statically identity mapping domain. This
+		 * is done because external devices are always untrusted.
+		 */
+		if (pdev->is_external)
+			return 0;
+
 		if ((iommu_identity_mapping & IDENTMAP_AZALIA) && IS_AZALIA(pdev))
 			return 1;
 
@@ -4728,14 +4738,55 @@ const struct attribute_group *intel_iommu_groups[] = {
 	NULL,
 };
 
+static int __init platform_optin_force_iommu(void)
+{
+	struct pci_dev *pdev = NULL;
+	bool has_ext_dev = false;
+
+	if (!dmar_platform_optin() || no_platform_optin)
+		return 0;
+
+	for_each_pci_dev(pdev) {
+		if (pdev->is_external) {
+			has_ext_dev = true;
+			break;
+		}
+	}
+
+	if (!has_ext_dev)
+		return 0;
+
+	if (no_iommu || dmar_disabled)
+		pr_info("Intel-IOMMU force enabled due to platform opt in\n");
+
+	/*
+	 *  If Intel-IOMMU is disabled by default, we will apply
+	 *  identity map for all devices except those marked as
+	 *  unsafe external devices.
+	 */
+	if (dmar_disabled)
+		iommu_identity_mapping |= IDENTMAP_ALL;
+
+	dmar_disabled = 0;
+#if defined(CONFIG_X86) && defined(CONFIG_SWIOTLB)
+	swiotlb = 0;
+#endif
+	no_iommu = 0;
+
+	return 1;
+}
+
 int __init intel_iommu_init(void)
 {
 	int ret = -ENODEV;
 	struct dmar_drhd_unit *drhd;
 	struct intel_iommu *iommu;
 
-	/* VT-d is required for a TXT/tboot launch, so enforce that */
-	force_on = tboot_force_iommu();
+	/*
+	 * Intel IOMMU is required for a TXT/tboot launch or platform
+	 * opt in, so enforce that.
+	 */
+	force_on = tboot_force_iommu() || platform_optin_force_iommu();
 
 	if (iommu_init_mempool()) {
 		if (force_on)
diff --git a/include/linux/dmar.h b/include/linux/dmar.h
index 843a41ba7e28..f8af1d770520 100644
--- a/include/linux/dmar.h
+++ b/include/linux/dmar.h
@@ -39,6 +39,7 @@ struct acpi_dmar_header;
 /* DMAR Flags */
 #define DMAR_INTR_REMAP		0x1
 #define DMAR_X2APIC_OPT_OUT	0x2
+#define DMAR_PLATFORM_OPT_IN	0x4
 
 struct intel_iommu;
 
@@ -170,6 +171,8 @@ static inline int dmar_ir_hotplug(struct dmar_drhd_unit *dmaru, bool insert)
 { return 0; }
 #endif /* CONFIG_IRQ_REMAP */
 
+extern bool dmar_platform_optin(void);
+
 #else /* CONFIG_DMAR_TABLE */
 
 static inline int dmar_device_add(void *handle)
@@ -182,6 +185,11 @@ static inline int dmar_device_remove(void *handle)
 	return 0;
 }
 
+static inline bool dmar_platform_optin(void)
+{
+	return false;
+}
+
 #endif /* CONFIG_DMAR_TABLE */
 
 struct irte {
-- 
2.19.1

[PATCH 3/4] iommu/vt-d: Do not enable ATS for external devices

[Mika Westerberg]

Currently Linux automatically enables ATS (Address Translation Service)
for any device that supports it (and IOMMU is turned on). ATS is used to
accelerate DMA access as the device can cache translations locally so
there is no need to do full translation on IOMMU side. However, as
pointed out in [1] ATS can be used to bypass IOMMU based security
completely by simply sending PCIe read/write transaction with AT
(Address Translation) field set to "translated".

To mitigate this modify the Intel IOMMU code so that it does not enable
ATS for any device that is marked as being external. In case this turns
out to cause performance issues we may selectively allow ATS based on
user decision but currently use big hammer and disable it completely to
be on the safe side.

[1] https://www.repository.cam.ac.uk/handle/1810/274352

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/iommu/intel-iommu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index ada786b05a59..b79788da6971 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -1473,7 +1473,8 @@ static void iommu_enable_dev_iotlb(struct device_domain_info *info)
 	if (info->pri_supported && !pci_reset_pri(pdev) && !pci_enable_pri(pdev, 32))
 		info->pri_enabled = 1;
 #endif
-	if (info->ats_supported && !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
+	if (!pdev->is_external && info->ats_supported &&
+	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
 		info->ats_enabled = 1;
 		domain_update_iotlb(info->domain);
 		info->ats_qdep = pci_ats_queue_depth(pdev);
-- 
2.19.1

[PATCH 4/4] thunderbolt: Export IOMMU based DMA protection support to userspace

[Mika Westerberg]

Recent systems shipping with Windows 10 version 1803 or later may
support a feature called Kernel DMA protection [1]. In practice this
means that Thunderbolt connected devices are placed behind an IOMMU
during the whole time it is connected (including during boot) making
Thunderbolt security levels redundant. Some of these systems still have
Thunderbolt security level set to "user" in order to support OS
downgrade (the older version of the OS might not support IOMMU based DMA
protection so connecting a device still relies on user approval then).

Export this information to userspace by introducing a new sysfs
attribute (iommu_dma_protection). Based on it userspace tools can make
more accurate decision whether or not authorize the connected device.

In addition update Thunderbolt documentation regarding IOMMU based DMA
protection.

[1] https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 .../ABI/testing/sysfs-bus-thunderbolt         |  9 ++++++++
 Documentation/admin-guide/thunderbolt.rst     | 23 +++++++++++++++++++
 drivers/thunderbolt/domain.c                  | 17 ++++++++++++++
 3 files changed, 49 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-thunderbolt b/Documentation/ABI/testing/sysfs-bus-thunderbolt
index 151584a1f950..b21fba14689b 100644
--- a/Documentation/ABI/testing/sysfs-bus-thunderbolt
+++ b/Documentation/ABI/testing/sysfs-bus-thunderbolt
@@ -21,6 +21,15 @@ Description:	Holds a comma separated list of device unique_ids that
 		If a device is authorized automatically during boot its
 		boot attribute is set to 1.
 
+What: /sys/bus/thunderbolt/devices/.../domainX/iommu_dma_protection
+Date:		Mar 2019
+KernelVersion:	4.21
+Contact:	thunderbolt-software@lists.01.org
+Description:	This attribute tells whether the system uses IOMMU
+		for DMA protection. Value of 1 means IOMMU is used 0 means
+		it is not (DMA protection is solely based on Thunderbolt
+		security levels).
+
 What: /sys/bus/thunderbolt/devices/.../domainX/security
 Date:		Sep 2017
 KernelVersion:	4.13
diff --git a/Documentation/admin-guide/thunderbolt.rst b/Documentation/admin-guide/thunderbolt.rst
index 35fccba6a9a6..ccac2596a49f 100644
--- a/Documentation/admin-guide/thunderbolt.rst
+++ b/Documentation/admin-guide/thunderbolt.rst
@@ -133,6 +133,29 @@ If the user still wants to connect the device they can either approve
 the device without a key or write a new key and write 1 to the
 ``authorized`` file to get the new key stored on the device NVM.
 
+DMA protection utilizing IOMMU
+------------------------------
+Recent systems shipping with Windows 10 version 1803 or later may support a
+feature called `Kernel DMA Protection for Thunderbolt 3`_.  This means that
+Thunderbolt security is handled by an IOMMU so connected devices cannot
+access memory regions outside of what is allocated for them by drivers.
+When Linux is running on such system it automatically enables IOMMU if not
+enabled by the user already. These systems can be identified by reading
+``1`` from ``/sys/bus/thunderbolt/devices/domainX/iommu_dma_protection``
+attribute.
+
+The driver does not do anything special in this case but because DMA
+protection is handled by the IOMMU, security levels (if set) are
+redundant. For this reason some systems ship with security level set to
+``none``. Other systems have security level set to ``user`` in order to
+support downgrade to older Windows, so users who want to automatically
+authorize devices when IOMMU DMA protection is enabled can use the
+following ``udev`` rule::
+
+  ACTION=="add", SUBSYSTEM=="thunderbolt", ATTRS{iommu_dma_protection}=="1", ATTR{authorized}=="0", ATTR{authorized}="1"
+
+.. _Kernel DMA Protection for Thunderbolt 3: https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt
+
 Upgrading NVM on Thunderbolt device or host
 -------------------------------------------
 Since most of the functionality is handled in firmware running on a
diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
index 93e562f18d40..7416bdbd8576 100644
--- a/drivers/thunderbolt/domain.c
+++ b/drivers/thunderbolt/domain.c
@@ -7,7 +7,9 @@
  */
 
 #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>
@@ -236,6 +238,20 @@ static ssize_t boot_acl_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(boot_acl);
 
+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());
+}
+static DEVICE_ATTR_RO(iommu_dma_protection);
+
 static ssize_t security_show(struct device *dev, struct device_attribute *attr,
 			     char *buf)
 {
@@ -251,6 +267,7 @@ static DEVICE_ATTR_RO(security);
 
 static struct attribute *domain_attrs[] = {
 	&dev_attr_boot_acl.attr,
+	&dev_attr_iommu_dma_protection.attr,
 	&dev_attr_security.attr,
 	NULL,
 };
-- 
2.19.1

RE: [PATCH 4/4] thunderbolt: Export IOMMU based DMA protection support to userspace

[Mario.Limonciello]

> -----Original Message-----
> From: Mika Westerberg <mika.westerberg@linux.intel.com>
> Sent: Monday, November 12, 2018 10:06 AM
> To: iommu@lists.linux-foundation.org
> Cc: Joerg Roedel; David Woodhouse; Lu Baolu; Ashok Raj; Bjorn Helgaas; Rafael J.
> Wysocki; Jacob jun Pan; Andreas Noever; Michael Jamet; Yehezkel Bernat; Lukas
> Wunner; Christian Kellner; Limonciello, Mario; Anthony Wong; Mika Westerberg;
> linux-acpi@vger.kernel.org; linux-pci@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [PATCH 4/4] thunderbolt: Export IOMMU based DMA protection support
> to userspace
> 
> 
> 
> 
> Recent systems shipping with Windows 10 version 1803 or later may
> support a feature called Kernel DMA protection [1]. In practice this
> means that Thunderbolt connected devices are placed behind an IOMMU
> during the whole time it is connected (including during boot) making
> Thunderbolt security levels redundant. Some of these systems still have
> Thunderbolt security level set to "user" in order to support OS
> downgrade (the older version of the OS might not support IOMMU based DMA
> protection so connecting a device still relies on user approval then).
> 
> Export this information to userspace by introducing a new sysfs
> attribute (iommu_dma_protection). Based on it userspace tools can make
> more accurate decision whether or not authorize the connected device.
> 
> In addition update Thunderbolt documentation regarding IOMMU based DMA
> protection.
> 
> [1] https://docs.microsoft.com/en-us/windows/security/information-
> protection/kernel-dma-protection-for-thunderbolt
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  .../ABI/testing/sysfs-bus-thunderbolt         |  9 ++++++++
>  Documentation/admin-guide/thunderbolt.rst     | 23 +++++++++++++++++++
>  drivers/thunderbolt/domain.c                  | 17 ++++++++++++++
>  3 files changed, 49 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-thunderbolt
> b/Documentation/ABI/testing/sysfs-bus-thunderbolt
> index 151584a1f950..b21fba14689b 100644
> --- a/Documentation/ABI/testing/sysfs-bus-thunderbolt
> +++ b/Documentation/ABI/testing/sysfs-bus-thunderbolt
> @@ -21,6 +21,15 @@ Description:	Holds a comma separated list of device
> unique_ids that
>  		If a device is authorized automatically during boot its
>  		boot attribute is set to 1.
> 
> +What: /sys/bus/thunderbolt/devices/.../domainX/iommu_dma_protection
> +Date:		Mar 2019
> +KernelVersion:	4.21
> +Contact:	thunderbolt-software@lists.01.org
> +Description:	This attribute tells whether the system uses IOMMU
> +		for DMA protection. Value of 1 means IOMMU is used 0 means
> +		it is not (DMA protection is solely based on Thunderbolt
> +		security levels).
> +
>  What: /sys/bus/thunderbolt/devices/.../domainX/security
>  Date:		Sep 2017
>  KernelVersion:	4.13
> diff --git a/Documentation/admin-guide/thunderbolt.rst b/Documentation/admin-
> guide/thunderbolt.rst
> index 35fccba6a9a6..ccac2596a49f 100644
> --- a/Documentation/admin-guide/thunderbolt.rst
> +++ b/Documentation/admin-guide/thunderbolt.rst
> @@ -133,6 +133,29 @@ If the user still wants to connect the device they can
> either approve
>  the device without a key or write a new key and write 1 to the
>  ``authorized`` file to get the new key stored on the device NVM.
> 
> +DMA protection utilizing IOMMU
> +------------------------------
> +Recent systems shipping with Windows 10 version 1803 or later may support a
> +feature called `Kernel DMA Protection for Thunderbolt 3`_.  This means that

Keep in mind there will be systems that ship with Linux that enable this feature too ;)

It might be better to make it time frame and platform  firmware oriented as it's
entirely possible for an OEM to have a field firmware upgrade that may enable this
functionality from the platform and allow an end user to upgrade to a sufficiently
protected kernel or Windows OS to take advantage of it.

> +Thunderbolt security is handled by an IOMMU so connected devices cannot
> +access memory regions outside of what is allocated for them by drivers.
> +When Linux is running on such system it automatically enables IOMMU if not
> +enabled by the user already. These systems can be identified by reading
> +``1`` from ``/sys/bus/thunderbolt/devices/domainX/iommu_dma_protection``
> +attribute.
> +
> +The driver does not do anything special in this case but because DMA
> +protection is handled by the IOMMU, security levels (if set) are
> +redundant. For this reason some systems ship with security level set to
> +``none``. Other systems have security level set to ``user`` in order to
> +support downgrade to older Windows, so users who want to automatically
> +authorize devices when IOMMU DMA protection is enabled can use the
> +following ``udev`` rule::
> +
> +  ACTION=="add", SUBSYSTEM=="thunderbolt",
> ATTRS{iommu_dma_protection}=="1", ATTR{authorized}=="0",
> ATTR{authorized}="1"
> +
> +.. _Kernel DMA Protection for Thunderbolt 3: https://docs.microsoft.com/en-
> us/windows/security/information-protection/kernel-dma-protection-for-
> thunderbolt
> +
>  Upgrading NVM on Thunderbolt device or host
>  -------------------------------------------
>  Since most of the functionality is handled in firmware running on a
> diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
> index 93e562f18d40..7416bdbd8576 100644
> --- a/drivers/thunderbolt/domain.c
> +++ b/drivers/thunderbolt/domain.c
> @@ -7,7 +7,9 @@
>   */
> 
>  #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>
> @@ -236,6 +238,20 @@ static ssize_t boot_acl_store(struct device *dev, struct
> device_attribute *attr,
>  }
>  static DEVICE_ATTR_RW(boot_acl);
> 
> +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());
> +}
> +static DEVICE_ATTR_RO(iommu_dma_protection);
> +
>  static ssize_t security_show(struct device *dev, struct device_attribute *attr,
>  			     char *buf)
>  {
> @@ -251,6 +267,7 @@ static DEVICE_ATTR_RO(security);
> 
>  static struct attribute *domain_attrs[] = {
>  	&dev_attr_boot_acl.attr,
> +	&dev_attr_iommu_dma_protection.attr,
>  	&dev_attr_security.attr,
>  	NULL,
>  };
> --
> 2.19.1

Re: [PATCH 4/4] thunderbolt: Export IOMMU based DMA protection support to userspace

[Yehezkel Bernat]

On Mon, Nov 12, 2018 at 6:06 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Recent systems shipping with Windows 10 version 1803 or later may
> support a feature called Kernel DMA protection [1]. In practice this
> means that Thunderbolt connected devices are placed behind an IOMMU
> during the whole time it is connected (including during boot) making
> Thunderbolt security levels redundant. Some of these systems still have
> Thunderbolt security level set to "user" in order to support OS
> downgrade (the older version of the OS might not support IOMMU based DMA
> protection so connecting a device still relies on user approval then).
>
> Export this information to userspace by introducing a new sysfs
> attribute (iommu_dma_protection). Based on it userspace tools can make
> more accurate decision whether or not authorize the connected device.
>
> In addition update Thunderbolt documentation regarding IOMMU based DMA
> protection.
>
> [1] https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---

I can't comment on the IOMMU side, but the Thunderbolt side looks good to me.

Just one point:
Have you considered the option to add this property per (TBT?) device?
If the kernel may decide to enable/disable the IOMMU or AST per device, maybe
it should be on this level.
Or maybe the IOMMU decision isn't going to change (it's system-wide) and the AST
decision will be communicated per device by a new sysfs attribute anyway, if
needed?

Re: [PATCH 2/4] iommu/vt-d: Force IOMMU on for platform opt in hint

[Raj, Ashok]

On Mon, Nov 12, 2018 at 07:06:26PM +0300, Mika Westerberg wrote:
> From: Lu Baolu <baolu.lu@linux.intel.com>
> 
> Intel VT-d spec added a new DMA_CTRL_PLATFORM_OPT_IN_FLAG flag
> in DMAR ACPI table for BIOS to report compliance about platform
> initiated DMA restricted to RMRR ranges when transferring control
> to the OS. The OS treats this as a hint that the IOMMU should be
> enabled to prevent DMA attacks from possible malicious devices.
> 
> A use of this flag is Kernel DMA protection for Thunderbolt[1]
> which in practice means that IOMMU should be enabled for PCIe
> devices connected to the Thunderbolt ports. With IOMMU enabled
> for these devices, all DMA operations are limited in the range
> reserved for it, thus the DMA attacks are prevented. All these
> devices are enumerated in the PCI/PCIe module and marked with
> an is_external flag.
> 
> This forces IOMMU to be enabled if DMA_CTRL_PLATFORM_OPT_IN_FLAG
> is set in DMAR ACPI table and there are PCIe devices marked as
> is_external in the system. This can be turned off by adding
> "intel_iommu=off" in the kernel command line, if any problems are
> found.
> 
> [1] https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt
> 
> Cc: Ashok Raj <ashok.raj@intel.com>
> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Cc: Sohil Mehta <sohil.mehta@intel.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Looks good to me

Reviewed-by: Ashok Raj <ashok.raj@intel.com>

> ---
>  drivers/iommu/dmar.c        | 25 +++++++++++++++++
>  drivers/iommu/intel-iommu.c | 55 +++++++++++++++++++++++++++++++++++--
>  include/linux/dmar.h        |  8 ++++++
>  3 files changed, 86 insertions(+), 2 deletions(-)
 

Re: [PATCH 3/4] iommu/vt-d: Do not enable ATS for external devices

[Raj, Ashok]

On Mon, Nov 12, 2018 at 07:06:27PM +0300, Mika Westerberg wrote:
> Currently Linux automatically enables ATS (Address Translation Service)
> for any device that supports it (and IOMMU is turned on). ATS is used to
> accelerate DMA access as the device can cache translations locally so
> there is no need to do full translation on IOMMU side. However, as
> pointed out in [1] ATS can be used to bypass IOMMU based security
> completely by simply sending PCIe read/write transaction with AT
> (Address Translation) field set to "translated".
> 
> To mitigate this modify the Intel IOMMU code so that it does not enable
> ATS for any device that is marked as being external. In case this turns
> out to cause performance issues we may selectively allow ATS based on
> user decision but currently use big hammer and disable it completely to
> be on the safe side.
> 
> [1] https://www.repository.cam.ac.uk/handle/1810/274352
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Reviewed-by: Ashok Raj <ashok.raj@intel.com>

> ---
>  drivers/iommu/intel-iommu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index ada786b05a59..b79788da6971 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -1473,7 +1473,8 @@ static void iommu_enable_dev_iotlb(struct device_domain_info *info)
>  	if (info->pri_supported && !pci_reset_pri(pdev) && !pci_enable_pri(pdev, 32))
>  		info->pri_enabled = 1;
>  #endif
> -	if (info->ats_supported && !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
> +	if (!pdev->is_external && info->ats_supported &&
> +	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
>  		info->ats_enabled = 1;
>  		domain_update_iotlb(info->domain);
>  		info->ats_qdep = pci_ats_queue_depth(pdev);
> -- 
> 2.19.1
> 

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Lukas Wunner]

On Mon, Nov 12, 2018 at 07:06:25PM +0300, Mika Westerberg wrote:
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1378,6 +1378,27 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
>  	}
>  }
>  
> +static void set_pcie_external(struct pci_dev *dev)
> +{
> +	struct pci_dev *parent;
> +
> +	/*
> +	 * Walk up the device hierarchy and check for any upstream
> +	 * bridge that has is_external_facing set to true. This means
> +	 * the hierarchy is below PCIe port that is exposed externally
> +	 * (such as Thunderbolt).
> +	 */
> +	parent = pci_upstream_bridge(dev);
> +	while (parent) {
> +		if (parent->is_external) {
> +			dev->is_external = true;
> +			break;
> +		}
> +
> +		parent = pci_upstream_bridge(parent);
> +	}
> +}
> +

This looks pretty much like a duplication of the is_thunderbolt bit
in struct pci_dev and the pci_is_thunderbolt_attached() helper.

Why constrain the functionality to ports with the _DSD property
instead of making it available for *any* Thunderbolt port?

Thanks,

Lukas

Re: [PATCH 2/4] iommu/vt-d: Force IOMMU on for platform opt in hint

[Alex Williamson]

On Mon, 12 Nov 2018 19:06:26 +0300
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> From: Lu Baolu <baolu.lu@linux.intel.com>
> 
> Intel VT-d spec added a new DMA_CTRL_PLATFORM_OPT_IN_FLAG flag
> in DMAR ACPI table for BIOS to report compliance about platform
> initiated DMA restricted to RMRR ranges when transferring control
> to the OS. The OS treats this as a hint that the IOMMU should be
> enabled to prevent DMA attacks from possible malicious devices.

Does this in any way suggest that there are additional recommended uses
cases from Intel for RMRRs?  My concern here is the incompatibility we
have with RMRRs and device assignment as we currently cannot assign
devices where the IOVA address space is encumbered by RMRR
requirements.  Unfortunately RMRRs do not indicate any sort or
lifespan, so firmware enabling an RMRR simply to support some boot-time
DMA encumbers the device with that RMRR for the life of that boot,
unless we have VT-d code that decides it knows better.  Thanks,

Alex

Re: [PATCH 0/4] PCI / iommu / thunderbolt: IOMMU based DMA protection

[Lukas Wunner]

On Mon, Nov 12, 2018 at 07:06:24PM +0300, Mika Westerberg wrote:
> Recent systems shipping with Windows 10 version 1803 or newer may be
> utilizing IOMMU to prevent DMA attacks via Thunderbolt ports. This is
> different from the previous security level based scheme because the
> connected device cannot access system memory outside of the regions
> allocated for it by the driver.
> 
> When enabled the BIOS makes sure no device can do DMA outside of RMRR
> (Reserved Memory Region Record) regions. This means that during OS boot,
> before it enables IOMMU, none of the connected devices can bypass DMA
> protection for instance by overwriting the data structures used by the
> IOMMU. The BIOS communicates support for this to the OS by setting a new
> bit in ACPI DMAR table [1].
> 
> Because these systems utilize an IOMMU to block possible DMA attacks,
> typically (but not always) the Thunderbolt security level is set to "none"
> which means that all PCIe devices are immediately usable. This also means
> that Linux needs to follow Windows 10 and enable IOMMU automatically when
> running on such system otherwise connected devices can read/write system
> memory pretty much without any restrictions.

What if the system is booted from a Thunderbolt-attached disk?
Won't this suddenly break with these patches?  That would seem like a
pretty significant regression.  What if the only GPU in the system is
Thunderbolt-attached?  Is it possible to recognize such scenarios and
automatically exempt affected devices from IOMMU blocking?

Thanks,

Lukas

Re: [PATCH 0/4] PCI / iommu / thunderbolt: IOMMU based DMA protection

[Yehezkel Bernat]

On Mon, Nov 12, 2018 at 8:12 PM Lukas Wunner <lukas@wunner.de> wrote:
>
> On Mon, Nov 12, 2018 at 07:06:24PM +0300, Mika Westerberg wrote:
> > Recent systems shipping with Windows 10 version 1803 or newer may be
> > utilizing IOMMU to prevent DMA attacks via Thunderbolt ports. This is
> > different from the previous security level based scheme because the
> > connected device cannot access system memory outside of the regions
> > allocated for it by the driver.
> >
> > When enabled the BIOS makes sure no device can do DMA outside of RMRR
> > (Reserved Memory Region Record) regions. This means that during OS boot,
> > before it enables IOMMU, none of the connected devices can bypass DMA
> > protection for instance by overwriting the data structures used by the
> > IOMMU. The BIOS communicates support for this to the OS by setting a new
> > bit in ACPI DMAR table [1].
> >
> > Because these systems utilize an IOMMU to block possible DMA attacks,
> > typically (but not always) the Thunderbolt security level is set to "none"
> > which means that all PCIe devices are immediately usable. This also means
> > that Linux needs to follow Windows 10 and enable IOMMU automatically when
> > running on such system otherwise connected devices can read/write system
> > memory pretty much without any restrictions.
>
> What if the system is booted from a Thunderbolt-attached disk?
> Won't this suddenly break with these patches? That would seem like a
> pretty significant regression.

My assumption is that either it isn't supported on such platforms (at least with
this security configuration active) so this doesn't break anything, it never
worked there, or the BIOS configures IOMMU in a way that allows the disk to work
until the OS will take control and configure the IOMMU according to OS
decisions.
In the latter case, the kernel+initrd will be loaded before IOMMU configuration
will be changed, and then the kernel should be able to config it correctly to
work. (Unless I really don't understand the mechanism and workflow of using
IOMMU, which is possible.)

Re: [PATCH 2/4] iommu/vt-d: Force IOMMU on for platform opt in hint

[Raj, Ashok]

On Mon, Nov 12, 2018 at 11:09:00AM -0700, Alex Williamson wrote:
> On Mon, 12 Nov 2018 19:06:26 +0300
> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> 
> > From: Lu Baolu <baolu.lu@linux.intel.com>
> > 
> > Intel VT-d spec added a new DMA_CTRL_PLATFORM_OPT_IN_FLAG flag
> > in DMAR ACPI table for BIOS to report compliance about platform
> > initiated DMA restricted to RMRR ranges when transferring control
> > to the OS. The OS treats this as a hint that the IOMMU should be
> > enabled to prevent DMA attacks from possible malicious devices.
> 
> Does this in any way suggest that there are additional recommended uses
> cases from Intel for RMRRs?  My concern here is the incompatibility we
> have with RMRRs and device assignment as we currently cannot assign
> devices where the IOVA address space is encumbered by RMRR
> requirements.  Unfortunately RMRRs do not indicate any sort or
> lifespan, so firmware enabling an RMRR simply to support some boot-time
> DMA encumbers the device with that RMRR for the life of that boot,
> unless we have VT-d code that decides it knows better.  Thanks,

IMO any new platform that requires RMRR should be a bug. It was designed 
originally for some legacy keyboard emulation etc.

The best behavior is to continue to not allow devices with RMRR be direct assigned.

Technically ignoring RMRR's and continuing to assign those devices is risky.
The problem is IF BIOS/SMM initiates some IO in the RMRR range and it happens to be
mapped by the direct assigned GPA its going to be ugly failure.

Re: [PATCH 0/4] PCI / iommu / thunderbolt: IOMMU based DMA protection

[Joerg Roedel]

On Mon, Nov 12, 2018 at 07:06:24PM +0300, Mika Westerberg wrote:
> Lu Baolu (1):
>   iommu/vt-d: Force IOMMU on for platform opt in hint
> 
> Mika Westerberg (3):
>   PCI / ACPI: Identify external PCI devices
>   iommu/vt-d: Do not enable ATS for external devices
>   thunderbolt: Export IOMMU based DMA protection support to userspace

Looks good to me. Which tree should go this trough? In case its not the
IOMMU tree, for the iommu-parts:

	Acked-by: Joerg Roedel <jroedel@suse.de>
	Reviewed-by: Joerg Roedel <jroedel@suse.de>

Thanks,

	Joerg

Re: [PATCH 4/4] thunderbolt: Export IOMMU based DMA protection support to userspace

[Mika Westerberg]

On Mon, Nov 12, 2018 at 04:22:25PM +0000, Mario.Limonciello@dell.com wrote:
> > +DMA protection utilizing IOMMU
> > +------------------------------
> > +Recent systems shipping with Windows 10 version 1803 or later may support a
> > +feature called `Kernel DMA Protection for Thunderbolt 3`_.  This means that
> 
> Keep in mind there will be systems that ship with Linux that enable this feature too ;)

Yes, you are absolutely right. I sometimes forgot that fact ;-)

> It might be better to make it time frame and platform  firmware oriented as it's
> entirely possible for an OEM to have a field firmware upgrade that may enable this
> functionality from the platform and allow an end user to upgrade to a sufficiently
> protected kernel or Windows OS to take advantage of it.

I agree. I will update this in the next version.

Re: [PATCH 4/4] thunderbolt: Export IOMMU based DMA protection support to userspace

[Mika Westerberg]

On Mon, Nov 12, 2018 at 06:59:02PM +0200, Yehezkel Bernat wrote:
> On Mon, Nov 12, 2018 at 6:06 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > Recent systems shipping with Windows 10 version 1803 or later may
> > support a feature called Kernel DMA protection [1]. In practice this
> > means that Thunderbolt connected devices are placed behind an IOMMU
> > during the whole time it is connected (including during boot) making
> > Thunderbolt security levels redundant. Some of these systems still have
> > Thunderbolt security level set to "user" in order to support OS
> > downgrade (the older version of the OS might not support IOMMU based DMA
> > protection so connecting a device still relies on user approval then).
> >
> > Export this information to userspace by introducing a new sysfs
> > attribute (iommu_dma_protection). Based on it userspace tools can make
> > more accurate decision whether or not authorize the connected device.
> >
> > In addition update Thunderbolt documentation regarding IOMMU based DMA
> > protection.
> >
> > [1] https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> 
> I can't comment on the IOMMU side, but the Thunderbolt side looks good to me.

Thanks!

> Just one point:
> Have you considered the option to add this property per (TBT?) device?

No. ;-)

You mean that one device uses security levels and another IOMMU? I don't
think it is possible without having some sort of table in the IOMMU
driver telling which devices it needs identity map and which not. Also
not sure what would be the benefit?

> If the kernel may decide to enable/disable the IOMMU or AST per device, maybe
> it should be on this level.
> Or maybe the IOMMU decision isn't going to change (it's system-wide) and the AST
> decision will be communicated per device by a new sysfs attribute anyway, if
> needed?

Not sure what you mean by "AST"? The IOMMU decision is pretty much
system-wide.

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Lorenzo Pieralisi]

On Mon, Nov 12, 2018 at 07:02:03PM +0100, Lukas Wunner wrote:
> On Mon, Nov 12, 2018 at 07:06:25PM +0300, Mika Westerberg wrote:
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1378,6 +1378,27 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
> >  	}
> >  }
> >  
> > +static void set_pcie_external(struct pci_dev *dev)
> > +{
> > +	struct pci_dev *parent;
> > +
> > +	/*
> > +	 * Walk up the device hierarchy and check for any upstream
> > +	 * bridge that has is_external_facing set to true. This means
> > +	 * the hierarchy is below PCIe port that is exposed externally
> > +	 * (such as Thunderbolt).
> > +	 */
> > +	parent = pci_upstream_bridge(dev);
> > +	while (parent) {
> > +		if (parent->is_external) {
> > +			dev->is_external = true;
> > +			break;
> > +		}
> > +
> > +		parent = pci_upstream_bridge(parent);
> > +	}
> > +}
> > +
> 
> This looks pretty much like a duplication of the is_thunderbolt bit
> in struct pci_dev and the pci_is_thunderbolt_attached() helper.
> 
> Why constrain the functionality to ports with the _DSD property
> instead of making it available for *any* Thunderbolt port?

I assume it is because this is just not needed for Thuderbolt ports but
rather for all PCIe devices that are "external" (whatever that is
supposed to mean), ie it is valid also for PCIe slots.

To be frank the concept (and Microsoft _DSD bindings) seems a bit vague
and not thoroughly defined and I would question its detection at
PCI/ACPI core level, I would hope this can be clarified at ACPI
specification level, at least.

Thanks,
Lorenzo

Re: [PATCH 4/4] thunderbolt: Export IOMMU based DMA protection support to userspace

[Yehezkel Bernat]

On Tue, Nov 13, 2018 at 12:56 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> > Just one point:
> > Have you considered the option to add this property per (TBT?) device?
>
> No. ;-)
>
> You mean that one device uses security levels and another IOMMU? I don't
> think it is possible without having some sort of table in the IOMMU
> driver telling which devices it needs identity map and which not. Also
> not sure what would be the benefit?

For performance, of course. If some devices are considered safe (maybe a list
communicated by platform firmware), the kernel may decide to configure them to
passthrough the IOMMU (I think I remember there is such an option, but maybe I'm
wrong.)


> > If the kernel may decide to enable/disable the IOMMU or AST per device, maybe
> > it should be on this level.
> > Or maybe the IOMMU decision isn't going to change (it's system-wide) and the AST
> > decision will be communicated per device by a new sysfs attribute anyway, if
> > needed?
>
> Not sure what you mean by "AST"? The IOMMU decision is pretty much
> system-wide.

Sorry, I meant ATS, Address Translation Service, mentioned in patch 3 in this
series, and possibly be enabled for some devices for performance, as mentioned
there.
So if needed, this will be another attribute, and definitely
per-device, isn't it?

Re: [PATCH 0/4] PCI / iommu / thunderbolt: IOMMU based DMA protection

[Mika Westerberg]

On Mon, Nov 12, 2018 at 07:12:14PM +0100, Lukas Wunner wrote:
> On Mon, Nov 12, 2018 at 07:06:24PM +0300, Mika Westerberg wrote:
> > Recent systems shipping with Windows 10 version 1803 or newer may be
> > utilizing IOMMU to prevent DMA attacks via Thunderbolt ports. This is
> > different from the previous security level based scheme because the
> > connected device cannot access system memory outside of the regions
> > allocated for it by the driver.
> > 
> > When enabled the BIOS makes sure no device can do DMA outside of RMRR
> > (Reserved Memory Region Record) regions. This means that during OS boot,
> > before it enables IOMMU, none of the connected devices can bypass DMA
> > protection for instance by overwriting the data structures used by the
> > IOMMU. The BIOS communicates support for this to the OS by setting a new
> > bit in ACPI DMAR table [1].
> > 
> > Because these systems utilize an IOMMU to block possible DMA attacks,
> > typically (but not always) the Thunderbolt security level is set to "none"
> > which means that all PCIe devices are immediately usable. This also means
> > that Linux needs to follow Windows 10 and enable IOMMU automatically when
> > running on such system otherwise connected devices can read/write system
> > memory pretty much without any restrictions.
> 
> What if the system is booted from a Thunderbolt-attached disk?
> Won't this suddenly break with these patches?

Like Yehezkel commented, it either is not supported or alternatively it
is (the BIOS/boot loader utilizes IOMMU as well), loads the OS image and
what is needed from the disk, disables BME (bus mastering enable) and
resets the IOMMU back to the default state before handing over to the OS.

> That would seem like a pretty significant regression.  What if the
> only GPU in the system is Thunderbolt-attached?  Is it possible to
> recognize such scenarios and automatically exempt affected devices
> from IOMMU blocking?

"IOMMU blocking" does not mean that the device cannot be used at all. It
actually means that the device is immediately usable and can do DMA as
the driver has programmed (using DMA-API) but it cannot access any
memory outside of those regions the driver has programmed.

The point of this exercise is to prevent so called drive-by DMA attacks
where you go to take a cup of coffee and during that time someone plugs
in a malicous device to Thunderbolt port of your laptop and reads all
your secrets.

When IOMMU is enabled the malicous device still is usable (assuming
there is driver for it) but it cannot go and read all the memory, just
the memory driver has programmed.

So in your GPU case it should just work assuming the GPU has a driver.

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Mika Westerberg]

On Tue, Nov 13, 2018 at 10:56:36AM +0000, Lorenzo Pieralisi wrote:
> On Mon, Nov 12, 2018 at 07:02:03PM +0100, Lukas Wunner wrote:
> > On Mon, Nov 12, 2018 at 07:06:25PM +0300, Mika Westerberg wrote:
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -1378,6 +1378,27 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
> > >  	}
> > >  }
> > >  
> > > +static void set_pcie_external(struct pci_dev *dev)
> > > +{
> > > +	struct pci_dev *parent;
> > > +
> > > +	/*
> > > +	 * Walk up the device hierarchy and check for any upstream
> > > +	 * bridge that has is_external_facing set to true. This means
> > > +	 * the hierarchy is below PCIe port that is exposed externally
> > > +	 * (such as Thunderbolt).
> > > +	 */
> > > +	parent = pci_upstream_bridge(dev);
> > > +	while (parent) {
> > > +		if (parent->is_external) {
> > > +			dev->is_external = true;
> > > +			break;
> > > +		}
> > > +
> > > +		parent = pci_upstream_bridge(parent);
> > > +	}
> > > +}
> > > +
> > 
> > This looks pretty much like a duplication of the is_thunderbolt bit
> > in struct pci_dev and the pci_is_thunderbolt_attached() helper.
> > 
> > Why constrain the functionality to ports with the _DSD property
> > instead of making it available for *any* Thunderbolt port?
> 
> I assume it is because this is just not needed for Thuderbolt ports but
> rather for all PCIe devices that are "external" (whatever that is
> supposed to mean), ie it is valid also for PCIe slots.

Yes, that was the idea. We could have other "external" devices that are
not using Thunderbolt as the interconnect.

We could do so that we automatically set "is_external" for devices with
"is_thunderbolt" so it should cover those as well.

> To be frank the concept (and Microsoft _DSD bindings) seems a bit vague
> and not thoroughly defined and I would question its detection at
> PCI/ACPI core level, I would hope this can be clarified at ACPI
> specification level, at least.

I guess that is the way they envision to use _DSD. Instead of having
single UUID that covers all properties (like what we have with device
properties) they have one UUID per property "class". I certainly hope we
don't need to keep extending prp_guids[] array each time they invent
another "class" of properties.

Re: [PATCH 0/4] PCI / iommu / thunderbolt: IOMMU based DMA protection

[Mika Westerberg]

On Tue, Nov 13, 2018 at 09:54:24AM +0100, Joerg Roedel wrote:
> On Mon, Nov 12, 2018 at 07:06:24PM +0300, Mika Westerberg wrote:
> > Lu Baolu (1):
> >   iommu/vt-d: Force IOMMU on for platform opt in hint
> > 
> > Mika Westerberg (3):
> >   PCI / ACPI: Identify external PCI devices
> >   iommu/vt-d: Do not enable ATS for external devices
> >   thunderbolt: Export IOMMU based DMA protection support to userspace
> 
> Looks good to me. Which tree should go this trough? In case its not the
> IOMMU tree, for the iommu-parts:

I was thinking to take them through Thunderbolt tree.

> 	Acked-by: Joerg Roedel <jroedel@suse.de>
> 	Reviewed-by: Joerg Roedel <jroedel@suse.de>

Thanks!

Re: [PATCH 4/4] thunderbolt: Export IOMMU based DMA protection support to userspace

[Mika Westerberg]

On Tue, Nov 13, 2018 at 01:13:31PM +0200, Yehezkel Bernat wrote:
> On Tue, Nov 13, 2018 at 12:56 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > > Just one point:
> > > Have you considered the option to add this property per (TBT?) device?
> >
> > No. ;-)
> >
> > You mean that one device uses security levels and another IOMMU? I don't
> > think it is possible without having some sort of table in the IOMMU
> > driver telling which devices it needs identity map and which not. Also
> > not sure what would be the benefit?
> 
> For performance, of course. If some devices are considered safe (maybe a list
> communicated by platform firmware), the kernel may decide to configure them to
> passthrough the IOMMU (I think I remember there is such an option, but maybe I'm
> wrong.)

At least I'm not aware of such an option. Windows for example enables
IOMMU for everything and I think macOS does the same. In Linux (with
these patches) we put all internal devices already passthrough mode so
things like internal graphics should not be affected. eGPUs are
different thing, though.

> > > If the kernel may decide to enable/disable the IOMMU or AST per device, maybe
> > > it should be on this level.
> > > Or maybe the IOMMU decision isn't going to change (it's system-wide) and the AST
> > > decision will be communicated per device by a new sysfs attribute anyway, if
> > > needed?
> >
> > Not sure what you mean by "AST"? The IOMMU decision is pretty much
> > system-wide.
> 
> Sorry, I meant ATS, Address Translation Service, mentioned in patch 3 in this
> series, and possibly be enabled for some devices for performance, as mentioned
> there.

Thanks for clarifying :)

> So if needed, this will be another attribute, and definitely
> per-device, isn't it?

Yes, we may want to add a way enable ATS for external devices (perhaps
global option or per-device attribute) if it turns out to cause
performance issues. However, I think it can be done later if needed. I
have not seen a single device actually supporting ATS (with the
exception of the "hacked" one in the linked thesis of patch 3/4 ;-))

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Lorenzo Pieralisi]

On Tue, Nov 13, 2018 at 01:27:00PM +0200, Mika Westerberg wrote:

[...]

> > To be frank the concept (and Microsoft _DSD bindings) seems a bit vague
> > and not thoroughly defined and I would question its detection at
> > PCI/ACPI core level, I would hope this can be clarified at ACPI
> > specification level, at least.
> 
> I guess that is the way they envision to use _DSD. Instead of having
> single UUID that covers all properties (like what we have with device
> properties) they have one UUID per property "class". I certainly hope we
> don't need to keep extending prp_guids[] array each time they invent
> another "class" of properties.

It is even worse than that. This is a unilateral/obscure change that
won't be part of ACPI specifications (I guess it was easier to add a
UUID than add this to the ACPI specifications through the AWSG) but it
is still supposed to be applicable to ACPI PCI bindings on any
platforms/arches; this way of adding bindings does not work and it
has to be rectified.

Lorenzo

Re: [PATCH 4/4] thunderbolt: Export IOMMU based DMA protection support to userspace

[Yehezkel Bernat]

On Tue, Nov 13, 2018 at 1:40 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Tue, Nov 13, 2018 at 01:13:31PM +0200, Yehezkel Bernat wrote:
> > On Tue, Nov 13, 2018 at 12:56 PM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > >
> > > > Just one point:
> > > > Have you considered the option to add this property per (TBT?) device?
> > >
> > > No. ;-)
> > >
> > > You mean that one device uses security levels and another IOMMU? I don't
> > > think it is possible without having some sort of table in the IOMMU
> > > driver telling which devices it needs identity map and which not. Also
> > > not sure what would be the benefit?
> >
> > For performance, of course. If some devices are considered safe (maybe a list
> > communicated by platform firmware), the kernel may decide to configure them to
> > passthrough the IOMMU (I think I remember there is such an option, but maybe I'm
> > wrong.)
>
> At least I'm not aware of such an option. Windows for example enables
> IOMMU for everything and I think macOS does the same. In Linux (with
> these patches) we put all internal devices already passthrough mode so
> things like internal graphics should not be affected. eGPUs are
> different thing, though.

So your point here is "currently we do the IOMMU decisions system-wide; we can
always add a per-device attribute if needed"?
Fair enough.

So for this patch,
Reviewed-by: Yehezkel Bernat <YehezkelShB@gmail.com>

Re: [PATCH 4/4] thunderbolt: Export IOMMU based DMA protection support to userspace

[Yehezkel Bernat]

On Tue, Nov 13, 2018 at 5:20 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Tue, Nov 13, 2018 at 04:42:58PM +0200, Yehezkel Bernat wrote:
> > On Tue, Nov 13, 2018 at 1:40 PM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > >
> > > On Tue, Nov 13, 2018 at 01:13:31PM +0200, Yehezkel Bernat wrote:
> > > > On Tue, Nov 13, 2018 at 12:56 PM Mika Westerberg
> > > > <mika.westerberg@linux.intel.com> wrote:
> > > > >
> > > > > > Just one point:
> > > > > > Have you considered the option to add this property per (TBT?) device?
> > > > >
> > > > > No. ;-)
> > > > >
> > > > > You mean that one device uses security levels and another IOMMU? I don't
> > > > > think it is possible without having some sort of table in the IOMMU
> > > > > driver telling which devices it needs identity map and which not. Also
> > > > > not sure what would be the benefit?
> > > >
> > > > For performance, of course. If some devices are considered safe (maybe a list
> > > > communicated by platform firmware), the kernel may decide to configure them to
> > > > passthrough the IOMMU (I think I remember there is such an option, but maybe I'm
> > > > wrong.)
> > >
> > > At least I'm not aware of such an option. Windows for example enables
> > > IOMMU for everything and I think macOS does the same. In Linux (with
> > > these patches) we put all internal devices already passthrough mode so
> > > things like internal graphics should not be affected. eGPUs are
> > > different thing, though.
> >
> > So your point here is "currently we do the IOMMU decisions system-wide; we can
> > always add a per-device attribute if needed"?
>
> Well, let me elaborate a bit :) I think it is possible to put certain
> devices into IOMMU passthrough mode even if they are "external" but I'm
> not that familiar with the guts of Intel IOMMU (Baolu or Ashok are the
> experts). Assuming it is possible we could have a table or similar in
> the kernel that can be used to identify devices that are allowed to be
> in passthrough mode.
>
> I'm not sure if it can be per-device sysfs attribute because at the time
> the PCIe device is enumerated it is already put to its IOMMU group.

Good point. But I thought about per-TBT-device decision. If the platform is
configured for IOMMU+"user" security level, while approving the device the user
may want to set also in which IOMMU group to put all the PCIe devices connected
to it. The same goes if kernel is supposed to auto-approve such devices based on
an internal table. The point is that we can think on a configuration where the
devices aren't tunneled yet and the decision about IOMMU can still be changed.

As you mentioned this isn't the common configuration anyway, so it probably
doesn't worth all this hassle.

Re: [PATCH 4/4] thunderbolt: Export IOMMU based DMA protection support to userspace

[Mika Westerberg]

On Tue, Nov 13, 2018 at 05:38:53PM +0200, Yehezkel Bernat wrote:
> Good point. But I thought about per-TBT-device decision. If the platform is
> configured for IOMMU+"user" security level, while approving the device the user
> may want to set also in which IOMMU group to put all the PCIe devices connected
> to it. The same goes if kernel is supposed to auto-approve such devices based on
> an internal table. The point is that we can think on a configuration where the
> devices aren't tunneled yet and the decision about IOMMU can still be changed.

Right, some of these systems have security level set to "user" so there
we could have a way to put the device into passthrough mode before it
appears on the PCIe bus. That would require some sort of API on the
IOMMU side, though.

> As you mentioned this isn't the common configuration anyway, so it probably
> doesn't worth all this hassle.

AFAIK mixing the two is not something they are going to be supporting in
Windows so I would not expect it to be common. I think the ultimate goal
is to move away from security levels towards IOMMU DMA protection so in
future I would expect more and more systems with IOMMU enabled +
security level set to "none".

So I agree with you that it probably is not worth doing at least without
having more data about real performance issues around this. ;-)

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Mika Westerberg]

On Tue, Nov 13, 2018 at 11:45:36AM +0000, Lorenzo Pieralisi wrote:
> On Tue, Nov 13, 2018 at 01:27:00PM +0200, Mika Westerberg wrote:
> 
> [...]
> 
> > > To be frank the concept (and Microsoft _DSD bindings) seems a bit vague
> > > and not thoroughly defined and I would question its detection at
> > > PCI/ACPI core level, I would hope this can be clarified at ACPI
> > > specification level, at least.
> > 
> > I guess that is the way they envision to use _DSD. Instead of having
> > single UUID that covers all properties (like what we have with device
> > properties) they have one UUID per property "class". I certainly hope we
> > don't need to keep extending prp_guids[] array each time they invent
> > another "class" of properties.
> 
> It is even worse than that. This is a unilateral/obscure change that
> won't be part of ACPI specifications (I guess it was easier to add a
> UUID than add this to the ACPI specifications through the AWSG) but it
> is still supposed to be applicable to ACPI PCI bindings on any
> platforms/arches; this way of adding bindings does not work and it
> has to be rectified.

I agree.

For the existing property "classes" such as the one here I don't think
we can do anything. There are systems already with these included in
their ACPI tables.

I wonder if you have any objections regarding this patch?

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Lorenzo Pieralisi]

On Thu, Nov 15, 2018 at 12:22:39PM +0200, Mika Westerberg wrote:
> On Tue, Nov 13, 2018 at 11:45:36AM +0000, Lorenzo Pieralisi wrote:
> > On Tue, Nov 13, 2018 at 01:27:00PM +0200, Mika Westerberg wrote:
> > 
> > [...]
> > 
> > > > To be frank the concept (and Microsoft _DSD bindings) seems a bit vague
> > > > and not thoroughly defined and I would question its detection at
> > > > PCI/ACPI core level, I would hope this can be clarified at ACPI
> > > > specification level, at least.
> > > 
> > > I guess that is the way they envision to use _DSD. Instead of having
> > > single UUID that covers all properties (like what we have with device
> > > properties) they have one UUID per property "class". I certainly hope we
> > > don't need to keep extending prp_guids[] array each time they invent
> > > another "class" of properties.
> > 
> > It is even worse than that. This is a unilateral/obscure change that
> > won't be part of ACPI specifications (I guess it was easier to add a
> > UUID than add this to the ACPI specifications through the AWSG) but it
> > is still supposed to be applicable to ACPI PCI bindings on any
> > platforms/arches; this way of adding bindings does not work and it
> > has to be rectified.
> 
> I agree.
> 
> For the existing property "classes" such as the one here I don't think
> we can do anything. There are systems already with these included in
> their ACPI tables.
> 
> I wonder if you have any objections regarding this patch?

I have strong objections to the way these bindings have been forced upon
everybody; if that's the way *generic* ACPI bindings are specified I
wonder why there still exists an ACPI specification and related working
group.

I personally (but that's Bjorn and Rafael choice) think that this is
not a change that belongs in PCI core, ACPI bindings are ill-defined
and device tree bindings are non-existing.

At the very least Microsoft should be asked to publish and discuss
these bindings within the ACPI and UEFI forums.

Lorenzo

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Mika Westerberg]

On Thu, Nov 15, 2018 at 11:13:56AM +0000, Lorenzo Pieralisi wrote:
> I have strong objections to the way these bindings have been forced upon
> everybody; if that's the way *generic* ACPI bindings are specified I
> wonder why there still exists an ACPI specification and related working
> group.
> 
> I personally (but that's Bjorn and Rafael choice) think that this is
> not a change that belongs in PCI core, ACPI bindings are ill-defined
> and device tree bindings are non-existing.

Any idea where should I put it then? These systems are already out there
and we need to support them one way or another.

> At the very least Microsoft should be asked to publish and discuss
> these bindings within the ACPI and UEFI forums.

These bindings are public, see here:

  https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports

However, they are not part of the ACPI spec as you say.

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Lukas Wunner]

On Thu, Nov 15, 2018 at 01:37:37PM +0200, Mika Westerberg wrote:
> On Thu, Nov 15, 2018 at 11:13:56AM +0000, Lorenzo Pieralisi wrote:
> > I have strong objections to the way these bindings have been forced upon
> > everybody; if that's the way *generic* ACPI bindings are specified I
> > wonder why there still exists an ACPI specification and related working
> > group.
> > 
> > I personally (but that's Bjorn and Rafael choice) think that this is
> > not a change that belongs in PCI core, ACPI bindings are ill-defined
> > and device tree bindings are non-existing.
> 
> Any idea where should I put it then? These systems are already out there
> and we need to support them one way or another.

I suppose those are all Thunderbolt, so could be handled by the
existing ->is_thunderbolt bit?

It was said in this thread that ->is_external is more generic in
that it could also be used on PCIe slots, however that use case
doesn't appear to lend itself to the "plug in while laptop owner
is getting coffee" attack.  To access PCIe slots on a server you
normally need access to a data center.  On a desktop, you usually
have to open the case, by which time the coffee may already have
been fetched.  So frankly the binding seems a bit over-engineered
to me and yet another thing that BIOS writers may get wrong.

Well, just my 2 cents anyway.

Lukas

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Mika Westerberg]

On Thu, Nov 15, 2018 at 01:07:36PM +0100, Lukas Wunner wrote:
> On Thu, Nov 15, 2018 at 01:37:37PM +0200, Mika Westerberg wrote:
> > On Thu, Nov 15, 2018 at 11:13:56AM +0000, Lorenzo Pieralisi wrote:
> > > I have strong objections to the way these bindings have been forced upon
> > > everybody; if that's the way *generic* ACPI bindings are specified I
> > > wonder why there still exists an ACPI specification and related working
> > > group.
> > > 
> > > I personally (but that's Bjorn and Rafael choice) think that this is
> > > not a change that belongs in PCI core, ACPI bindings are ill-defined
> > > and device tree bindings are non-existing.
> > 
> > Any idea where should I put it then? These systems are already out there
> > and we need to support them one way or another.
> 
> I suppose those are all Thunderbolt, so could be handled by the
> existing ->is_thunderbolt bit?
> 
> It was said in this thread that ->is_external is more generic in
> that it could also be used on PCIe slots, however that use case
> doesn't appear to lend itself to the "plug in while laptop owner
> is getting coffee" attack.  To access PCIe slots on a server you
> normally need access to a data center.  On a desktop, you usually
> have to open the case, by which time the coffee may already have
> been fetched.  So frankly the binding seems a bit over-engineered
> to me and yet another thing that BIOS writers may get wrong.

I would not say it should include PCIe slots but there are other cables
that carry PCIe and I was thinking we could make it to support those as
well.

I have no problem using is_thunderbolt here, though if we don't want to
support non-Thunderbolt external devices this way.

However, the question here is more that where I should put the _DSD
parsing code if it is not suitable to be placed inside PCI/ACPI core as
I've done in this patch? ;-)

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Lorenzo Pieralisi]

On Thu, Nov 15, 2018 at 02:16:27PM +0200, Mika Westerberg wrote:
> On Thu, Nov 15, 2018 at 01:07:36PM +0100, Lukas Wunner wrote:
> > On Thu, Nov 15, 2018 at 01:37:37PM +0200, Mika Westerberg wrote:
> > > On Thu, Nov 15, 2018 at 11:13:56AM +0000, Lorenzo Pieralisi wrote:
> > > > I have strong objections to the way these bindings have been forced upon
> > > > everybody; if that's the way *generic* ACPI bindings are specified I
> > > > wonder why there still exists an ACPI specification and related working
> > > > group.
> > > > 
> > > > I personally (but that's Bjorn and Rafael choice) think that this is
> > > > not a change that belongs in PCI core, ACPI bindings are ill-defined
> > > > and device tree bindings are non-existing.
> > > 
> > > Any idea where should I put it then? These systems are already out there
> > > and we need to support them one way or another.
> > 
> > I suppose those are all Thunderbolt, so could be handled by the
> > existing ->is_thunderbolt bit?
> > 
> > It was said in this thread that ->is_external is more generic in
> > that it could also be used on PCIe slots, however that use case
> > doesn't appear to lend itself to the "plug in while laptop owner
> > is getting coffee" attack.  To access PCIe slots on a server you
> > normally need access to a data center.  On a desktop, you usually
> > have to open the case, by which time the coffee may already have
> > been fetched.  So frankly the binding seems a bit over-engineered
> > to me and yet another thing that BIOS writers may get wrong.
> 
> I would not say it should include PCIe slots but there are other cables
> that carry PCIe and I was thinking we could make it to support those as
> well.
> 
> I have no problem using is_thunderbolt here, though if we don't want to
> support non-Thunderbolt external devices this way.
> 
> However, the question here is more that where I should put the _DSD
> parsing code if it is not suitable to be placed inside PCI/ACPI core as
> I've done in this patch? ;-)

Do you really need to parse it if the dev->is_thunderbolt check is enough ?

Thanks,
Lorenzo

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Yehezkel Bernat]

On Thu, Nov 15, 2018 at 7:46 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Thu, Nov 15, 2018 at 02:16:27PM +0200, Mika Westerberg wrote:
> > On Thu, Nov 15, 2018 at 01:07:36PM +0100, Lukas Wunner wrote:
> > > On Thu, Nov 15, 2018 at 01:37:37PM +0200, Mika Westerberg wrote:
> > > > On Thu, Nov 15, 2018 at 11:13:56AM +0000, Lorenzo Pieralisi wrote:
> > > > > I have strong objections to the way these bindings have been forced upon
> > > > > everybody; if that's the way *generic* ACPI bindings are specified I
> > > > > wonder why there still exists an ACPI specification and related working
> > > > > group.
> > > > >
> > > > > I personally (but that's Bjorn and Rafael choice) think that this is
> > > > > not a change that belongs in PCI core, ACPI bindings are ill-defined
> > > > > and device tree bindings are non-existing.
> > > >
> > > > Any idea where should I put it then? These systems are already out there
> > > > and we need to support them one way or another.
> > >
> > > I suppose those are all Thunderbolt, so could be handled by the
> > > existing ->is_thunderbolt bit?
> > >
> > > It was said in this thread that ->is_external is more generic in
> > > that it could also be used on PCIe slots, however that use case
> > > doesn't appear to lend itself to the "plug in while laptop owner
> > > is getting coffee" attack.  To access PCIe slots on a server you
> > > normally need access to a data center.  On a desktop, you usually
> > > have to open the case, by which time the coffee may already have
> > > been fetched.  So frankly the binding seems a bit over-engineered
> > > to me and yet another thing that BIOS writers may get wrong.
> >
> > I would not say it should include PCIe slots but there are other cables
> > that carry PCIe and I was thinking we could make it to support those as
> > well.
> >
> > I have no problem using is_thunderbolt here, though if we don't want to
> > support non-Thunderbolt external devices this way.
> >
> > However, the question here is more that where I should put the _DSD
> > parsing code if it is not suitable to be placed inside PCI/ACPI core as
> > I've done in this patch? ;-)
>
> Do you really need to parse it if the dev->is_thunderbolt check is enough ?

From what I know, there are more devices that suffer from similar security
issues like Thunderbolt, e.g. FireWire [1].
My assumption is that the same protection may be applied to such devices too,
even if currently it sounds like vendors care mostly about Thunderbolt (probably
because it removes the need for user approval for device connection; it becames
a simple plug-and-play experience).

Thus, I don't think binding it with dev->is_thunderbolt is the correct
thing to do.

[1] https://github.com/carmaa/inception

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Mika Westerberg]

On Thu, Nov 15, 2018 at 05:46:08PM +0000, Lorenzo Pieralisi wrote:
> Do you really need to parse it if the dev->is_thunderbolt check is enough ?

Yes, we need to parse it one way or another. dev->is_thunderbolt is
based on heuristics which do not apply anymore when the thing gets
integrated in the SoC.

The _DSD is there already (on existing systems) and is being used by
Windows so I don't understand why we cannot take advantage of it? Every
new system with Thunderbolt ports will have it.

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Mika Westerberg]

On Thu, Nov 15, 2018 at 07:58:13PM +0200, Yehezkel Bernat wrote:
> From what I know, there are more devices that suffer from similar security
> issues like Thunderbolt, e.g. FireWire [1].
> My assumption is that the same protection may be applied to such devices too,
> even if currently it sounds like vendors care mostly about Thunderbolt (probably
> because it removes the need for user approval for device connection; it becames
> a simple plug-and-play experience).

FireWire is kind of different but there are connectors such as
ExpressCard and NVMe (over U.2 connector) which carry PCIe and are
relatively easy to access without need for a screwdriver. AFAIK some
eGPUs are also using some other proprietary (non-TBT) connector that
carries PCIe.

I was thinking we could cover all these with is_external filling them
based on the _DSD or some other means in the kernel.

We would then deal all such devices as "untrusted" by default.

> Thus, I don't think binding it with dev->is_thunderbolt is the correct
> thing to do.

One option that I suggested already is that we keep both and mark all
is_thunderbolt devices as is_external as well. But I guess this is up to
Bjorn and Rafael to decide :-)

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Lukas Wunner]

On Thu, Nov 15, 2018 at 09:10:26PM +0200, Mika Westerberg wrote:
> I was thinking we could cover all these with is_external filling them
> based on the _DSD or some other means in the kernel.
> 
> We would then deal all such devices as "untrusted" by default.

Tinfoil hat on, even internal devices could be malicious.
What's the downside of enabling the feature for everything?

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Mika Westerberg]

On Thu, Nov 15, 2018 at 08:27:41PM +0100, Lukas Wunner wrote:
> On Thu, Nov 15, 2018 at 09:10:26PM +0200, Mika Westerberg wrote:
> > I was thinking we could cover all these with is_external filling them
> > based on the _DSD or some other means in the kernel.
> > 
> > We would then deal all such devices as "untrusted" by default.
> 
> Tinfoil hat on, even internal devices could be malicious.
> What's the downside of enabling the feature for everything?

Mostly performance, I think. That's the main reason we put all non
external devices to passthrough IOMMU mode.

RE: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Mario.Limonciello]

> -----Original Message-----
> From: Mika Westerberg <mika.westerberg@linux.intel.com>
> Sent: Thursday, November 15, 2018 1:01 PM
> To: Lorenzo Pieralisi
> Cc: Lukas Wunner; iommu@lists.linux-foundation.org; Joerg Roedel; David
> Woodhouse; Lu Baolu; Ashok Raj; Bjorn Helgaas; Rafael J. Wysocki; Jacob jun Pan;
> Andreas Noever; Michael Jamet; Yehezkel Bernat; Christian Kellner; Limonciello,
> Mario; Anthony Wong; linux-acpi@vger.kernel.org; linux-pci@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices
> 
> 
> [EXTERNAL EMAIL]
> 
> On Thu, Nov 15, 2018 at 05:46:08PM +0000, Lorenzo Pieralisi wrote:
> > Do you really need to parse it if the dev->is_thunderbolt check is enough ?
> 
> Yes, we need to parse it one way or another. dev->is_thunderbolt is
> based on heuristics which do not apply anymore when the thing gets
> integrated in the SoC.
> 
> The _DSD is there already (on existing systems) and is being used by
> Windows so I don't understand why we cannot take advantage of it? Every
> new system with Thunderbolt ports will have it.

Furthermore it's entirely in the BIOS writers best interest to do this correctly
as it applies proper policy on Windows as well.

I wouldn't be surprised if WHCK failed if it was done wrong.

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Mika Westerberg]

On Thu, Nov 15, 2018 at 09:00:54PM +0200, Mika Westerberg wrote:
> On Thu, Nov 15, 2018 at 05:46:08PM +0000, Lorenzo Pieralisi wrote:
> > Do you really need to parse it if the dev->is_thunderbolt check is enough ?
> 
> Yes, we need to parse it one way or another. dev->is_thunderbolt is
> based on heuristics which do not apply anymore when the thing gets
> integrated in the SoC.
> 
> The _DSD is there already (on existing systems) and is being used by
> Windows so I don't understand why we cannot take advantage of it? Every
> new system with Thunderbolt ports will have it.

Just to clarify a bit. We can use is_thunderbolt in place of is_external
if we don't want to deal with other possible "external" devices right now.

However, we still need to parse the _DSD and based on that fill the
is_thunderbolt for these devices (same way we do for is_external in this
series). So basically we just get rid of the is_external flag and use
is_thunderbolt instead.

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Christoph Hellwig]

On Thu, Nov 15, 2018 at 09:10:26PM +0200, Mika Westerberg wrote:
> FireWire is kind of different but there are connectors such as
> ExpressCard and NVMe (over U.2 connector) which carry PCIe and are
> relatively easy to access without need for a screwdriver. AFAIK some
> eGPUs are also using some other proprietary (non-TBT) connector that
> carries PCIe.

U.2 is a data center internal form factor with hot plug capability. If
you enable an iommu for that by default you will make a lot of people
very unhappy.

More importantly NVMe is now used for the current/next generation
Compact Flash and SD cards, which contain full PCIe gen 3 links.

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Mika Westerberg]

On Fri, Nov 16, 2018 at 01:18:04AM -0800, Christoph Hellwig wrote:
> On Thu, Nov 15, 2018 at 09:10:26PM +0200, Mika Westerberg wrote:
> > FireWire is kind of different but there are connectors such as
> > ExpressCard and NVMe (over U.2 connector) which carry PCIe and are
> > relatively easy to access without need for a screwdriver. AFAIK some
> > eGPUs are also using some other proprietary (non-TBT) connector that
> > carries PCIe.
> 
> U.2 is a data center internal form factor with hot plug capability. If
> you enable an iommu for that by default you will make a lot of people
> very unhappy.

Well, it needs the other bit in ACPI DMAR table to be enabled by default
so I don't think anyone in data center domain will notice ;-)

> More importantly NVMe is now used for the current/next generation
> Compact Flash and SD cards, which contain full PCIe gen 3 links.

OK, thanks for the information - I did not know that. I guess those
belong to the "external" category as well.

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Lorenzo Pieralisi]

On Thu, Nov 15, 2018 at 07:33:54PM +0000, Mario.Limonciello@dell.com wrote:
> 
> 
> > -----Original Message-----
> > From: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Sent: Thursday, November 15, 2018 1:01 PM
> > To: Lorenzo Pieralisi
> > Cc: Lukas Wunner; iommu@lists.linux-foundation.org; Joerg Roedel; David
> > Woodhouse; Lu Baolu; Ashok Raj; Bjorn Helgaas; Rafael J. Wysocki; Jacob jun Pan;
> > Andreas Noever; Michael Jamet; Yehezkel Bernat; Christian Kellner; Limonciello,
> > Mario; Anthony Wong; linux-acpi@vger.kernel.org; linux-pci@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices
> > 
> > 
> > [EXTERNAL EMAIL]
> > 
> > On Thu, Nov 15, 2018 at 05:46:08PM +0000, Lorenzo Pieralisi wrote:
> > > Do you really need to parse it if the dev->is_thunderbolt check is enough ?
> > 
> > Yes, we need to parse it one way or another. dev->is_thunderbolt is
> > based on heuristics which do not apply anymore when the thing gets
> > integrated in the SoC.
> > 
> > The _DSD is there already (on existing systems) and is being used by
> > Windows so I don't understand why we cannot take advantage of it? Every
> > new system with Thunderbolt ports will have it.

We have different opinions on this, there is no point in me reiterating
it over and over, I am against the approach taken to solve this problem
first in defining the bindings outside the ACPI specifications and
second by acquiescing to what has been done so that it will be done
over and over again.

I will raise the point in the appropriate forum, it is up to Bjorn
and Rafael to decide on this patch.

Lorenzo

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Rafael J. Wysocki]

On Friday, November 16, 2018 11:57:38 AM CET Lorenzo Pieralisi wrote:
> On Thu, Nov 15, 2018 at 07:33:54PM +0000, Mario.Limonciello@dell.com wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > Sent: Thursday, November 15, 2018 1:01 PM
> > > To: Lorenzo Pieralisi
> > > Cc: Lukas Wunner; iommu@lists.linux-foundation.org; Joerg Roedel; David
> > > Woodhouse; Lu Baolu; Ashok Raj; Bjorn Helgaas; Rafael J. Wysocki; Jacob jun Pan;
> > > Andreas Noever; Michael Jamet; Yehezkel Bernat; Christian Kellner; Limonciello,
> > > Mario; Anthony Wong; linux-acpi@vger.kernel.org; linux-pci@vger.kernel.org; linux-
> > > kernel@vger.kernel.org
> > > Subject: Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices
> > > 
> > > 
> > > [EXTERNAL EMAIL]
> > > 
> > > On Thu, Nov 15, 2018 at 05:46:08PM +0000, Lorenzo Pieralisi wrote:
> > > > Do you really need to parse it if the dev->is_thunderbolt check is enough ?
> > > 
> > > Yes, we need to parse it one way or another. dev->is_thunderbolt is
> > > based on heuristics which do not apply anymore when the thing gets
> > > integrated in the SoC.
> > > 
> > > The _DSD is there already (on existing systems) and is being used by
> > > Windows so I don't understand why we cannot take advantage of it? Every
> > > new system with Thunderbolt ports will have it.
> 
> We have different opinions on this, there is no point in me reiterating
> it over and over, I am against the approach taken to solve this problem
> first in defining the bindings outside the ACPI specifications and
> second by acquiescing to what has been done so that it will be done
> over and over again.

Arguably, however, we are on the receiving end of things here and even if
we don't use this binding, that won't buy us anything (but it will introduce
a fair amount of disappointment among both users and OEMs).

If Windows uses it, then systems will ship with it regardless of what Linux
does with it, so your "acquiescing to what has been done" argument leads to
nowhere in this particular case.  It's just a matter of whether or not
Linux will provide the same level of functionality as Windows with respect
to this and IMO it would be impractical to refuse to do that for purely
formal reasons.

> I will raise the point in the appropriate forum, it is up to Bjorn
> and Rafael to decide on this patch.

For the record, my opinion is that there's a little choice on whether or not
to use this extra information that firmware is willing to give us.  It could
be defined in a better way and so on, but since it is in use anyway, we really
have nothing to gain by refusing to use it.

Now, where the handling of it belongs to is a separate matter that should be
decided on its own.

Thanks,
Rafael

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Lorenzo Pieralisi]

On Tue, Nov 20, 2018 at 10:43:35PM +0100, Rafael J. Wysocki wrote:
> On Friday, November 16, 2018 11:57:38 AM CET Lorenzo Pieralisi wrote:
> > On Thu, Nov 15, 2018 at 07:33:54PM +0000, Mario.Limonciello@dell.com wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > Sent: Thursday, November 15, 2018 1:01 PM
> > > > To: Lorenzo Pieralisi
> > > > Cc: Lukas Wunner; iommu@lists.linux-foundation.org; Joerg Roedel; David
> > > > Woodhouse; Lu Baolu; Ashok Raj; Bjorn Helgaas; Rafael J. Wysocki; Jacob jun Pan;
> > > > Andreas Noever; Michael Jamet; Yehezkel Bernat; Christian Kellner; Limonciello,
> > > > Mario; Anthony Wong; linux-acpi@vger.kernel.org; linux-pci@vger.kernel.org; linux-
> > > > kernel@vger.kernel.org
> > > > Subject: Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices
> > > > 
> > > > 
> > > > [EXTERNAL EMAIL]
> > > > 
> > > > On Thu, Nov 15, 2018 at 05:46:08PM +0000, Lorenzo Pieralisi wrote:
> > > > > Do you really need to parse it if the dev->is_thunderbolt check is enough ?
> > > > 
> > > > Yes, we need to parse it one way or another. dev->is_thunderbolt is
> > > > based on heuristics which do not apply anymore when the thing gets
> > > > integrated in the SoC.
> > > > 
> > > > The _DSD is there already (on existing systems) and is being used by
> > > > Windows so I don't understand why we cannot take advantage of it? Every
> > > > new system with Thunderbolt ports will have it.
> > 
> > We have different opinions on this, there is no point in me reiterating
> > it over and over, I am against the approach taken to solve this problem
> > first in defining the bindings outside the ACPI specifications and
> > second by acquiescing to what has been done so that it will be done
> > over and over again.
> 
> Arguably, however, we are on the receiving end of things here and even if
> we don't use this binding, that won't buy us anything (but it will introduce
> a fair amount of disappointment among both users and OEMs).
> 
> If Windows uses it, then systems will ship with it regardless of what Linux
> does with it, so your "acquiescing to what has been done" argument leads to
> nowhere in this particular case.  It's just a matter of whether or not
> Linux will provide the same level of functionality as Windows with respect
> to this and IMO it would be impractical to refuse to do that for purely
> formal reasons.
> 
> > I will raise the point in the appropriate forum, it is up to Bjorn
> > and Rafael to decide on this patch.
> 
> For the record, my opinion is that there's a little choice on whether
> or not to use this extra information that firmware is willing to give
> us.  It could be defined in a better way and so on, but since it is in
> use anyway, we really have nothing to gain by refusing to use it.

AFAIK PCI firmware bindings should go into PCI firmware specifications,
not Microsoft webpages.

If we deviate from this model there is no way to tell whether that extra
information is right or wrong, it is not necessarily about this patch,
it is about changing the way these bindings are deployed in future
systems.

> Now, where the handling of it belongs to is a separate matter that should be
> decided on its own.

I think that the way these bindings were deployed is wrong, I agree this
is not the right forum to discuss that though. What you will do with
this patch is not my call anyway, I just expressed my opinion.

Thanks,
Lorenzo

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Mika Westerberg]

On Fri, Nov 16, 2018 at 11:32:10AM +0200, Mika Westerberg wrote:
> On Fri, Nov 16, 2018 at 01:18:04AM -0800, Christoph Hellwig wrote:
> > On Thu, Nov 15, 2018 at 09:10:26PM +0200, Mika Westerberg wrote:
> > > FireWire is kind of different but there are connectors such as
> > > ExpressCard and NVMe (over U.2 connector) which carry PCIe and are
> > > relatively easy to access without need for a screwdriver. AFAIK some
> > > eGPUs are also using some other proprietary (non-TBT) connector that
> > > carries PCIe.
> > 
> > U.2 is a data center internal form factor with hot plug capability. If
> > you enable an iommu for that by default you will make a lot of people
> > very unhappy.
> 
> Well, it needs the other bit in ACPI DMAR table to be enabled by default
> so I don't think anyone in data center domain will notice ;-)
> 
> > More importantly NVMe is now used for the current/next generation
> > Compact Flash and SD cards, which contain full PCIe gen 3 links.
> 
> OK, thanks for the information - I did not know that. I guess those
> belong to the "external" category as well.

We had an internal discussion regarding this and it was suggested that
the new flag is called "is_untrusted" instead of "is_external". This
covers Thunderbolt devices currently but can be extend to any other PCIe
device such as "SD express" ones. When IOMMU is turned on it will then
make sure devices with "is_untrusted" set are always using full IOMMU
protection.

Any comments, objections? I was going to send v2 with this change
included.

Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Christoph Hellwig]

On Thu, Nov 22, 2018 at 12:48:40PM +0200, Mika Westerberg wrote:
> We had an internal discussion regarding this and it was suggested that
> the new flag is called "is_untrusted" instead of "is_external". This
> covers Thunderbolt devices currently but can be extend to any other PCIe
> device such as "SD express" ones. When IOMMU is turned on it will then
> make sure devices with "is_untrusted" set are always using full IOMMU
> protection.
> 
> Any comments, objections? I was going to send v2 with this change
> included.

Sounds good to me as long as it goes along with a nice fat comment
explaining it.