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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241023205818.GA930054@bhelgaas>
Date: Wed, 23 Oct 2024 15:58:18 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Lukas Wunner <lukas@...ner.de>
Cc: Esther Shimanovich <eshimanovich@...omium.org>,
	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 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 <lukas@...ner.de> 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