Message-ID: <20240729081616.GH1532424@black.fi.intel.com>
Date: Mon, 29 Jul 2024 11:16:16 +0300
From: Mika Westerberg <mika.westerberg@...ux.intel.com>
To: Esther Shimanovich <eshimanovich@...omium.org>
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

Hi,

On Fri, Jul 26, 2024 at 02:17:46PM -0400, Esther Shimanovich wrote:
> 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.

The "untrusted" means the device is something that is not "soldered
down" or equivalent. E.g something you can hot-plug through a port on the
laptop system such as USB4/TB. It is used to enable full IOMMU mappings
for these devices to avoid malicious devices from accessing memory that
does not belong to it.

So it is pretty much same as "removable".

Why we do want to have the USB4 host controller + xHCI not "untrusted"
is because they don't need to go through the full IOMMU mappings and
therefore we get for one more throughput for things like networking over
USB4.

> 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 think they should be the "same".

You can add something like "Suggested-by" or so if you like but up to
you. No need to add other tags from me.

Please Cc IOMMU folks too so that they can take a look just in case.

The patch is line-wrapped but otherwise looks good except the above.
Thanks for cleaning it up.

> 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