[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZstCyti3FHZIeFO8@wunner.de>
Date: Sun, 25 Aug 2024 16:42:18 +0200
From: Lukas Wunner <lukas@...ner.de>
To: Esther Shimanovich <eshimanovich@...omium.org>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, Rajat Jain <rajatja@...gle.com>,
"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
Mario Limonciello <mario.limonciello@....com>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
iommu@...ts.linux.dev,
Mika Westerberg <mika.westerberg@...ux.intel.com>,
linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] PCI: Detect and trust built-in Thunderbolt chips
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
Powered by blists - more mailing lists