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: <ZtBAP4TayLmdiya_@wunner.de>
Date: Thu, 29 Aug 2024 11:32:47 +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 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.

I hope I haven't confused you completely.

Thanks,

Lukas