[v4] PCI: Detect and trust built-in Thunderbolt chips

Commit Message

Esther Shimanovich Aug. 23, 2024, 4:53 p.m. UTC

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.

Detect the above scenario through the process of elimination.

1) Integrated Thunderbolt host controllers already have Thunderbolt
   implemented, so anything outside their external facing root port is
   removable and untrusted.

   Detect them using the following properties:

     - Most integrated host controllers have the usb4-host-interface
       ACPI property, as described here:
Link: 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

     - Integrated Thunderbolt PCIe root ports before Alder Lake do not
       have the usb4-host-interface ACPI property. Identify those with
       their PCI IDs instead.

2) If a root port does not have integrated Thunderbolt capabilities, but
   has the ExternalFacingPort ACPI property, that means the manufacturer
   has opted to use a discrete Thunderbolt host controller that is
   built into the computer.

   This host controller can be identified by virtue of being located
   directly below an external-facing root port that lacks integrated
   Thunderbolt. Label it as trusted and fixed.

   Everything downstream from it is untrusted and removable.

The ExternalFacingPort ACPI property is described here:
Link: https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-externally-exposed-pcie-root-ports

Suggested-by: Mika Westerberg <[e-mail]>
Signed-off-by: Esther Shimanovich <[e-mail]>
Tested-by: Mika Westerberg <[e-mail]>
Tested-by: Mario Limonciello <[e-mail]>
Reviewed-by: Mika Westerberg <[e-mail]>
Reviewed-by: Mario Limonciello <[e-mail]>
---
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/[e-mail]
I refactored it and am submitting as a new patch.
---
Changes in v4:
- Applied edits on logic-flow clarity and formatting suggested by Ilpo
  Järvinen
- Mario Limonciello tested patch and confirmed works as intended.
- Link to v3: https://lore.kernel.org/r/[e-mail]

Changes in v3:
- Incorporated minor edits suggested by Mika Westerberg.
- Mika Westerberg tested patch (more details in v2 link)
- Added "reviewed-by" and "tested-by" lines
- Link to v2: https://lore.kernel.org/r/[e-mail]

Changes in v2:
- I clarified some comments, and made minor fixins
- I also added a more detailed description of implementation into the
  commit message
- Added Cc recipients Mike recommended
- Link to v1: https://lore.kernel.org/r/[e-mail]
---
 drivers/pci/probe.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 138 insertions(+), 7 deletions(-)


---
base-commit: 3f386cb8ee9f04ff4be164ca7a1d0ef3f81f7374
change-id: 20240806-trust-tbt-fix-5f337fd9ec8a

Best regards,

Comments

Bjorn Helgaas Aug. 23, 2024, 9:12 p.m. UTC | #1

On Fri, Aug 23, 2024 at 04:53:16PM +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.

Is this a firmware defect?  I asked this before, and I interpret your
answer of "ExternalFacingPort is not 100% accurate all of the time" as
"yes, this is a firmware defect."  That should be part of the commit
log and code comments.

We (of course) have to work around firmware defects, but workarounds
need to be labeled as such instead of masquerading as generic code.

> 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.
> 
> Detect the above scenario through the process of elimination.
> 
> 1) Integrated Thunderbolt host controllers already have Thunderbolt
>    implemented, so anything outside their external facing root port is
>    removable and untrusted.
> 
>    Detect them using the following properties:
> 
>      - Most integrated host controllers have the usb4-host-interface
>        ACPI property, as described here:
> Link: 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
> 
>      - Integrated Thunderbolt PCIe root ports before Alder Lake do not
>        have the usb4-host-interface ACPI property. Identify those with
>        their PCI IDs instead.
> 
> 2) If a root port does not have integrated Thunderbolt capabilities, but
>    has the ExternalFacingPort ACPI property, that means the manufacturer
>    has opted to use a discrete Thunderbolt host controller that is
>    built into the computer.

Unconvincing.  If a Root Port has an external connector, is it
impossible to plug in a Thunderbolt device to that connector?  I
assume the wires from a Root Port could be traces on a PCB to a
soldered-down Thunderbolt controller, OR could be wires to a connector
where a Thunderbolt controller could be plugged in.  How could we tell
the difference?

>    This host controller can be identified by virtue of being located
>    directly below an external-facing root port that lacks integrated
>    Thunderbolt. Label it as trusted and fixed.
> 
>    Everything downstream from it is untrusted and removable.
> 
> The ExternalFacingPort ACPI property is described here:
> Link: https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-externally-exposed-pcie-root-ports

Mika Westerberg Aug. 24, 2024, 4:26 a.m. UTC | #2

On Fri, Aug 23, 2024 at 04:12:54PM -0500, Bjorn Helgaas wrote:
> On Fri, Aug 23, 2024 at 04:53:16PM +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.
> 
> Is this a firmware defect?  I asked this before, and I interpret your
> answer of "ExternalFacingPort is not 100% accurate all of the time" as
> "yes, this is a firmware defect."  That should be part of the commit
> log and code comments.
> 
> We (of course) have to work around firmware defects, but workarounds
> need to be labeled as such instead of masquerading as generic code.
> 
> > 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.
> > 
> > Detect the above scenario through the process of elimination.
> > 
> > 1) Integrated Thunderbolt host controllers already have Thunderbolt
> >    implemented, so anything outside their external facing root port is
> >    removable and untrusted.
> > 
> >    Detect them using the following properties:
> > 
> >      - Most integrated host controllers have the usb4-host-interface
> >        ACPI property, as described here:
> > Link: 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
> > 
> >      - Integrated Thunderbolt PCIe root ports before Alder Lake do not
> >        have the usb4-host-interface ACPI property. Identify those with
> >        their PCI IDs instead.
> > 
> > 2) If a root port does not have integrated Thunderbolt capabilities, but
> >    has the ExternalFacingPort ACPI property, that means the manufacturer
> >    has opted to use a discrete Thunderbolt host controller that is
> >    built into the computer.
> 
> Unconvincing.  If a Root Port has an external connector, is it
> impossible to plug in a Thunderbolt device to that connector?  I
> assume the wires from a Root Port could be traces on a PCB to a
> soldered-down Thunderbolt controller, OR could be wires to a connector
> where a Thunderbolt controller could be plugged in.  How could we tell
> the difference?

You are talking about soldered down controller vs. add-in card (e.g PCIe
slot)? We don't really distinguish those. The whole ExternalFacingPort
and the like IOMMU protection is meant for exposed hot-pluggable ports
on your laptop such as Type-C where a malicious charger for instance may
have a PCIe endpoint that steals your secrets. Not something that
requires dissasembly, opening the system cover and plugging in the
controller, connecting all the wires etc.

(We have also an additional "line of defence" in Linux because we do not
automatically create any PCIe tunnels and this is something user can
even disable completely [there is a flip in GNOME settings that disables
PCIe tunneling completely, which can be turned on whenever you are
travelling for instance]).

Bjorn Helgaas Aug. 24, 2024, 4:20 p.m. UTC | #3

On Sat, Aug 24, 2024 at 07:26:35AM +0300, Mika Westerberg wrote:
> On Fri, Aug 23, 2024 at 04:12:54PM -0500, Bjorn Helgaas wrote:
> > On Fri, Aug 23, 2024 at 04:53:16PM +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.
> > 
> > Is this a firmware defect?  I asked this before, and I interpret your
> > answer of "ExternalFacingPort is not 100% accurate all of the time" as
> > "yes, this is a firmware defect."  That should be part of the commit
> > log and code comments.
> > 
> > We (of course) have to work around firmware defects, but workarounds
> > need to be labeled as such instead of masquerading as generic code.
> > 
> > > 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.
> > > 
> > > Detect the above scenario through the process of elimination.
> > > 
> > > 1) Integrated Thunderbolt host controllers already have Thunderbolt
> > >    implemented, so anything outside their external facing root port is
> > >    removable and untrusted.
> > > 
> > >    Detect them using the following properties:
> > > 
> > >      - Most integrated host controllers have the usb4-host-interface
> > >        ACPI property, as described here:
> > > Link: 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
> > > 
> > >      - Integrated Thunderbolt PCIe root ports before Alder Lake do not
> > >        have the usb4-host-interface ACPI property. Identify those with
> > >        their PCI IDs instead.
> > > 
> > > 2) If a root port does not have integrated Thunderbolt capabilities, but
> > >    has the ExternalFacingPort ACPI property, that means the manufacturer
> > >    has opted to use a discrete Thunderbolt host controller that is
> > >    built into the computer.
> > 
> > Unconvincing.  If a Root Port has an external connector, is it
> > impossible to plug in a Thunderbolt device to that connector?  I
> > assume the wires from a Root Port could be traces on a PCB to a
> > soldered-down Thunderbolt controller, OR could be wires to a connector
> > where a Thunderbolt controller could be plugged in.  How could we tell
> > the difference?
> 
> You are talking about soldered down controller vs. add-in card (e.g PCIe
> slot)? We don't really distinguish those.

That's kind of my point.  We're depending on the platform using
ExternalFacingPort to tell us whether there's an external connector,
and in this case it sounds like the platform is lying to us.

What about PCI_EXP_FLAGS_SLOT?  If a discrete Thunderbolt controller
is built into the platform, maybe there would be no reason for the
Root Port to set Slot Implemented and provide the Slot Capabilities/
Control/Status registers.

Bjorn

Lukas Wunner Aug. 25, 2024, 2:42 p.m. UTC | #4

On Fri, Aug 23, 2024 at 04:53:16PM +0000, Esther Shimanovich wrote:
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> +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" ACPI property, so we look for
> +	 * that first. This should cover most cases.
> +	 */
> +	fwnode = fwnode_find_reference(dev_fwnode(&pdev->dev),
> +				       "usb4-host-interface", 0);

This is all ACPI only, so it should either be #ifdef'ed to CONFIG_ACPI
or moved to drivers/pci/pci-acpi.c.

Alternatively, it could be moved to arch/x86/pci/ because ACPI can also
be enabled on arm64 or riscv but the issue seems to only affect x86.


>  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)) {
> +		pci_dbg(dev, "marking as untrusted\n");
> +		dev->untrusted = true;
> +	}
>  }

I think you want to return in the "if (parent->untrusted)" case
because there's no need to double-check pcie_is_tunneled(dev)
if you've already determined that the device is untrusted.


>  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,9 +1784,13 @@ 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)) {
> +		pci_dbg(dev, "marking as removable\n");
>  		dev_set_removable(&dev->dev, DEVICE_REMOVABLE);
> +	}
>  }

Same here, return in the "if (dev_is_removable(&parent->dev))" case.

Thanks,

Lukas

Mika Westerberg Aug. 26, 2024, 4:31 a.m. UTC | #5

On Sat, Aug 24, 2024 at 11:20:42AM -0500, Bjorn Helgaas wrote:
> On Sat, Aug 24, 2024 at 07:26:35AM +0300, Mika Westerberg wrote:
> > On Fri, Aug 23, 2024 at 04:12:54PM -0500, Bjorn Helgaas wrote:
> > > On Fri, Aug 23, 2024 at 04:53:16PM +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.
> > > 
> > > Is this a firmware defect?  I asked this before, and I interpret your
> > > answer of "ExternalFacingPort is not 100% accurate all of the time" as
> > > "yes, this is a firmware defect."  That should be part of the commit
> > > log and code comments.
> > > 
> > > We (of course) have to work around firmware defects, but workarounds
> > > need to be labeled as such instead of masquerading as generic code.
> > > 
> > > > 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.
> > > > 
> > > > Detect the above scenario through the process of elimination.
> > > > 
> > > > 1) Integrated Thunderbolt host controllers already have Thunderbolt
> > > >    implemented, so anything outside their external facing root port is
> > > >    removable and untrusted.
> > > > 
> > > >    Detect them using the following properties:
> > > > 
> > > >      - Most integrated host controllers have the usb4-host-interface
> > > >        ACPI property, as described here:
> > > > Link: 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
> > > > 
> > > >      - Integrated Thunderbolt PCIe root ports before Alder Lake do not
> > > >        have the usb4-host-interface ACPI property. Identify those with
> > > >        their PCI IDs instead.
> > > > 
> > > > 2) If a root port does not have integrated Thunderbolt capabilities, but
> > > >    has the ExternalFacingPort ACPI property, that means the manufacturer
> > > >    has opted to use a discrete Thunderbolt host controller that is
> > > >    built into the computer.
> > > 
> > > Unconvincing.  If a Root Port has an external connector, is it
> > > impossible to plug in a Thunderbolt device to that connector?  I
> > > assume the wires from a Root Port could be traces on a PCB to a
> > > soldered-down Thunderbolt controller, OR could be wires to a connector
> > > where a Thunderbolt controller could be plugged in.  How could we tell
> > > the difference?
> > 
> > You are talking about soldered down controller vs. add-in card (e.g PCIe
> > slot)? We don't really distinguish those.
> 
> That's kind of my point.  We're depending on the platform using
> ExternalFacingPort to tell us whether there's an external connector,
> and in this case it sounds like the platform is lying to us.

It is defined only for PCIe Root Ports (for reasons unknown to me) so
there is no way to put it under the PCIe Downstream Ports itself that
are tunneled. The platform does the best it can here.

> What about PCI_EXP_FLAGS_SLOT?  If a discrete Thunderbolt controller
> is built into the platform, maybe there would be no reason for the
> Root Port to set Slot Implemented and provide the Slot Capabilities/
> Control/Status registers.

It is a regular PCIe device with regular PCIe link upstream and it will
even be hot-removable during the firmware upgrade.

Esther Shimanovich Aug. 28, 2024, 9:05 p.m. UTC | #6

On Sat, Aug 24, 2024 at 12:20 PM Bjorn Helgaas <[e-mail]> wrote:
>
> On Sat, Aug 24, 2024 at 07:26:35AM +0300, Mika Westerberg wrote:
> > On Fri, Aug 23, 2024 at 04:12:54PM -0500, Bjorn Helgaas wrote:
> > > On Fri, Aug 23, 2024 at 04:53:16PM +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.
> > >
> > > Is this a firmware defect?  I asked this before, and I interpret your
> > > answer of "ExternalFacingPort is not 100% accurate all of the time" as
> > > "yes, this is a firmware defect."  That should be part of the commit
> > > log and code comments.
> > >

I like how Lukas explained it here:
https://lore.kernel.org/all/[e-mail]/

It's a bit unclear whether this is firmware implemented incorrectly or
the spec not being specific enough.
Being that I see this interpretation of the spec on all devices with
discrete Thunderbolt chips (across different manufacturers)--that
makes me think that this is an ambiguity on the spec's part.

Given that, how do you suggest I modify the commit log and code comments?

> > > > 2) If a root port does not have integrated Thunderbolt capabilities, but
> > > >    has the ExternalFacingPort ACPI property, that means the manufacturer
> > > >    has opted to use a discrete Thunderbolt host controller that is
> > > >    built into the computer.
> > >
> > > Unconvincing.  If a Root Port has an external connector, is it
> > > impossible to plug in a Thunderbolt device to that connector?  I
> > > assume the wires from a Root Port could be traces on a PCB to a
> > > soldered-down Thunderbolt controller, OR could be wires to a connector
> > > where a Thunderbolt controller could be plugged in.  How could we tell
> > > the difference?
> >
We may assume this both because of how the spec is worded, and how
I've seen it implemented in the case of a discrete Thunderbolt
controller, across all cases.
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

"ExternalFacingPort" is only used when there is an externally exposed
PCIe hierarchy. (Never otherwise)
I don't know any other examples of externally exposed PCIe hierarchies
other than Thunderbolt.
Therefore I assume, that if there is "ExternalFacingPort", that means
the device has Thunderbolt capabilities.
If we have confirmed that the device has no integrated thunderbolt
capabilities, that means the device MUST expect a discrete thunderbolt
chip outside of its ExternalFacing root port.

That is my understanding of this. Also let me know how I can reword
here as well.

Esther Shimanovich Aug. 28, 2024, 9:15 p.m. UTC | #7

On Sun, Aug 25, 2024 at 10:49 AM Lukas Wunner <[e-mail]> wrote:
>
> On Fri, Aug 23, 2024 at 04:53:16PM +0000, Esther Shimanovich wrote:
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > +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" ACPI property, so we look for
> > +      * that first. This should cover most cases.
> > +      */
> > +     fwnode = fwnode_find_reference(dev_fwnode(&pdev->dev),
> > +                                    "usb4-host-interface", 0);
>
> This is all ACPI only, so it should either be #ifdef'ed to CONFIG_ACPI
> or moved to drivers/pci/pci-acpi.c.
>
> Alternatively, it could be moved to arch/x86/pci/ because ACPI can also
> be enabled on arm64 or riscv but the issue seems to only affect x86.

Thanks for the feedback! Adding an #ifdef to CONFIG_ACPI seems more
straightforward, but I do like the idea of not having unnecessary code
run on non-x86 systems.

I'd appreciate some guidance here. How would I move a portion of a
function into a completely different location in the kernel src?
Could you show me an example?
I'm assuming you'd want me to write another function elsewhere.
Shouldn't it be in some existing file in
`include/linux/platform_data/x86`? I don't see `arch/x86/pci/`
included anywhere.
Or would CONFIG_X86 be sufficient? (instead of or in addition to CONFIG_ACPI?)

Lukas Wunner Aug. 29, 2024, 9:32 a.m. UTC | #8

On Wed, Aug 28, 2024 at 05:15:24PM -0400, Esther Shimanovich wrote:
> On Sun, Aug 25, 2024 at 10:49???AM Lukas Wunner <[e-mail]> wrote:
> > On Fri, Aug 23, 2024 at 04:53:16PM +0000, Esther Shimanovich wrote:
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > +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" ACPI property, so we look for
> > > +      * that first. This should cover most cases.
> > > +      */
> > > +     fwnode = fwnode_find_reference(dev_fwnode(&pdev->dev),
> > > +                                    "usb4-host-interface", 0);
> >
> > This is all ACPI only, so it should either be #ifdef'ed to CONFIG_ACPI
> > or moved to drivers/pci/pci-acpi.c.
> >
> > Alternatively, it could be moved to arch/x86/pci/ because ACPI can also
> > be enabled on arm64 or riscv but the issue seems to only affect x86.
> 
> Thanks for the feedback! Adding an #ifdef to CONFIG_ACPI seems more
> straightforward, but I do like the idea of not having unnecessary code
> run on non-x86 systems.
> 
> I'd appreciate some guidance here. How would I move a portion of a
> function into a completely different location in the kernel src?
> Could you show me an example?

One way to do this would be to move pcie_is_tunneled(),
pcie_has_usb4_host_interface() and pcie_switch_directly_under()
to arch/x86/pci/acpi.c.

Rename pcie_is_tunneled() to arch_pci_dev_is_removable() and remove
the "static" declaration specifier from that function.

Add a function declaration for arch_pci_dev_is_removable() to
include/linux/pci.h.

Add a __weak arch_pci_dev_is_removable() function which just returns
false in drivers/pci/probe.c right above pci_set_removable().

And that's it.

See pcibios_device_add() for an example.

That's one way to do it.  It ensures that the code is only compiled
on x86 and only if CONFIG_ACPI=y.  Basically the linker picks the
arch_pci_dev_is_removable() in arch/x86/pci/acpi.c, or the empty
__weak function of the same name on !x86 or if CONFIG_ACPI=n.

An alternative approach would involve using an empty static inline.
I think the difference is that an empty static inline is optimized
away by the compiler, whereas the empty __weak function is not
optimized away by the compiler, but may be optimized away by the
linker if CONFIG_LTO=y.

For the static inline it's basically the same but you omit the
__weak arch_pci_dev_is_removable() in drivers/pci/probe.c and
instead constrain the function declaration in include/linux/pci.h to:
#if defined(CONFIG_X86) && defined(CONFIG_ACPI)
...and the #else branch would contain the empty static inline
which just returns false.

See pci_mmcfg_early_init() for an example.

Maybe the empty static inline is better because then the entire
"if (arch_pci_dev_is_removable(...))" clause can be optimized away
without reliance on CONFIG_LTO=y.

I hope I haven't confused you completely.

Thanks,

Lukas

Esther Shimanovich Sept. 30, 2024, 6:23 p.m. UTC | #9

Hello!

Just a friendly ping! I see that this may have gotten lost in the mix.

Have we addressed your main concerns? How would you want me to reword things?

This is the latest patch in this series:
https://lore.kernel.org/all/[e-mail]/
Thank you!
Esther

Bjorn Helgaas Oct. 23, 2024, 8:58 p.m. UTC | #10

On Thu, Aug 29, 2024 at 11:32:47AM +0200, Lukas Wunner wrote:
> On Wed, Aug 28, 2024 at 05:15:24PM -0400, Esther Shimanovich wrote:
> > On Sun, Aug 25, 2024 at 10:49???AM Lukas Wunner <[e-mail]> wrote:
> > > On Fri, Aug 23, 2024 at 04:53:16PM +0000, Esther Shimanovich wrote:
> > > > --- a/drivers/pci/probe.c
> > > > +++ b/drivers/pci/probe.c
> > > > +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" ACPI property, so we look for
> > > > +      * that first. This should cover most cases.
> > > > +      */
> > > > +     fwnode = fwnode_find_reference(dev_fwnode(&pdev->dev),
> > > > +                                    "usb4-host-interface", 0);
> > >
> > > This is all ACPI only, so it should either be #ifdef'ed to CONFIG_ACPI
> > > or moved to drivers/pci/pci-acpi.c.
> > >
> > > Alternatively, it could be moved to arch/x86/pci/ because ACPI can also
> > > be enabled on arm64 or riscv but the issue seems to only affect x86.
> > 
> > Thanks for the feedback! Adding an #ifdef to CONFIG_ACPI seems more
> > straightforward, but I do like the idea of not having unnecessary code
> > run on non-x86 systems.
> > 
> > I'd appreciate some guidance here. How would I move a portion of a
> > function into a completely different location in the kernel src?
> > Could you show me an example?
> 
> One way to do this would be to move pcie_is_tunneled(),
> pcie_has_usb4_host_interface() and pcie_switch_directly_under()
> to arch/x86/pci/acpi.c.
> 
> Rename pcie_is_tunneled() to arch_pci_dev_is_removable() and remove
> the "static" declaration specifier from that function.
> 
> Add a function declaration for arch_pci_dev_is_removable() to
> include/linux/pci.h.
> 
> Add a __weak arch_pci_dev_is_removable() function which just returns
> false in drivers/pci/probe.c right above pci_set_removable().
> 
> And that's it.
> 
> See pcibios_device_add() for an example.
> 
> That's one way to do it.  It ensures that the code is only compiled
> on x86 and only if CONFIG_ACPI=y.  Basically the linker picks the
> arch_pci_dev_is_removable() in arch/x86/pci/acpi.c, or the empty
> __weak function of the same name on !x86 or if CONFIG_ACPI=n.
> 
> An alternative approach would involve using an empty static inline.
> I think the difference is that an empty static inline is optimized
> away by the compiler, whereas the empty __weak function is not
> optimized away by the compiler, but may be optimized away by the
> linker if CONFIG_LTO=y.
> 
> For the static inline it's basically the same but you omit the
> __weak arch_pci_dev_is_removable() in drivers/pci/probe.c and
> instead constrain the function declaration in include/linux/pci.h to:
> #if defined(CONFIG_X86) && defined(CONFIG_ACPI)
> ...and the #else branch would contain the empty static inline
> which just returns false.
> 
> See pci_mmcfg_early_init() for an example.
> 
> Maybe the empty static inline is better because then the entire
> "if (arch_pci_dev_is_removable(...))" clause can be optimized away
> without reliance on CONFIG_LTO=y.

Was there ever any followup on this?  Do we need any?

This uses fwnode_find_reference("usb4-host-interface"), and while
"usb4-host-interface" is only defined for ACPI systems (as far as I
know), the fwnode_find_reference() interface itself is not
ACPI-specific.

So maybe this is OK as-is, and it will just never find that property
on non-ACPI systems?

Bjorn

Bjorn Helgaas Oct. 23, 2024, 9:06 p.m. UTC | #11

On Wed, Oct 23, 2024 at 03:58:21PM -0500, Bjorn Helgaas wrote:
> On Thu, Aug 29, 2024 at 11:32:47AM +0200, Lukas Wunner wrote:
> > On Wed, Aug 28, 2024 at 05:15:24PM -0400, Esther Shimanovich wrote:
> > > On Sun, Aug 25, 2024 at 10:49???AM Lukas Wunner <[e-mail]> wrote:
> > > > On Fri, Aug 23, 2024 at 04:53:16PM +0000, Esther Shimanovich wrote:
> > > > > --- a/drivers/pci/probe.c
> > > > > +++ b/drivers/pci/probe.c
> > > > > +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" ACPI property, so we look for
> > > > > +      * that first. This should cover most cases.
> > > > > +      */
> > > > > +     fwnode = fwnode_find_reference(dev_fwnode(&pdev->dev),
> > > > > +                                    "usb4-host-interface", 0);
> > > >
> > > > This is all ACPI only, so it should either be #ifdef'ed to CONFIG_ACPI
> > > > or moved to drivers/pci/pci-acpi.c.
> > > >
> > > > Alternatively, it could be moved to arch/x86/pci/ because ACPI can also
> > > > be enabled on arm64 or riscv but the issue seems to only affect x86.
> > > 
> > > Thanks for the feedback! Adding an #ifdef to CONFIG_ACPI seems more
> > > straightforward, but I do like the idea of not having unnecessary code
> > > run on non-x86 systems.
> > > 
> > > I'd appreciate some guidance here. How would I move a portion of a
> > > function into a completely different location in the kernel src?
> > > Could you show me an example?
> > 
> > One way to do this would be to move pcie_is_tunneled(),
> > pcie_has_usb4_host_interface() and pcie_switch_directly_under()
> > to arch/x86/pci/acpi.c.
> > 
> > Rename pcie_is_tunneled() to arch_pci_dev_is_removable() and remove
> > the "static" declaration specifier from that function.
> > 
> > Add a function declaration for arch_pci_dev_is_removable() to
> > include/linux/pci.h.
> > 
> > Add a __weak arch_pci_dev_is_removable() function which just returns
> > false in drivers/pci/probe.c right above pci_set_removable().
> > 
> > And that's it.
> > 
> > See pcibios_device_add() for an example.
> > 
> > That's one way to do it.  It ensures that the code is only compiled
> > on x86 and only if CONFIG_ACPI=y.  Basically the linker picks the
> > arch_pci_dev_is_removable() in arch/x86/pci/acpi.c, or the empty
> > __weak function of the same name on !x86 or if CONFIG_ACPI=n.
> > 
> > An alternative approach would involve using an empty static inline.
> > I think the difference is that an empty static inline is optimized
> > away by the compiler, whereas the empty __weak function is not
> > optimized away by the compiler, but may be optimized away by the
> > linker if CONFIG_LTO=y.
> > 
> > For the static inline it's basically the same but you omit the
> > __weak arch_pci_dev_is_removable() in drivers/pci/probe.c and
> > instead constrain the function declaration in include/linux/pci.h to:
> > #if defined(CONFIG_X86) && defined(CONFIG_ACPI)
> > ...and the #else branch would contain the empty static inline
> > which just returns false.
> > 
> > See pci_mmcfg_early_init() for an example.
> > 
> > Maybe the empty static inline is better because then the entire
> > "if (arch_pci_dev_is_removable(...))" clause can be optimized away
> > without reliance on CONFIG_LTO=y.
> 
> Was there ever any followup on this?  Do we need any?
> 
> This uses fwnode_find_reference("usb4-host-interface"), and while
> "usb4-host-interface" is only defined for ACPI systems (as far as I
> know), the fwnode_find_reference() interface itself is not
> ACPI-specific.
> 
> So maybe this is OK as-is, and it will just never find that property
> on non-ACPI systems?

Sigh, sorry for the noise.  Right after sending this, I noticed that
Esther had posted a v5 with this rework:
https://lore.kernel.org/r/[e-mail]

Although I'm still unclear on whether we actually need to make this
ACPI-specific as v5 does.

I'm also not sure about making it x86-specific, since the
"usb4-host-interface" property doesn't seem x86-specific, other than
maybe as an accidental consequence of some hardware implementations.

Bjorn

Patch

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index b14b9876c030..816f1f879fa3 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1629,25 +1629,152 @@  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, check if it is below the
+	 * corresponding PCIe switch components (if applicable). Then check
+	 * if its upstream port is directly beneath the specified bridge.
+	 */
+	switch (pci_pcie_type(pdev)) {
+	case PCI_EXP_TYPE_UPSTREAM:
+		return parent == bridge;
+
+	case PCI_EXP_TYPE_DOWNSTREAM:
+		if (pci_pcie_type(parent) != PCI_EXP_TYPE_UPSTREAM)
+			return false;
+		parent = pci_upstream_bridge(parent);
+		return parent == bridge;
+
+	case PCI_EXP_TYPE_ENDPOINT:
+		if (pci_pcie_type(parent) != PCI_EXP_TYPE_DOWNSTREAM)
+			return false;
+		parent = pci_upstream_bridge(parent);
+		if (!parent || pci_pcie_type(parent) != PCI_EXP_TYPE_UPSTREAM)
+			return false;
+		parent = pci_upstream_bridge(parent);
+		return parent == bridge;
+	}
+
+	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" ACPI property, so we look for
+	 * that first. This should cover 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 "usb4-host-interface"
+	 * 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;
+
+	/* pdev without a parent or Root Port is never tunneled. */
+	parent = pci_upstream_bridge(pdev);
+	if (!parent)
+		return false;
+	root = pcie_find_root_port(pdev);
+	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". Those 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)) {
+		pci_dbg(dev, "marking as untrusted\n");
+		dev->untrusted = true;
+	}
 }
 
 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,9 +1784,13 @@  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)) {
+		pci_dbg(dev, "marking as removable\n");
 		dev_set_removable(&dev->dev, DEVICE_REMOVABLE);
+	}
 }
 
 /**