* Re: [PATCH] PCI: Detect and trust built-in TBT chips
[not found] <20240806-trust-tbt-fix-v1-1-73ae5f446d5a@chromium.org>
@ 2024-08-07 7:17 ` Mika Westerberg
2024-08-07 21:15 ` Esther Shimanovich
0 siblings, 1 reply; 3+ messages in thread
From: Mika Westerberg @ 2024-08-07 7:17 UTC (permalink / raw)
To: Esther Shimanovich
Cc: Bjorn Helgaas, Rajat Jain, Rafael J. Wysocki, Lukas Wunner,
linux-pci, linux-kernel, iommu, Mario Limonciello
Hi Esther,
I think it is good to include IOMMU folks too, iommu@lists.linux.dev. I
Cc'd them now but for the next iteration please add them. Also added
Mario from AMD as this affects them too.
In the $subject, use "Thunderbolt" instead of TBT.
On Tue, Aug 06, 2024 at 09:39:11PM +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.
I suggest opening the heuristic here a bit more. And add the links to
the Microsoft firmware document somewhere. These:
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
https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-externally-exposed-pcie-root-ports
Other than this the patch looks good to me.
> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Esther Shimanovich <eshimanovich@chromium.org>
> ---
> 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/#r
> I refactored it and am submitting as a new patch.
> ---
> drivers/pci/probe.c | 149 +++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 142 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index b14b9876c030..30de2f6da164 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1629,16 +1629,147 @@ 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, that means it is part of a PCIe
> + * switch.
> + */
> + 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" property, so we look for that
> + * first. This should cover the 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 above device 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". These PCIe devices are used to add Thunderbolt
> + * capabilities to CPUs that lack integrated Thunderbolt.
> + * These 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))
> dev->untrusted = true;
> }
>
> @@ -1646,8 +1777,10 @@ 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,8 +1790,10 @@ 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))
> dev_set_removable(&dev->dev, DEVICE_REMOVABLE);
> }
>
>
> ---
> base-commit: 3f386cb8ee9f04ff4be164ca7a1d0ef3f81f7374
> change-id: 20240806-trust-tbt-fix-5f337fd9ec8a
>
> Best regards,
> --
> Esther Shimanovich <eshimanovich@chromium.org>
^ permalink raw reply [flat|nested] 3+ messages in thread