On Tue, Aug 06, 2024 at 05:04:06PM -0500, Bjorn Helgaas wrote: > 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. > > So is this fundamentally a firmware defect? ACPI says a Root Port is > an "ExternalFacingPort", but the Root Port is actually connected to an > internal Thunderbolt chip, not an external connector? > > > 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. > > > > Suggested-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx> > > Signed-off-by: Esther Shimanovich <eshimanovich@xxxxxxxxxxxx> > > --- > > 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@xxxxxxxxxxxxxxxxxx/#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; > > Add blank line here. > > > + /* > > + * 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) { > > This case is not part of a PCIe switch, so the comment above isn't > quite right. > > > + 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. > > What is the source of this property? ACPI? DT? Is there some spec > we can cite that defines it? They are all here: https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports > s/cover the most/cover most/ > > > + 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. > > Is the "usb4-host-interface" property built into the hardware somehow? > Or is this a statement about the firmware we expect to see with the > parts listed below? It is with all USB4 except below (which did not have that) which is why we hard-code the PCI IDS here. > > + */ > > + 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. > > I need more context to be convinced that this is a reliable heuristic. > What keeps somebody from plugging a discrete Thunderbolt/USB4 > controller into an external port? Maybe this just needs a sentence or > two from Lukas's (?) helpful intro to tunneling? > > > + 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@xxxxxxxxxxxx> > >
