Message-ID: <CA+Y6NJEg-1uGCS0eJ2QP4p6EEh2S+6-yTAUKpPvvqDpyb6_DMQ@mail.gmail.com>
Date: Fri, 26 Jul 2024 14:17:46 -0400
From: Esther Shimanovich <eshimanovich@...omium.org>
To: Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc: Lukas Wunner <lukas@...ner.de>, Mario Limonciello <mario.limonciello@....com>, 
	Dmitry Torokhov <dmitry.torokhov@...il.com>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Rajat Jain <rajatja@...gle.com>
Subject: Re: [PATCH v4] PCI: Relabel JHL6540 on Lenovo X1 Carbon 7,8

On Wed, Jun 26, 2024 at 4:05 AM Mika Westerberg
<mika.westerberg@...ux.intel.com> wrote:
> I will be on vacation starting next week for the whole July. The patch
> is kind of "pseudo-code" in that sense that it probably needs some
> additional work, cleanup, maybe drop the serial number checks and so on.
> You are free to use it as you see fit, or submit upstream as proper
> patch if nobody objects.

I cleaned it up, but I think I'd like to run it by you before
submitting it, as you are the author and also some of my cleanups
ended up being a bit more involved than I anticipated.

For cleanup, I did the following:

1) I ended up moving the changes from pcie_set_pcie_untrusted to
pci_set_removable for multiple reasons:

- The downstream bug I ran into happened because of the "removable" attribute.

- There seems to be a reason why both removable and untrusted exist
despite both having the same logic. pci_fixup_early is run after
pcie_set_pcie_untrusted, but before pci_set_removable. It seems like
this was done on purpose so that downstream security policies can use
quirks to set specific internal, fixed devices as untrusted.

- The way you wrote it makes the attributes removable = untrusted,
which wasn't the case before, and undos the pci_fixup_early quirks
logic.

- If you want to make sure that these non-tunneled discrete
thunderbolt chips are labeled as trusted, we may have to duplicate
this logic in both functions (which seems to be already the case
anyways in their current state).
I just don't fully know what the "untrusted" attribute entails, so I
am erring on the more conservative side of only making changes I fully
understand.

2) I changed this comment into code:

> +/* root->external_facing is true, parent != NULL */

3) I edited legacy comments to reflect what the code does now. I also
changed your comments to reflect how I changed the code, but for the
most part I kept your words in as they were really clear.

4) I removed the serial checks as you suggested

> If nothing has happened when I come back, I can pick up the work if I
> still remember this ;-)

I did my best to clean up! I'm unsure if you will want me to duplicate
this logic to pcie_set_pcie_untrusted, so just let me know if I should
fix that, and I'll send it to the kernel! (I'm assuming with the
Co-developed-by, and the Signed-off-by lines, to properly attribute
you?)

I hope you had a nice vacation! Both you and Lukas Wurner have been so
helpful and attentive.

The cleaned up patch is below:


diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 43159965e09e9..fc3ef2cf66d58 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1613,24 +1613,161 @@ static void set_pcie_untrusted(struct pci_dev *dev)
                dev->untrusted = true;
 }

+/*
+ * Checks if the PCIe switch that contains pdev is directly under
+ * the specified bridge.
+ */
+static bool pcie_switch_directly_under(struct pci_dev *bridge,
+                                      struct pci_dev *parent,
+                                      struct pci_dev *pdev)
+{
+       /*
+        * 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 *root, struct pci_dev *parent,
+                            struct pci_dev *pdev)
+{
+       /* Return least trusted outcome if params are invalid */
+       if (!(root && root->external_facing && parent))
+               return true;
+
+       /* 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 a PCIe Root Port marked as "ExternalFacingPort".
+        * These are not behind a PCIe tunnel.
+        */
+       if (pcie_switch_directly_under(root, parent, pdev))
+               return false;
+
+       return true;
+}
+
 static void pci_set_removable(struct pci_dev *dev)
 {
-       struct pci_dev *parent = pci_upstream_bridge(dev);
+       struct pci_dev *parent, *root;
+
+       parent = pci_upstream_bridge(dev);

        /*
-        * We (only) consider everything downstream from 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
-        * the firmware as external_facing. Devices in traditional hotplug
-        * slots can technically be removed, but the expectation is that unless
-        * the port is marked with external_facing, such devices are less
-        * accessible to user / may not be removed by end user, and thus not
-        * exposed as "removable" to userspace.
+        * We're mainly concerned with consumer platforms with user accessible
+        * thunderbolt ports that are vulnerable to DMA attacks.
+        * We expect those ports to be marked by the firmware as
external_facing.
+        * Devices outside external_facing ports are labeled as removable, with
+        * the exception of discrete thunderbolt chips within the chassis.
+        *
+        * Devices in traditional hotplug slots can technically be removed,
+        * but the expectation is that unless the port is marked with
+        * external_facing, such devices are less 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 (!parent)
+               return;
+
+       if (dev_is_removable(&parent->dev))
                dev_set_removable(&dev->dev, DEVICE_REMOVABLE);
+
+       root = pcie_find_root_port(dev);
+
+       if (root && root->external_facing) {
+               /*
+                * All devices behind a PCIe root port labeled as
+                * "ExternalFacingPort" are tunneled by definition,
+                * with the exception of discrete Thunderbolt/USB4
+                * controllers that add Thunderbolt capabilities
+                * to CPUs that lack integrated Thunderbolt.
+                * They are identified because by definition, they
+                * aren't tunneled.
+                *
+                * Those discrete Thunderbolt/USB4 controllers are
+                * not removable. Only their downstream facing ports
+                * are actually something that are exposed to the
+                * wild so we only mark devices tunneled behind those
+                * as removable.
+                */
+               if (pcie_is_tunneled(root, parent, dev))
+                       dev_set_removable(&dev->dev, DEVICE_REMOVABLE);
+       }
 }

 /**

Powered by blists - more mailing lists