[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240807065433.GE1532424@black.fi.intel.com>
Date: Wed, 7 Aug 2024 09:54:33 +0300
From: Mika Westerberg <mika.westerberg@...ux.intel.com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Esther Shimanovich <eshimanovich@...omium.org>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Rajat Jain <rajatja@...gle.com>,
"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
Lukas Wunner <lukas@...ner.de>, linux-pci@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] PCI: Detect and trust built-in TBT chips
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@...ux.intel.com>
> > Signed-off-by: Esther Shimanovich <eshimanovich@...omium.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@...omium.org>
> >
Powered by blists - more mailing lists