| Message ID | 20240806-trust-tbt-fix-v1-1-73ae5f446d5a@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Detect and trust built-in TBT chips | expand |
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@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; 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? 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? > + */ > + 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@chromium.org> >
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@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; > > 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@chromium.org> > >
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>
On Wed, Aug 7, 2024 at 3:17 AM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > I suggest opening the heuristic here a bit more. Could you clarify what you mean by "opening the heuristic"? What details should I remove? 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 Is it good enough to have them in the commit message? Or should they be linked in probe.c?
On Tue, Aug 6, 2024 at 6:04 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > 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? > This seems to be a case of neglecting to add another ACPI definition that distinguishes internal Thunderbolt chips from user-removable ones. ExternalFacingPort is not 100% accurate all of the time, as we just discovered, but it does clearly describe what root port we are talking about. > > 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? Can you point me to this resource? Thanks for your suggestions and requests for clarification! I'll incorporate them into the next patch.
On Wed, Aug 07, 2024 at 05:15:36PM -0400, Esther Shimanovich wrote: > On Wed, Aug 7, 2024 at 3:17 AM Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > > > I suggest opening the heuristic here a bit more. > > Could you clarify what you mean by "opening the heuristic"? > What details should I remove? I mean not to remove anything but open up how we "detect" the built-in controlllers. > 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 > > Is it good enough to have them in the commit message? Or should they > be linked in probe.c? IMHO they can be in the commit message.
On Tue, Aug 06, 2024 at 05:04:06PM -0500, Bjorn Helgaas wrote: > > +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? In case of integrated host controller the above function returns true for anything connected behind its tunneled PCIe root ports so if you plug in a discrete controller into that it will be treated as tunneled and gets full IOMMU mappings. In case of discrete host controller there are two ways. With USB4 the above function finds "usb4-host-interface" firmware description and returns true the discrete controller you plug in will be treated as tunneled and gets full IOMMU mappings. With non-USB4 (that's Thunderbolt 3 and below) discrete host controller we have root->external_facing true (it is marked with "ExternalFacingPort" firmware property) the check below fails because the discrete controller you plug in is not directly under that so it will be treated as tunneled and gets full IOMMU mappings. > 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; > > +}
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? We're the victim of an ambiguity in Microsoft's spec here: The spec says that ExternalFacingPort is used to identify externally exposed PCIe hierarchies. But the spec mandates that the property shall only exist under the Root Port ACPI device scope. OEMs follow that spec to a T by specifying ExternalFacingPort below the Root Port, even though the Root Port itself is not external facing but connects to a discrete Thunderbolt controller which is soldered down to the mainboard. In reality the external facing ports extend from the discrete controller, but Microsoft's spec doesn't allow marking them as such. Here's the relevant spec language: "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. Note: On systems shipping with Windows 10, version 1803, this object should only be implemented on PCIe Root Ports of Thunderbolt hierarchies." https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-externally-exposed-pcie-root-ports It's probably safe to assume that Microsoft intended the ExternalFacingPort property to only be used on SoC-integrated Thunderbolt controllers, in which case a Root Port is indeed external facing. But that didn't stop OEMs from also specifying the property on Root Ports above soldered-down discrete Thunderbolt controllers. The spec doesn't explicitly forbid that. Thanks, Lukas
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); }
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. 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(-) --- base-commit: 3f386cb8ee9f04ff4be164ca7a1d0ef3f81f7374 change-id: 20240806-trust-tbt-fix-5f337fd9ec8a Best regards,