On Wed, Jun 26, 2024 at 4:05 AM Mika Westerberg
<mika.westerberg@xxxxxxxxxxxxxxx> 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);
+ }
}
/**
