Re: [PATCH v2 6/7] PCI: Move pci_dev->untrusted logic to use device location instead

From: Lu Baolu
Date: Tue Jun 30 2020 - 03:40:13 EST

On 2020/6/30 12:49, Rajat Jain wrote:
The firmware was provinding "ExternalFacing" attribute on PCI root ports,
to allow the kernel to mark devices behind it as external. Note that the
firmware provides an immutable, read-only property, i.e. the location of
the device.

The use of (external) device location as hint for (dis)trust, is a
decision that IOMMU drivers have taken, so we should call it out
explicitly.

This patch removes the pci_dev->untrusted and changes the users of it to
use device core provided device location instead. This location is
populated by PCI using the same "ExternalFacing" firmware info. Any
device not behind the "ExternalFacing" bridges are marked internal and
the ones behind such bridges are markes external.

Signed-off-by: Rajat Jain <rajatja@xxxxxxxxxx>

For changes in Intel VT-d driver,

Reviewed-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>

Best regards,
baolu

---
v2: (Initial version)

drivers/iommu/intel/iommu.c | 31 +++++++++++++++++++++----------
drivers/pci/ats.c | 2 +-
drivers/pci/pci-driver.c | 1 +
drivers/pci/pci.c | 2 +-
drivers/pci/probe.c | 18 ++++++++++++------
drivers/pci/quirks.c | 2 +-
include/linux/pci.h | 10 +---------
7 files changed, 38 insertions(+), 28 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 1ccb224f82496..ca66a196f5e97 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -168,6 +168,22 @@ static inline unsigned long virt_to_dma_pfn(void *p)
return page_to_dma_pfn(virt_to_page(p));
}
+static inline bool untrusted_dev(struct device *dev)
+{
+ /*
+ * Treat all external PCI devices as untrusted devices. These are the
+ * devices behing marked behind external-facing bridges as marked by
+ * the firmware. The untrusted devices are the ones that can potentially
+ * execute DMA attacks and similar. They are typically connected through
+ * external thunderbolt ports. When an IOMMU is enabled they should be
+ * getting full mappings to ensure they cannot access arbitrary memory.
+ */
+ if (dev_is_pci(dev) && dev_is_external(dev))
+ return true;
+
+ return false;
+}
+
/* global iommu list, set NULL for ignored DMAR units */
static struct intel_iommu **g_iommus;
@@ -383,8 +399,7 @@ struct device_domain_info *get_domain_info(struct device *dev)
DEFINE_SPINLOCK(device_domain_lock);
static LIST_HEAD(device_domain_list);
-#define device_needs_bounce(d) (!intel_no_bounce && dev_is_pci(d) && \
- to_pci_dev(d)->untrusted)
+#define device_needs_bounce(d) (!intel_no_bounce && untrusted_dev(d))
/*
* Iterate over elements in device_domain_list and call the specified
@@ -2830,7 +2845,7 @@ static int device_def_domain_type(struct device *dev)
* Prevent any device marked as untrusted from getting
* placed into the statically identity mapping domain.
*/
- if (pdev->untrusted)
+ if (untrusted_dev(dev))
return IOMMU_DOMAIN_DMA;
if ((iommu_identity_mapping & IDENTMAP_AZALIA) && IS_AZALIA(pdev))
@@ -3464,7 +3479,6 @@ static void intel_unmap(struct device *dev, dma_addr_t dev_addr, size_t size)
unsigned long iova_pfn;
struct intel_iommu *iommu;
struct page *freelist;
- struct pci_dev *pdev = NULL;
domain = find_domain(dev);
BUG_ON(!domain);
@@ -3477,11 +3491,8 @@ static void intel_unmap(struct device *dev, dma_addr_t dev_addr, size_t size)
start_pfn = mm_to_dma_pfn(iova_pfn);
last_pfn = start_pfn + nrpages - 1;
- if (dev_is_pci(dev))
- pdev = to_pci_dev(dev);
-
freelist = domain_unmap(domain, start_pfn, last_pfn);
- if (intel_iommu_strict || (pdev && pdev->untrusted) ||
+ if (intel_iommu_strict || untrusted_dev(dev) ||
!has_iova_flush_queue(&domain->iovad)) {
iommu_flush_iotlb_psi(iommu, domain, start_pfn,
nrpages, !freelist, 0);
@@ -4743,7 +4754,7 @@ static inline bool has_untrusted_dev(void)
struct pci_dev *pdev = NULL;
for_each_pci_dev(pdev)
- if (pdev->untrusted || pdev->external_facing)
+ if (pdev->external_facing || untrusted_dev(&pdev->dev))
return true;
return false;
@@ -6036,7 +6047,7 @@ intel_iommu_domain_set_attr(struct iommu_domain *domain,
*/
static bool risky_device(struct pci_dev *pdev)
{
- if (pdev->untrusted) {
+ if (untrusted_dev(&pdev->dev)) {
pci_info(pdev,
"Skipping IOMMU quirk for dev [%04X:%04X] on untrusted PCI link\n",
pdev->vendor, pdev->device);
diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index b761c1f72f672..ebd370f4d5b06 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -42,7 +42,7 @@ bool pci_ats_supported(struct pci_dev *dev)
if (!dev->ats_cap)
return false;
- return (dev->untrusted == 0);
+ return (!dev_is_external(&dev->dev));
}
EXPORT_SYMBOL_GPL(pci_ats_supported);
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index da6510af12214..9608053a8a62c 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1630,6 +1630,7 @@ struct bus_type pci_bus_type = {
.pm = PCI_PM_OPS_PTR,
.num_vf = pci_bus_num_vf,
.dma_configure = pci_dma_configure,
+ .supports_site = true,
};
EXPORT_SYMBOL(pci_bus_type);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 79853b52658a2..35f25ac39167b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3330,7 +3330,7 @@ static void pci_std_enable_acs(struct pci_dev *dev)
/* Upstream Forwarding */
ctrl |= (cap & PCI_ACS_UF);
- if (dev->external_facing || dev->untrusted)
+ if (dev->external_facing || dev_is_external(&dev->dev))
/* Translation Blocking */
ctrl |= (cap & PCI_ACS_TB);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8c40c00413e74..1609329cc5b4e 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1543,17 +1543,23 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
}
}
-static void set_pcie_untrusted(struct pci_dev *dev)
+static void set_pcie_dev_site(struct pci_dev *dev)
{
struct pci_dev *parent;
/*
- * If the upstream bridge is untrusted we treat this device
- * untrusted as well.
+ * All devices are considered internal by default, unless behind an
+ * external-facing bridge, as marked by the firmware.
+ */
+ dev_set_site(&dev->dev, SITE_INTERNAL);
+
+ /*
+ * If the upstream bridge is external or external-facing, this device
+ * is also external.
*/
parent = pci_upstream_bridge(dev);
- if (parent && (parent->untrusted || parent->external_facing))
- dev->untrusted = true;
+ if (parent && (parent->external_facing || dev_is_external(&parent->dev)))
+ dev_set_site(&dev->dev, SITE_EXTERNAL);
}
/**
@@ -1814,7 +1820,7 @@ int pci_setup_device(struct pci_dev *dev)
/* Need to have dev->cfg_size ready */
set_pcie_thunderbolt(dev);
- set_pcie_untrusted(dev);
+ set_pcie_dev_site(dev);
/* "Unknown power state" */
dev->current_state = PCI_UNKNOWN;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 6294adeac4049..65d0b8745c915 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4980,7 +4980,7 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev)
ctrl |= (cap & PCI_ACS_CR);
ctrl |= (cap & PCI_ACS_UF);
- if (dev->external_facing || dev->untrusted)
+ if (dev->external_facing || dev_is_external(&dev->dev))
/* Translation Blocking */
ctrl |= (cap & PCI_ACS_TB);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index fe1bc603fda40..8bb5065e5aed2 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -424,20 +424,12 @@ 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 */
- /*
- * Devices marked being untrusted are the ones that can potentially
- * execute DMA attacks and similar. They are typically connected
- * through external ports such as Thunderbolt but not limited to
- * that. When an IOMMU is enabled they should be getting full
- * mappings to make sure they cannot access arbitrary memory.
- */
- unsigned int untrusted:1;
/*
* Devices are marked as external-facing using info from platform
* (ACPI / devicetree). An external-facing device is still an internal
* trusted device, but it faces external untrusted devices. Thus any
* devices enumerated downstream an external-facing device is marked
- * as untrusted.
+ * as external device.
*/
unsigned int external_facing:1;
unsigned int broken_intx_masking:1; /* INTx masking can't be used */