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

[Lukas Wunner]

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