Re: [PATCH v3] PCI: Detect and trust built-in Thunderbolt chips

Mario Limonciello
2024-08-22T15:29:51Z

On 8/15/2024 14:07, 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>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.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.

My apologies on my delayed response, I've been OOO a while.

I had a test with this patch on an AMD Phoenix system on top of
6.11-rc4. I am noticing that with it, it's now flagging an internal PCI
host bridge as untrusted. I added some extra debugging and it falls
through to the last case of pcie_is_tunneled() where it returns true.

Here is the topology of the system:

-[0000:00]-+-00.0
+-00.2
+-01.0
+-01.3-[01]----00.0
+-02.0
+-02.1-[02]----00.0
+-02.4-[03]----00.0
+-03.0
+-03.1-[04-62]--
+-04.0
+-04.1-[63-c1]--
+-08.0
+-08.1-[c2]--+-00.0
| +-00.1
| +-00.2
| +-00.3
| +-00.4
| +-00.5
| +-00.6
| \-00.7
+-08.2-[c3]--+-00.0
| \-00.1
+-08.3-[c4]--+-00.0
| +-00.3
| +-00.4
| +-00.5
| \-00.6
+-14.0
+-14.3
+-18.0
+-18.1
+-18.2
+-18.3
+-18.4
+-18.5
+-18.6
\-18.7

Here are the details of all devices on the system:

00:00.0 Host bridge [0600]: Advanced Micro Devices, Inc. [AMD] Device
[1022:14e8]
00:00.2 IOMMU [0806]: Advanced Micro Devices, Inc. [AMD] Device [1022:14e9]
00:01.0 Host bridge [0600]: Advanced Micro Devices, Inc. [AMD] Device
[1022:14ea]
00:01.3 PCI bridge [0604]: Advanced Micro Devices, Inc. [AMD] Device
[1022:14ee]
00:02.0 Host bridge [0600]: Advanced Micro Devices, Inc. [AMD] Device
[1022:14ea]
00:02.1 PCI bridge [0604]: Advanced Micro Devices, Inc. [AMD] Device
[1022:14ee]
00:02.4 PCI bridge [0604]: Advanced Micro Devices, Inc. [AMD] Device
[1022:14ee]
00:03.0 Host bridge [0600]: Advanced Micro Devices, Inc. [AMD] Device
[1022:14ea]
00:03.1 PCI bridge [0604]: Advanced Micro Devices, Inc. [AMD] Family 19h
USB4/Thunderbolt PCIe tunnel [1022:14ef]
00:04.0 Host bridge [0600]: Advanced Micro Devices, Inc. [AMD] Device
[1022:14ea]
00:04.1 PCI bridge [0604]: Advanced Micro Devices, Inc. [AMD] Family 19h
USB4/Thunderbolt PCIe tunnel [1022:14ef]
00:08.0 Host bridge [0600]: Advanced Micro Devices, Inc. [AMD] Device
[1022:14ea]
00:08.1 PCI bridge [0604]: Advanced Micro Devices, Inc. [AMD] Device
[1022:14eb]
00:08.2 PCI bridge [0604]: Advanced Micro Devices, Inc. [AMD] Device
[1022:14eb]
00:08.3 PCI bridge [0604]: Advanced Micro Devices, Inc. [AMD] Device
[1022:14eb]
00:14.0 SMBus [0c05]: Advanced Micro Devices, Inc. [AMD] FCH SMBus
Controller [1022:790b] (rev 71)
00:14.3 ISA bridge [0601]: Advanced Micro Devices, Inc. [AMD] FCH LPC
Bridge [1022:790e] (rev 51)
00:18.0 Host bridge [0600]: Advanced Micro Devices, Inc. [AMD] Device
[1022:14f0]
00:18.1 Host bridge [0600]: Advanced Micro Devices, Inc. [AMD] Device
[1022:14f1]
00:18.2 Host bridge [0600]: Advanced Micro Devices, Inc. [AMD] Device
[1022:14f2]
00:18.3 Host bridge [0600]: Advanced Micro Devices, Inc. [AMD] Device
[1022:14f3]
00:18.4 Host bridge [0600]: Advanced Micro Devices, Inc. [AMD] Device
[1022:14f4]
00:18.5 Host bridge [0600]: Advanced Micro Devices, Inc. [AMD] Device
[1022:14f5]
00:18.6 Host bridge [0600]: Advanced Micro Devices, Inc. [AMD] Device
[1022:14f6]
00:18.7 Host bridge [0600]: Advanced Micro Devices, Inc. [AMD] Device
[1022:14f7]
01:00.0 Ethernet controller [0200]: Realtek Semiconductor Co., Ltd.
RTL8111/8168/8211/8411 PCI Express Gigabit Ethernet Controller
[10ec:8168] (rev 1c)
02:00.0 Unassigned class [ff00]: Realtek Semiconductor Co., Ltd. RTS5261
PCI Express Card Reader [10ec:5261] (rev 01)
03:00.0 Non-Volatile memory controller [0108]: Samsung Electronics Co
Ltd NVMe SSD Controller PM9A1/PM9A3/980PRO [144d:a80a]
c2:00.0 VGA compatible controller [0300]: Advanced Micro Devices, Inc.
[AMD/ATI] Phoenix1 [1002:15bf] (rev 03)
c2:00.1 Audio device [0403]: Advanced Micro Devices, Inc. [AMD/ATI]
Rembrandt Radeon High Definition Audio Controller [1002:1640]
c2:00.2 Encryption controller [1080]: Advanced Micro Devices, Inc. [AMD]
Family 19h (Model 74h) CCP/PSP 3.0 Device [1022:15c7]
c2:00.3 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] Device
[1022:15b9]
c2:00.4 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] Device
[1022:15ba]
c2:00.5 Multimedia controller [0480]: Advanced Micro Devices, Inc. [AMD]
ACP/ACP3X/ACP6x Audio Coprocessor [1022:15e2] (rev 63)
c2:00.6 Audio device [0403]: Advanced Micro Devices, Inc. [AMD] Family
17h/19h HD Audio Controller [1022:15e3]
c2:00.7 Signal processing controller [1180]: Advanced Micro Devices,
Inc. [AMD] Device [1022:164a]
c3:00.0 Non-Essential Instrumentation [1300]: Advanced Micro Devices,
Inc. [AMD] Device [1022:14ec]
c3:00.1 Signal processing controller [1180]: Advanced Micro Devices,
Inc. [AMD] AMD IPU Device [1022:1502]
c4:00.0 Non-Essential Instrumentation [1300]: Advanced Micro Devices,
Inc. [AMD] Device [1022:14ec]
c4:00.3 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] Device
[1022:15c0]
c4:00.4 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] Device
[1022:15c1]
c4:00.5 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] Pink
Sardine USB4/Thunderbolt NHI controller #1 [1022:1668]
c4:00.6 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] Pink
Sardine USB4/Thunderbolt NHI controller #2 [1022:1669]

Here's the snippet from the kernel log with the patch in place. You can
see it flagged 00:02.0 as untrusted and removable, but it definitely isn't.

Aug 22 10:11:10 test-TBI1100B kernel: pci_bus 0000:02: scanning bus
Aug 22 10:11:10 test-TBI1100B kernel: pci 0000:02:00.0: marking as untrusted
Aug 22 10:11:10 test-TBI1100B kernel: pci 0000:02:00.0: marking as removable
Aug 22 10:11:10 test-TBI1100B kernel: pci 0000:02:00.0: [10ec:5261] type
00 class 0xff0000 PCIe Endpoint
Aug 22 10:11:10 test-TBI1100B kernel: pci 0000:02:00.0: BAR 0 [mem
0xb0c00000-0xb0c00fff]
Aug 22 10:11:10 test-TBI1100B kernel: pci 0000:02:00.0: supports D1 D2
Aug 22 10:11:10 test-TBI1100B kernel: pci 0000:02:00.0: PME# supported
from D1 D2 D3hot D3cold
Aug 22 10:11:10 test-TBI1100B kernel: pci_bus 0000:02: fixups for bus
Aug 22 10:11:10 test-TBI1100B kernel: pci 0000:00:02.1: PCI bridge to
[bus 02]
Aug 22 10:11:10 test-TBI1100B kernel: pci_bus 0000:02: bus scan
returning with max=02

I wonder if you want another case for pcie_switch_directly_under() of
PCI_EXP_TYPE_PCIE_BRIDGE to help this perhaps?

> ---
> 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
> ---
> drivers/pci/probe.c | 153 +++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 146 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index b14b9876c030..308d5a0e5c51 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1629,25 +1629,160 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
> dev->is_thunderbolt = 1;
> }
>
> +/*
> + * 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:
> + if (parent == bridge)
> + return true;
> + break;
> +
> + case PCI_EXP_TYPE_DOWNSTREAM:
> + if (pci_pcie_type(parent) == PCI_EXP_TYPE_UPSTREAM) {
> + parent = pci_upstream_bridge(parent);
> + if (parent == bridge)
> + return true;
> + }
> + break;
> +
> + case PCI_EXP_TYPE_ENDPOINT:
> + if (pci_pcie_type(parent) == PCI_EXP_TYPE_DOWNSTREAM) {
> + parent = pci_upstream_bridge(parent);
> + if (parent && pci_pcie_type(parent) == PCI_EXP_TYPE_UPSTREAM) {
> + parent = pci_upstream_bridge(parent);
> + if (parent == bridge)
> + return true;
> + }
> + }
> + break;
> + }
> +
> + 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;
> +}
> +
> +static bool pcie_is_tunneled(struct pci_dev *pdev)
> +{
> + struct pci_dev *parent, *root;
> +
> + parent = pci_upstream_bridge(pdev);
> + /* If pdev doesn't have a parent, then there's no way it is tunneled.*/
> + if (!parent)
> + return false;
> +
> + root = pcie_find_root_port(pdev);
> + /* If pdev doesn't have a root, then there's no way it is tunneled.*/
> + 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;
> +}
> +
> 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;
> +
> + if (pcie_is_tunneled(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 +1792,13 @@ 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);
> +
> + if (pcie_is_tunneled(dev)) {
> + pci_dbg(dev, "marking as removable\n");
> dev_set_removable(&dev->dev, DEVICE_REMOVABLE);
> + }
> }
>
> /**
>
> ---
> base-commit: 3f386cb8ee9f04ff4be164ca7a1d0ef3f81f7374
> change-id: 20240806-trust-tbt-fix-5f337fd9ec8a
>
> Best regards,