| Message ID | 20240815-trust-tbt-fix-v3-1-6ba01865d54c@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v3] PCI: Detect and trust built-in Thunderbolt chips | expand |
On Thu, 15 Aug 2024, 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. > --- > 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.*/ Missing space. > + 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; return parent == bridge; ? > + break; > + > + case PCI_EXP_TYPE_DOWNSTREAM: > + if (pci_pcie_type(parent) == PCI_EXP_TYPE_UPSTREAM) { It would be slightly easier to follow the logic if code uses reverse logic and returns false immediately when the decision is known. (Will also help the indentation levels especially in the last case block). > + 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.*/ Missing space. > + 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.*/ Missing space. Though instead of near identical repeat, I'd combine those two comments into something along the lines of: /* pdev without a parent or Root Port is never tunneled. */
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,
Hi, On Thu, Aug 22, 2024 at 10:29:55AM -0500, Mario Limonciello wrote: > 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. Is it marked as ExternalFacingPort in the ACPI tables?
On 8/22/2024 10:53, Mika Westerberg wrote: > Hi, > > On Thu, Aug 22, 2024 at 10:29:55AM -0500, Mario Limonciello wrote: >> 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. > > Is it marked as ExternalFacingPort in the ACPI tables? No; it doesn't have an ACPI companion device.
Hi, On Thu, Aug 22, 2024 at 10:56:44AM -0500, Mario Limonciello wrote: > > > 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. > > > > Is it marked as ExternalFacingPort in the ACPI tables? > > No; it doesn't have an ACPI companion device. Hm, how can it pass this then? static bool pcie_is_tunneled(struct pci_dev *pdev) { ... /* Internal PCIe devices are not tunneled. */ if (!root->external_facing) return false; ... Would you mind adding some debug statements there so we can see (hopefully) what goes wrong? The intention is that pcie_switch_directly_under() is only called on Intel pre-USB4 discrete controllers (well and Maple Ridge as that's still using the Connection Manager firmware).
On 8/23/2024 00:01, Mika Westerberg wrote: > Hi, > > On Thu, Aug 22, 2024 at 10:56:44AM -0500, Mario Limonciello wrote: >>>> 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. >>> >>> Is it marked as ExternalFacingPort in the ACPI tables? >> >> No; it doesn't have an ACPI companion device. > > Hm, how can it pass this then? > > static bool pcie_is_tunneled(struct pci_dev *pdev) > { > ... > /* Internal PCIe devices are not tunneled. */ > if (!root->external_facing) > return false; > ... > > Would you mind adding some debug statements there so we can see > (hopefully) what goes wrong? > > The intention is that pcie_switch_directly_under() is only called on > Intel pre-USB4 discrete controllers (well and Maple Ridge as that's > still using the Connection Manager firmware). Ah, I made a mistake on my system and was mixing up 00:02.0 and 02:00.0! Everything is working as intended, thanks. Reviewed-by: Mario Limonciello <mario.limonciello@amd.com> Tested-by: Mario Limonciello <mario.limonciello@amd.com>
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); + } } /**
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. --- 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(-) --- base-commit: 3f386cb8ee9f04ff4be164ca7a1d0ef3f81f7374 change-id: 20240806-trust-tbt-fix-5f337fd9ec8a Best regards,