Some computers with CPUs that lack Thunderbolt features use discrete
Thunderbolt chips to add Thunderbolt functionality. These Thunderbolt
chips are located within the chassis; between the root port labeled
ExternalFacingPort and the USB-C port.

These Thunderbolt PCIe devices should be labeled as fixed and trusted,
as they are built into the computer. Otherwise, security policies that
rely on those flags may have unintended results, such as preventing
USB-C ports from enumerating.

Detect the above scenario through the process of elimination.

1) Integrated Thunderbolt host controllers already have Thunderbolt
   implemented, so anything outside their external facing root port is
   removable and untrusted.

   Detect them using the following properties:

     - Most integrated host controllers have the usb4-host-interface
       ACPI property, as described here:
Link: https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#mapping-native-protocols-pcie-displayport-tunneled-through-usb4-to-usb4-host-routers

     - Integrated Thunderbolt PCIe root ports before Alder Lake do not
       have the usb4-host-interface ACPI property. Identify those with
       their PCI IDs instead.

2) If a root port does not have integrated Thunderbolt capabilities, but
   has the ExternalFacingPort ACPI property, that means the manufacturer
   has opted to use a discrete Thunderbolt host controller that is
   built into the computer.

   This host controller can be identified by virtue of being located
   directly below an external-facing root port that lacks integrated
   Thunderbolt. Label it as trusted and fixed.

   Everything downstream from it is untrusted and removable.

The ExternalFacingPort ACPI property is described here:
Link: https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-externally-exposed-pcie-root-ports

Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Esther Shimanovich <eshimanovich@chromium.org>
Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Tested-by: Mario Limonciello <mario.limonciello@amd.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
---
While working with devices that have discrete Thunderbolt chips, I
noticed that their internal TBT chips are inaccurately labeled as
untrusted and removable.

I've observed that this issue impacts all computers with internal,
discrete Intel JHL Thunderbolt chips, such as JHL6240, JHL6340, JHL6540,
and JHL7540, across multiple device manufacturers such as Lenovo, Dell,
and HP.

This affects the execution of any downstream security policy that
relies on the "untrusted" or "removable" flags.

I initially submitted a quirk to resolve this, which was too small in
scope, and after some discussion, Mika proposed a more thorough fix:
https://lore.kernel.org/lkml/20240510052616.GC4162345@black.fi.intel.com
I refactored it and am submitting as a new patch.
---
Changes in v5:
- Applied the following edits suggested by Lukas Wunner:
  - Applied ifdefs edits to ensure code is only compiled on x86 systems
  with ACPI
  - Added returns to avoid unecessary checks
  - Renamed pcie_is_tunneled to arch_pci_dev_is_removable
- Link to v4: https://lore.kernel.org/r/20240823-trust-tbt-fix-v4-1-c6f1e3bdd9be@chromium.org

Changes in v4:
- Applied edits on logic-flow clarity and formatting suggested by Ilpo
  Järvinen
- Mario Limonciello tested patch and confirmed works as intended.
- Link to v3: https://lore.kernel.org/r/20240815-trust-tbt-fix-v3-1-6ba01865d54c@chromium.org

Changes in v3:
- Incorporated minor edits suggested by Mika Westerberg.
- Mika Westerberg tested patch (more details in v2 link)
- Added "reviewed-by" and "tested-by" lines
- Link to v2: https://lore.kernel.org/r/20240808-trust-tbt-fix-v2-1-2e34a05a9186@chromium.org

Changes in v2:
- I clarified some comments, and made minor fixins
- I also added a more detailed description of implementation into the
  commit message
- Added Cc recipients Mike recommended
- Link to v1: https://lore.kernel.org/r/20240806-trust-tbt-fix-v1-1-73ae5f446d5a@chromium.org
---
 arch/x86/pci/acpi.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/probe.c |  30 +++++++++----
 include/linux/pci.h |   6 +++
 3 files changed, 148 insertions(+), 7 deletions(-)

diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index 55c4b07ec1f6..62271668c3b1 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -250,6 +250,125 @@ void __init pci_acpi_crs_quirks(void)
 		pr_info("Please notify linux-pci@vger.kernel.org so future kernels can do this automatically\n");
 }
 
+/*
+ * Checks if pdev is part of a PCIe switch that is directly below the
+ * specified bridge.
+ */
+static bool pcie_switch_directly_under(struct pci_dev *bridge,
+				       struct pci_dev *pdev)
+{
+	struct pci_dev *parent = pci_upstream_bridge(pdev);
+
+	/* If the device doesn't have a parent, it's not under anything. */
+	if (!parent)
+		return false;
+
+	/*
+	 * If the device has a PCIe type, check if it is below the
+	 * corresponding PCIe switch components (if applicable). Then check
+	 * if its upstream port is directly beneath the specified bridge.
+	 */
+	switch (pci_pcie_type(pdev)) {
+	case PCI_EXP_TYPE_UPSTREAM:
+		return parent == bridge;
+
+	case PCI_EXP_TYPE_DOWNSTREAM:
+		if (pci_pcie_type(parent) != PCI_EXP_TYPE_UPSTREAM)
+			return false;
+		parent = pci_upstream_bridge(parent);
+		return parent == bridge;
+
+	case PCI_EXP_TYPE_ENDPOINT:
+		if (pci_pcie_type(parent) != PCI_EXP_TYPE_DOWNSTREAM)
+			return false;
+		parent = pci_upstream_bridge(parent);
+		if (!parent || pci_pcie_type(parent) != PCI_EXP_TYPE_UPSTREAM)
+			return false;
+		parent = pci_upstream_bridge(parent);
+		return parent == bridge;
+	}
+
+	return false;
+}
+
+static bool pcie_has_usb4_host_interface(struct pci_dev *pdev)
+{
+	struct fwnode_handle *fwnode;
+
+	/*
+	 * For USB4, the tunneled PCIe root or downstream ports are marked
+	 * with the "usb4-host-interface" ACPI property, so we look for
+	 * that first. This should cover most cases.
+	 */
+	fwnode = fwnode_find_reference(dev_fwnode(&pdev->dev),
+				       "usb4-host-interface", 0);
+	if (!IS_ERR(fwnode)) {
+		fwnode_handle_put(fwnode);
+		return true;
+	}
+
+	/*
+	 * Any integrated Thunderbolt 3/4 PCIe root ports from Intel
+	 * before Alder Lake do not have the "usb4-host-interface"
+	 * property so we use their PCI IDs instead. All these are
+	 * tunneled. This list is not expected to grow.
+	 */
+	if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
+		switch (pdev->device) {
+		/* Ice Lake Thunderbolt 3 PCIe Root Ports */
+		case 0x8a1d:
+		case 0x8a1f:
+		case 0x8a21:
+		case 0x8a23:
+		/* Tiger Lake-LP Thunderbolt 4 PCIe Root Ports */
+		case 0x9a23:
+		case 0x9a25:
+		case 0x9a27:
+		case 0x9a29:
+		/* Tiger Lake-H Thunderbolt 4 PCIe Root Ports */
+		case 0x9a2b:
+		case 0x9a2d:
+		case 0x9a2f:
+		case 0x9a31:
+			return true;
+		}
+	}
+
+	return false;
+}
+
+bool arch_pci_dev_is_removable(struct pci_dev *pdev)
+{
+	struct pci_dev *parent, *root;
+
+	/* pdev without a parent or Root Port is never tunneled. */
+	parent = pci_upstream_bridge(pdev);
+	if (!parent)
+		return false;
+	root = pcie_find_root_port(pdev);
+	if (!root)
+		return false;
+
+	/* Internal PCIe devices are not tunneled. */
+	if (!root->external_facing)
+		return false;
+
+	/* Anything directly behind a "usb4-host-interface" is tunneled. */
+	if (pcie_has_usb4_host_interface(parent))
+		return true;
+
+	/*
+	 * Check if this is a discrete Thunderbolt/USB4 controller that is
+	 * directly behind the non-USB4 PCIe Root Port marked as
+	 * "ExternalFacingPort". Those are not behind a PCIe tunnel.
+	 */
+	if (pcie_switch_directly_under(root, pdev))
+		return false;
+
+	/* PCIe devices after the discrete chip are tunneled. */
+	return true;
+}
+
 #ifdef	CONFIG_PCI_MMCONFIG
 static int check_segment(u16 seg, struct device *dev, char *estr)
 {
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index b14b9876c030..7e0d6a1e151b 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1631,23 +1631,33 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
 
 static void set_pcie_untrusted(struct pci_dev *dev)
 {
-	struct pci_dev *parent;
+	struct pci_dev *parent = pci_upstream_bridge(dev);
 
+	if (!parent)
+		return;
 	/*
-	 * If the upstream bridge is untrusted we treat this device
+	 * If the upstream bridge is untrusted we treat this device as
 	 * untrusted as well.
 	 */
-	parent = pci_upstream_bridge(dev);
-	if (parent && (parent->untrusted || parent->external_facing))
+	if (parent->untrusted) {
+		dev->untrusted = true;
+		return;
+	}
+
+	if (arch_pci_dev_is_removable(dev)) {
+		pci_dbg(dev, "marking as untrusted\n");
 		dev->untrusted = true;
+	}
 }
 
 static void pci_set_removable(struct pci_dev *dev)
 {
 	struct pci_dev *parent = pci_upstream_bridge(dev);
 
+	if (!parent)
+		return;
 	/*
-	 * We (only) consider everything downstream from an external_facing
+	 * We (only) consider everything tunneled below an external_facing
 	 * device to be removable by the user. We're mainly concerned with
 	 * consumer platforms with user accessible thunderbolt ports that are
 	 * vulnerable to DMA attacks, and we expect those ports to be marked by
@@ -1657,9 +1667,15 @@ static void pci_set_removable(struct pci_dev *dev)
 	 * accessible to user / may not be removed by end user, and thus not
 	 * exposed as "removable" to userspace.
 	 */
-	if (parent &&
-	    (parent->external_facing || dev_is_removable(&parent->dev)))
+	if (dev_is_removable(&parent->dev)) {
+		dev_set_removable(&dev->dev, DEVICE_REMOVABLE);
+		return;
+	}
+
+	if (arch_pci_dev_is_removable(dev)) {
+		pci_dbg(dev, "marking as removable\n");
 		dev_set_removable(&dev->dev, DEVICE_REMOVABLE);
+	}
 }
 
 /**
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 9e36b6c1810e..dca19203bc7c 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2604,6 +2604,12 @@ pci_host_bridge_acpi_msi_domain(struct pci_bus *bus) { return NULL; }
 static inline bool pci_pr3_present(struct pci_dev *pdev) { return false; }
 #endif
 
+#if defined(CONFIG_X86) && defined(CONFIG_ACPI)
+bool arch_pci_dev_is_removable(struct pci_dev *pdev);
+#else
+static inline bool arch_pci_dev_is_removable(struct pci_dev *pdev) { return false; }
+#endif
+
 #ifdef CONFIG_EEH
 static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
 {

---
base-commit: 3f386cb8ee9f04ff4be164ca7a1d0ef3f81f7374
change-id: 20240806-trust-tbt-fix-5f337fd9ec8a

Best regards,
-- 
Esther Shimanovich <eshimanovich@chromium.org>

On Tue, Sep 10, 2024 at 05:57:45PM +0000, Esther Shimanovich wrote:
> Some computers with CPUs that lack Thunderbolt features use discrete
> Thunderbolt chips to add Thunderbolt functionality. These Thunderbolt
> chips are located within the chassis; between the root port labeled
> ExternalFacingPort and the USB-C port.
> 
> These Thunderbolt PCIe devices should be labeled as fixed and trusted,
> as they are built into the computer. Otherwise, security policies that
> rely on those flags may have unintended results, such as preventing
> USB-C ports from enumerating.
> 
> Detect the above scenario through the process of elimination.
> 
> 1) Integrated Thunderbolt host controllers already have Thunderbolt
>    implemented, so anything outside their external facing root port is
>    removable and untrusted.
> 
>    Detect them using the following properties:
> 
>      - Most integrated host controllers have the usb4-host-interface
>        ACPI property, as described here:
> Link: https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#mapping-native-protocols-pcie-displayport-tunneled-through-usb4-to-usb4-host-routers
> 
>      - Integrated Thunderbolt PCIe root ports before Alder Lake do not
>        have the usb4-host-interface ACPI property. Identify those with
>        their PCI IDs instead.
> 
> 2) If a root port does not have integrated Thunderbolt capabilities, but
>    has the ExternalFacingPort ACPI property, that means the manufacturer
>    has opted to use a discrete Thunderbolt host controller that is
>    built into the computer.
> 
>    This host controller can be identified by virtue of being located
>    directly below an external-facing root port that lacks integrated
>    Thunderbolt. Label it as trusted and fixed.
> 
>    Everything downstream from it is untrusted and removable.
> 
> The ExternalFacingPort ACPI property is described here:
> Link: https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-externally-exposed-pcie-root-ports
> 
> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Esther Shimanovich <eshimanovich@chromium.org>
> Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Tested-by: Mario Limonciello <mario.limonciello@amd.com>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> While working with devices that have discrete Thunderbolt chips, I
> noticed that their internal TBT chips are inaccurately labeled as
> untrusted and removable.
> 
> I've observed that this issue impacts all computers with internal,
> discrete Intel JHL Thunderbolt chips, such as JHL6240, JHL6340, JHL6540,
> and JHL7540, across multiple device manufacturers such as Lenovo, Dell,
> and HP.
> 
> This affects the execution of any downstream security policy that
> relies on the "untrusted" or "removable" flags.
> 
> I initially submitted a quirk to resolve this, which was too small in
> scope, and after some discussion, Mika proposed a more thorough fix:
> https://lore.kernel.org/lkml/20240510052616.GC4162345@black.fi.intel.com
> I refactored it and am submitting as a new patch.
> ---
> Changes in v5:
> - Applied the following edits suggested by Lukas Wunner:
>   - Applied ifdefs edits to ensure code is only compiled on x86 systems
>   with ACPI
>   - Added returns to avoid unecessary checks
>   - Renamed pcie_is_tunneled to arch_pci_dev_is_removable
> - Link to v4: https://lore.kernel.org/r/20240823-trust-tbt-fix-v4-1-c6f1e3bdd9be@chromium.org
> 
> Changes in v4:
> - Applied edits on logic-flow clarity and formatting suggested by Ilpo
>   Järvinen
> - Mario Limonciello tested patch and confirmed works as intended.
> - Link to v3: https://lore.kernel.org/r/20240815-trust-tbt-fix-v3-1-6ba01865d54c@chromium.org
> 
> Changes in v3:
> - Incorporated minor edits suggested by Mika Westerberg.
> - Mika Westerberg tested patch (more details in v2 link)
> - Added "reviewed-by" and "tested-by" lines
> - Link to v2: https://lore.kernel.org/r/20240808-trust-tbt-fix-v2-1-2e34a05a9186@chromium.org
> 
> Changes in v2:
> - I clarified some comments, and made minor fixins
> - I also added a more detailed description of implementation into the
>   commit message
> - Added Cc recipients Mike recommended
> - Link to v1: https://lore.kernel.org/r/20240806-trust-tbt-fix-v1-1-73ae5f446d5a@chromium.org
> ---
>  arch/x86/pci/acpi.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/probe.c |  30 +++++++++----
>  include/linux/pci.h |   6 +++
>  3 files changed, 148 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index 55c4b07ec1f6..62271668c3b1 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -250,6 +250,125 @@ void __init pci_acpi_crs_quirks(void)
>  		pr_info("Please notify linux-pci@vger.kernel.org so future kernels can do this automatically\n");
>  }
>  
> +/*
> + * Checks if pdev is part of a PCIe switch that is directly below the
> + * specified bridge.
> + */
> +static bool pcie_switch_directly_under(struct pci_dev *bridge,
> +				       struct pci_dev *pdev)
> +{
> +	struct pci_dev *parent = pci_upstream_bridge(pdev);
> +
> +	/* If the device doesn't have a parent, it's not under anything. */
> +	if (!parent)
> +		return false;
> +
> +	/*
> +	 * If the device has a PCIe type, check if it is below the
> +	 * corresponding PCIe switch components (if applicable). Then check
> +	 * if its upstream port is directly beneath the specified bridge.
> +	 */
> +	switch (pci_pcie_type(pdev)) {
> +	case PCI_EXP_TYPE_UPSTREAM:
> +		return parent == bridge;
> +
> +	case PCI_EXP_TYPE_DOWNSTREAM:
> +		if (pci_pcie_type(parent) != PCI_EXP_TYPE_UPSTREAM)
> +			return false;
> +		parent = pci_upstream_bridge(parent);
> +		return parent == bridge;
> +
> +	case PCI_EXP_TYPE_ENDPOINT:
> +		if (pci_pcie_type(parent) != PCI_EXP_TYPE_DOWNSTREAM)
> +			return false;
> +		parent = pci_upstream_bridge(parent);
> +		if (!parent || pci_pcie_type(parent) != PCI_EXP_TYPE_UPSTREAM)
> +			return false;
> +		parent = pci_upstream_bridge(parent);
> +		return parent == bridge;
> +	}
> +
> +	return false;
> +}
> +
> +static bool pcie_has_usb4_host_interface(struct pci_dev *pdev)
> +{
> +	struct fwnode_handle *fwnode;
> +
> +	/*
> +	 * For USB4, the tunneled PCIe root or downstream ports are marked
> +	 * with the "usb4-host-interface" ACPI property, so we look for
> +	 * that first. This should cover most cases.
> +	 */
> +	fwnode = fwnode_find_reference(dev_fwnode(&pdev->dev),
> +				       "usb4-host-interface", 0);
> +	if (!IS_ERR(fwnode)) {
> +		fwnode_handle_put(fwnode);
> +		return true;
> +	}

The Intel devices below look like they're obviously x86-specific, but
what about the "usb4-host-interface" above?  Is there any reason that
should be x86-specific?

> +	/*
> +	 * Any integrated Thunderbolt 3/4 PCIe root ports from Intel
> +	 * before Alder Lake do not have the "usb4-host-interface"
> +	 * property so we use their PCI IDs instead. All these are
> +	 * tunneled. This list is not expected to grow.
> +	 */
> +	if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
> +		switch (pdev->device) {
> +		/* Ice Lake Thunderbolt 3 PCIe Root Ports */
> +		case 0x8a1d:
> +		case 0x8a1f:
> +		case 0x8a21:
> +		case 0x8a23:
> +		/* Tiger Lake-LP Thunderbolt 4 PCIe Root Ports */
> +		case 0x9a23:
> +		case 0x9a25:
> +		case 0x9a27:
> +		case 0x9a29:
> +		/* Tiger Lake-H Thunderbolt 4 PCIe Root Ports */
> +		case 0x9a2b:
> +		case 0x9a2d:
> +		case 0x9a2f:
> +		case 0x9a31:
> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}
> +
> +bool arch_pci_dev_is_removable(struct pci_dev *pdev)
> +{
> +	struct pci_dev *parent, *root;
> +
> +	/* pdev without a parent or Root Port is never tunneled. */
> +	parent = pci_upstream_bridge(pdev);
> +	if (!parent)
> +		return false;
> +	root = pcie_find_root_port(pdev);
> +	if (!root)
> +		return false;
> +
> +	/* Internal PCIe devices are not tunneled. */
> +	if (!root->external_facing)
> +		return false;
> +
> +	/* Anything directly behind a "usb4-host-interface" is tunneled. */
> +	if (pcie_has_usb4_host_interface(parent))
> +		return true;
> +
> +	/*
> +	 * Check if this is a discrete Thunderbolt/USB4 controller that is
> +	 * directly behind the non-USB4 PCIe Root Port marked as
> +	 * "ExternalFacingPort". Those are not behind a PCIe tunnel.
> +	 */
> +	if (pcie_switch_directly_under(root, pdev))
> +		return false;
> +
> +	/* PCIe devices after the discrete chip are tunneled. */
> +	return true;
> +}

I asked on the v4 patch whether we really need to make all this
ACPI specific, and I'm still curious about that, since we don't
actually use any ACPI interfaces directly.

Bjorn

On Tue, Oct 29, 2024 at 07:15:24PM -0500, Bjorn Helgaas wrote:
> I asked on the v4 patch whether we really need to make all this
> ACPI specific, and I'm still curious about that, since we don't
> actually use any ACPI interfaces directly.

The patch works around a deficiency in a Microsoft spec which is
specifically for ACPI-based systems, not devicetree-based systems:

   "ACPI Interface: Device Specific Data (_DSD) for PCIe Root Ports
    In Windows 10 (Version 1803), new ACPI _DSD methods have been added
    to support Modern Standby and PCI hot plug scenarios."

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

The deficiency is that Microsoft says the ExternalFacingPort property
must be below the Root Port...

   "This ACPI object enables the operating system to identify externally
    exposed PCIe hierarchies, such as Thunderbolt. This object must be
    implemented in the Root Port ACPI device scope."

...but on the systems in question, external-facing ports do not
originate from the Root Port, but from Downstream Ports.
So there's the Root Port (with the external facing property),
below that an Upstream Port and below that a Downstream Port
(which is the actual external facing port).

I'm not sure if Windows on ARM systems use ACPI or devicetree.
I'm also not sure whether the Qualcomm SnapDragon SoCs they use
have Thunderbolt built-in, in which case they won't need a
discrete Thunderbolt controller.  If they don't use discrete
Thunderbolt controllers or if they don't use ACPI, they can't
exhibit the problem.

In any case I haven't heard of any Windows on ARM systems being
affected by the issue.

So it boils down to:  Should we compile the quirk in just in case
ARM-based ACPI systems with discrete Thunderbolt controllers and
problematic ACPI tables show up, or should we constrain it to x86,
which is the only known architecture that actually needs it right now.

My recommendation would be the latter because it's easy to move
code around in the tree, should other arches become affected,
but in the meantime we save memory and compile time on anything
not x86.

Thanks,

Lukas

On Wed, Oct 30, 2024 at 12:11:33PM +0100, Lukas Wunner wrote:
> On Tue, Oct 29, 2024 at 07:15:24PM -0500, Bjorn Helgaas wrote:
> > I asked on the v4 patch whether we really need to make all this
> > ACPI specific, and I'm still curious about that, since we don't
> > actually use any ACPI interfaces directly.
> 
> The patch works around a deficiency in a Microsoft spec which is
> specifically for ACPI-based systems, not devicetree-based systems:
> 
>    "ACPI Interface: Device Specific Data (_DSD) for PCIe Root Ports
>     In Windows 10 (Version 1803), new ACPI _DSD methods have been added
>     to support Modern Standby and PCI hot plug scenarios."
> 
>     https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports
> 
> The deficiency is that Microsoft says the ExternalFacingPort property
> must be below the Root Port...
> 
>    "This ACPI object enables the operating system to identify externally
>     exposed PCIe hierarchies, such as Thunderbolt. This object must be
>     implemented in the Root Port ACPI device scope."
> 
> ...but on the systems in question, external-facing ports do not
> originate from the Root Port, but from Downstream Ports.
> So there's the Root Port (with the external facing property),
> below that an Upstream Port and below that a Downstream Port
> (which is the actual external facing port).
> 
> I'm not sure if Windows on ARM systems use ACPI or devicetree.
> I'm also not sure whether the Qualcomm SnapDragon SoCs they use
> have Thunderbolt built-in, in which case they won't need a
> discrete Thunderbolt controller.  If they don't use discrete
> Thunderbolt controllers or if they don't use ACPI, they can't
> exhibit the problem.
> 
> In any case I haven't heard of any Windows on ARM systems being
> affected by the issue.

Well they can do whatever they want without us knowing ;-) This problem
does not happen even in x86 Windows probably because they do something
similar than this patch.

> So it boils down to:  Should we compile the quirk in just in case
> ARM-based ACPI systems with discrete Thunderbolt controllers and
> problematic ACPI tables show up, or should we constrain it to x86,
> which is the only known architecture that actually needs it right now.
> 
> My recommendation would be the latter because it's easy to move
> code around in the tree, should other arches become affected,
> but in the meantime we save memory and compile time on anything
> not x86.

IMHO this should be made generic enough that allows device tree based
systems to take advantage of this right from the get-go. Note also there
is already "external-facing" device tree property that matches the ACPI
one defined in Documentation/devicetree/bindings/pci/pci.txt.

On Wed, Oct 30, 2024 at 01:31:08PM +0200, Mika Westerberg wrote:
> On Wed, Oct 30, 2024 at 12:11:33PM +0100, Lukas Wunner wrote:
> > In any case I haven't heard of any Windows on ARM systems being
> > affected by the issue.
> 
> Well they can do whatever they want without us knowing ;-) This problem
> does not happen even in x86 Windows probably because they do something
> similar than this patch.

I meant Linux on "Windows on ARM" machines. :)

This article claims that UEFI+ACPI is used to boot Windows,
but Qualcomm recommends devicetree is used for Linux:

https://www.heise.de/en/news/Snapdragon-X-notebooks-Ditch-Windows-Install-Linux-9781461.html


> > So it boils down to:  Should we compile the quirk in just in case
> > ARM-based ACPI systems with discrete Thunderbolt controllers and
> > problematic ACPI tables show up, or should we constrain it to x86,
> > which is the only known architecture that actually needs it right now.
> > 
> > My recommendation would be the latter because it's easy to move
> > code around in the tree, should other arches become affected,
> > but in the meantime we save memory and compile time on anything
> > not x86.
> 
> IMHO this should be made generic enough that allows device tree based
> systems to take advantage of this right from the get-go. Note also there
> is already "external-facing" device tree property that matches the ACPI
> one defined in Documentation/devicetree/bindings/pci/pci.txt.

The workaround implemented by Esther's patch (only) becomes necessary
because OEMs followed Microsoft's spec blindly and put the property
below the Root Port, instead of the Downstream Port.

Devicetree-based systems are not bound by Microsoft's spec, so do not
*have* to fall into the same trap.

Thanks,

Lukas

On Wed, Oct 30, 2024 at 02:11:31PM +0100, Lukas Wunner wrote:
> > IMHO this should be made generic enough that allows device tree based
> > systems to take advantage of this right from the get-go. Note also there
> > is already "external-facing" device tree property that matches the ACPI
> > one defined in Documentation/devicetree/bindings/pci/pci.txt.
> 
> The workaround implemented by Esther's patch (only) becomes necessary
> because OEMs followed Microsoft's spec blindly and put the property
> below the Root Port, instead of the Downstream Port.

Honestly I don't know how else that could be implemented otherwise
without changing that definition (which BTW came from Intel originally).
They did the exact correct thing. The only real problem is that Linux
interprets it in different way.

On Tue, Oct 29, 2024 at 07:15:24PM -0500, Bjorn Helgaas wrote:
> On Tue, Sep 10, 2024 at 05:57:45PM +0000, Esther Shimanovich wrote:
> > Some computers with CPUs that lack Thunderbolt features use discrete
> > Thunderbolt chips to add Thunderbolt functionality. These Thunderbolt
> > chips are located within the chassis; between the root port labeled
> > ExternalFacingPort and the USB-C port.
> > 
> > These Thunderbolt PCIe devices should be labeled as fixed and trusted,
> > as they are built into the computer. Otherwise, security policies that
> > rely on those flags may have unintended results, such as preventing
> > USB-C ports from enumerating.
> > 
> > Detect the above scenario through the process of elimination.
> > 
> > 1) Integrated Thunderbolt host controllers already have Thunderbolt
> >    implemented, so anything outside their external facing root port is
> >    removable and untrusted.
> > 
> >    Detect them using the following properties:
> > 
> >      - Most integrated host controllers have the usb4-host-interface
> >        ACPI property, as described here:
> > Link: https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#mapping-native-protocols-pcie-displayport-tunneled-through-usb4-to-usb4-host-routers
> > 
> >      - Integrated Thunderbolt PCIe root ports before Alder Lake do not
> >        have the usb4-host-interface ACPI property. Identify those with
> >        their PCI IDs instead.
> > 
> > 2) If a root port does not have integrated Thunderbolt capabilities, but
> >    has the ExternalFacingPort ACPI property, that means the manufacturer
> >    has opted to use a discrete Thunderbolt host controller that is
> >    built into the computer.
> > 
> >    This host controller can be identified by virtue of being located
> >    directly below an external-facing root port that lacks integrated
> >    Thunderbolt. Label it as trusted and fixed.
> > 
> >    Everything downstream from it is untrusted and removable.
> > 
> > The ExternalFacingPort ACPI property is described here:
> > Link: https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-externally-exposed-pcie-root-ports
> > 
> > Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Signed-off-by: Esther Shimanovich <eshimanovich@chromium.org>
> > Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Tested-by: Mario Limonciello <mario.limonciello@amd.com>
> > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> > ---
> > While working with devices that have discrete Thunderbolt chips, I
> > noticed that their internal TBT chips are inaccurately labeled as
> > untrusted and removable.
> > 
> > I've observed that this issue impacts all computers with internal,
> > discrete Intel JHL Thunderbolt chips, such as JHL6240, JHL6340, JHL6540,
> > and JHL7540, across multiple device manufacturers such as Lenovo, Dell,
> > and HP.
> > 
> > This affects the execution of any downstream security policy that
> > relies on the "untrusted" or "removable" flags.
> > 
> > I initially submitted a quirk to resolve this, which was too small in
> > scope, and after some discussion, Mika proposed a more thorough fix:
> > https://lore.kernel.org/lkml/20240510052616.GC4162345@black.fi.intel.com
> > I refactored it and am submitting as a new patch.
> > ---
> > Changes in v5:
> > - Applied the following edits suggested by Lukas Wunner:
> >   - Applied ifdefs edits to ensure code is only compiled on x86 systems
> >   with ACPI
> >   - Added returns to avoid unecessary checks
> >   - Renamed pcie_is_tunneled to arch_pci_dev_is_removable
> > - Link to v4: https://lore.kernel.org/r/20240823-trust-tbt-fix-v4-1-c6f1e3bdd9be@chromium.org
> > 
> > Changes in v4:
> > - Applied edits on logic-flow clarity and formatting suggested by Ilpo
> >   Järvinen
> > - Mario Limonciello tested patch and confirmed works as intended.
> > - Link to v3: https://lore.kernel.org/r/20240815-trust-tbt-fix-v3-1-6ba01865d54c@chromium.org
> > 
> > Changes in v3:
> > - Incorporated minor edits suggested by Mika Westerberg.
> > - Mika Westerberg tested patch (more details in v2 link)
> > - Added "reviewed-by" and "tested-by" lines
> > - Link to v2: https://lore.kernel.org/r/20240808-trust-tbt-fix-v2-1-2e34a05a9186@chromium.org
> > 
> > Changes in v2:
> > - I clarified some comments, and made minor fixins
> > - I also added a more detailed description of implementation into the
> >   commit message
> > - Added Cc recipients Mike recommended
> > - Link to v1: https://lore.kernel.org/r/20240806-trust-tbt-fix-v1-1-73ae5f446d5a@chromium.org
> > ---
> >  arch/x86/pci/acpi.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/pci/probe.c |  30 +++++++++----
> >  include/linux/pci.h |   6 +++
> >  3 files changed, 148 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> > index 55c4b07ec1f6..62271668c3b1 100644
> > --- a/arch/x86/pci/acpi.c
> > +++ b/arch/x86/pci/acpi.c
> > @@ -250,6 +250,125 @@ void __init pci_acpi_crs_quirks(void)
> >  		pr_info("Please notify linux-pci@vger.kernel.org so future kernels can do this automatically\n");
> >  }
> >  
> > +/*
> > + * Checks if pdev is part of a PCIe switch that is directly below the
> > + * specified bridge.
> > + */
> > +static bool pcie_switch_directly_under(struct pci_dev *bridge,
> > +				       struct pci_dev *pdev)
> > +{
> > +	struct pci_dev *parent = pci_upstream_bridge(pdev);
> > +
> > +	/* If the device doesn't have a parent, it's not under anything. */
> > +	if (!parent)
> > +		return false;
> > +
> > +	/*
> > +	 * If the device has a PCIe type, check if it is below the
> > +	 * corresponding PCIe switch components (if applicable). Then check
> > +	 * if its upstream port is directly beneath the specified bridge.
> > +	 */
> > +	switch (pci_pcie_type(pdev)) {
> > +	case PCI_EXP_TYPE_UPSTREAM:
> > +		return parent == bridge;
> > +
> > +	case PCI_EXP_TYPE_DOWNSTREAM:
> > +		if (pci_pcie_type(parent) != PCI_EXP_TYPE_UPSTREAM)
> > +			return false;
> > +		parent = pci_upstream_bridge(parent);
> > +		return parent == bridge;
> > +
> > +	case PCI_EXP_TYPE_ENDPOINT:
> > +		if (pci_pcie_type(parent) != PCI_EXP_TYPE_DOWNSTREAM)
> > +			return false;
> > +		parent = pci_upstream_bridge(parent);
> > +		if (!parent || pci_pcie_type(parent) != PCI_EXP_TYPE_UPSTREAM)
> > +			return false;
> > +		parent = pci_upstream_bridge(parent);
> > +		return parent == bridge;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +static bool pcie_has_usb4_host_interface(struct pci_dev *pdev)
> > +{
> > +	struct fwnode_handle *fwnode;
> > +
> > +	/*
> > +	 * For USB4, the tunneled PCIe root or downstream ports are marked
> > +	 * with the "usb4-host-interface" ACPI property, so we look for
> > +	 * that first. This should cover most cases.
> > +	 */
> > +	fwnode = fwnode_find_reference(dev_fwnode(&pdev->dev),
> > +				       "usb4-host-interface", 0);
> > +	if (!IS_ERR(fwnode)) {
> > +		fwnode_handle_put(fwnode);
> > +		return true;
> > +	}
> 
> The Intel devices below look like they're obviously x86-specific, but
> what about the "usb4-host-interface" above?  Is there any reason that
> should be x86-specific?

It is generic to any USB4 host, not just x86 although currently we know
of only Intel and AMD using it.

> > +	/*
> > +	 * Any integrated Thunderbolt 3/4 PCIe root ports from Intel
> > +	 * before Alder Lake do not have the "usb4-host-interface"
> > +	 * property so we use their PCI IDs instead. All these are
> > +	 * tunneled. This list is not expected to grow.
> > +	 */
> > +	if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
> > +		switch (pdev->device) {
> > +		/* Ice Lake Thunderbolt 3 PCIe Root Ports */
> > +		case 0x8a1d:
> > +		case 0x8a1f:
> > +		case 0x8a21:
> > +		case 0x8a23:
> > +		/* Tiger Lake-LP Thunderbolt 4 PCIe Root Ports */
> > +		case 0x9a23:
> > +		case 0x9a25:
> > +		case 0x9a27:
> > +		case 0x9a29:
> > +		/* Tiger Lake-H Thunderbolt 4 PCIe Root Ports */
> > +		case 0x9a2b:
> > +		case 0x9a2d:
> > +		case 0x9a2f:
> > +		case 0x9a31:
> > +			return true;
> > +		}
> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +bool arch_pci_dev_is_removable(struct pci_dev *pdev)
> > +{
> > +	struct pci_dev *parent, *root;
> > +
> > +	/* pdev without a parent or Root Port is never tunneled. */
> > +	parent = pci_upstream_bridge(pdev);
> > +	if (!parent)
> > +		return false;
> > +	root = pcie_find_root_port(pdev);
> > +	if (!root)
> > +		return false;
> > +
> > +	/* Internal PCIe devices are not tunneled. */
> > +	if (!root->external_facing)
> > +		return false;
> > +
> > +	/* Anything directly behind a "usb4-host-interface" is tunneled. */
> > +	if (pcie_has_usb4_host_interface(parent))
> > +		return true;
> > +
> > +	/*
> > +	 * Check if this is a discrete Thunderbolt/USB4 controller that is
> > +	 * directly behind the non-USB4 PCIe Root Port marked as
> > +	 * "ExternalFacingPort". Those are not behind a PCIe tunnel.
> > +	 */
> > +	if (pcie_switch_directly_under(root, pdev))
> > +		return false;
> > +
> > +	/* PCIe devices after the discrete chip are tunneled. */
> > +	return true;
> > +}
> 
> I asked on the v4 patch whether we really need to make all this
> ACPI specific, and I'm still curious about that, since we don't
> actually use any ACPI interfaces directly.

We definitely don't want this to be ACPI specific, there can be ARM
based systems with USB4 host controller too and those can also use
DeviceTree instead of ACPI.