| 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
| ||
|
Message-ID: <20250715224705.GA2504490@bhelgaas>
Date: Tue, 15 Jul 2025 17:47:05 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Hans Zhang <18255117159@....com>
Cc: lpieralisi@...nel.org, bhelgaas@...gle.com, mani@...nel.org,
ilpo.jarvinen@...ux.intel.com, kwilczynski@...nel.org,
robh@...nel.org, jingoohan1@...il.com, linux-pci@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v13 3/6] PCI: Refactor capability search into common
macros
On Sun, Jun 08, 2025 at 12:14:02AM +0800, Hans Zhang wrote:
> The PCI Capability search functionality is duplicated across the PCI core
> and several controller drivers. The core's current implementation requires
> fully initialized PCI device and bus structures, which prevents controller
> drivers from using it during early initialization phases before these
> structures are available.
>
> Move the Capability search logic into a header-based macro that accepts a
> config space accessor function as an argument. This enables controller
> drivers to perform Capability discovery using their early access
> mechanisms prior to full device initialization while sharing the
> Capability search code.
>
> Convert the existing PCI core Capability search implementation to use this
> new macro. Controller drivers can later use the same macros with their
> early access mechanisms while maintaining the existing protection against
> infinite loops through preserved TTL checks.
>
> The ttl parameter was originally an additional safeguard to prevent
> infinite loops in corrupted config space. However, the
> PCI_FIND_NEXT_CAP_TTL() macro already enforces a TTL limit internally.
> Removing redundant ttl handling simplifies the interface while maintaining
> the safety guarantee. This aligns with the macro's design intent of
> encapsulating TTL management.
This is a big gulp, but I think I like it :) It really enables some
nice cleanups later.
> -static u8 __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int devfn,
> - u8 pos, int cap, int *ttl)
> -{
> - u8 id;
> - u16 ent;
> -
> - pci_bus_read_config_byte(bus, devfn, pos, &pos);
> -
> - while ((*ttl)--) {
> - if (pos < PCI_STD_HEADER_SIZEOF)
> - break;
> - pos = ALIGN_DOWN(pos, 4);
> - pci_bus_read_config_word(bus, devfn, pos, &ent);
> -
> - id = FIELD_GET(PCI_CAP_ID_MASK, ent);
> - if (id == 0xff)
> - break;
> - if (id == cap)
> - return pos;
> - pos = FIELD_GET(PCI_CAP_LIST_NEXT_MASK, ent);
> - }
> - return 0;
> -}
> -
> static u8 __pci_find_next_cap(struct pci_bus *bus, unsigned int devfn,
> u8 pos, int cap)
> {
> - int ttl = PCI_FIND_CAP_TTL;
> -
> - return __pci_find_next_cap_ttl(bus, devfn, pos, cap, &ttl);
> + return PCI_FIND_NEXT_CAP_TTL(pci_bus_read_config, pos, cap, bus, devfn);
> }
>
> u8 pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap)
> @@ -554,42 +527,11 @@ EXPORT_SYMBOL(pci_bus_find_capability);
> */
> u16 pci_find_next_ext_capability(struct pci_dev *dev, u16 start, int cap)
> {
> - u32 header;
> - int ttl;
> - u16 pos = PCI_CFG_SPACE_SIZE;
> -
> - /* minimum 8 bytes per capability */
> - ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
> -
> if (dev->cfg_size <= PCI_CFG_SPACE_SIZE)
> return 0;
>
> - if (start)
> - pos = start;
> -
> - if (pci_read_config_dword(dev, pos, &header) != PCIBIOS_SUCCESSFUL)
> - return 0;
> -
> - /*
> - * If we have no capabilities, this is indicated by cap ID,
> - * cap version and next pointer all being 0.
> - */
> - if (header == 0)
> - return 0;
> -
> - while (ttl-- > 0) {
> - if (PCI_EXT_CAP_ID(header) == cap && pos != start)
> - return pos;
> -
> - pos = PCI_EXT_CAP_NEXT(header);
> - if (pos < PCI_CFG_SPACE_SIZE)
> - break;
> -
> - if (pci_read_config_dword(dev, pos, &header) != PCIBIOS_SUCCESSFUL)
> - break;
> - }
> -
> - return 0;
> + return PCI_FIND_NEXT_EXT_CAPABILITY(pci_bus_read_config, start, cap,
> + dev->bus, dev->devfn);
> }
> EXPORT_SYMBOL_GPL(pci_find_next_ext_capability);
>
> @@ -649,7 +591,7 @@ EXPORT_SYMBOL_GPL(pci_get_dsn);
>
> static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
> {
> - int rc, ttl = PCI_FIND_CAP_TTL;
> + int rc;
> u8 cap, mask;
>
> if (ht_cap == HT_CAPTYPE_SLAVE || ht_cap == HT_CAPTYPE_HOST)
> @@ -657,8 +599,8 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
> else
> mask = HT_5BIT_CAP_MASK;
>
> - pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn, pos,
> - PCI_CAP_ID_HT, &ttl);
> + pos = PCI_FIND_NEXT_CAP_TTL(pci_bus_read_config, pos,
> + PCI_CAP_ID_HT, dev->bus, dev->devfn);
> while (pos) {
> rc = pci_read_config_byte(dev, pos + 3, &cap);
> if (rc != PCIBIOS_SUCCESSFUL)
> @@ -667,9 +609,10 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
> if ((cap & mask) == ht_cap)
> return pos;
>
> - pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn,
> - pos + PCI_CAP_LIST_NEXT,
> - PCI_CAP_ID_HT, &ttl);
> + pos = PCI_FIND_NEXT_CAP_TTL(pci_bus_read_config,
> + pos + PCI_CAP_LIST_NEXT,
> + PCI_CAP_ID_HT, dev->bus,
> + dev->devfn);
> }
>
> return 0;
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index e7d31ed56731..46fb6b5a854e 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -2,6 +2,8 @@
> #ifndef DRIVERS_PCI_H
> #define DRIVERS_PCI_H
>
> +#include <linux/align.h>
> +#include <linux/bitfield.h>
> #include <linux/pci.h>
>
> struct pcie_tlp_log;
> @@ -93,6 +95,89 @@ bool pcie_cap_has_rtctl(const struct pci_dev *dev);
> int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32 size,
> u32 *val);
>
> +/* Standard Capability finder */
> +/**
> + * PCI_FIND_NEXT_CAP_TTL - Find a PCI standard capability
I don't think "_TTL" is relevant in the macro name here. I see it
copied the previous __pci_find_next_cap_ttl() name; "ttl" *was*
relevant there, but it isn't anymore.
I would like this a lot better if it could be implemented as a
function, but I assume it has to be a macro for some varargs reason.
> + * @read_cfg: Function pointer for reading PCI config space
> + * @start: Starting position to begin search
> + * @cap: Capability ID to find
> + * @args: Arguments to pass to read_cfg function
> + *
> + * Iterates through the capability list in PCI config space to find
> + * @cap. Implements TTL (time-to-live) protection against infinite loops.
> + *
> + * Returns: Position of the capability if found, 0 otherwise.
> + */
> +#define PCI_FIND_NEXT_CAP_TTL(read_cfg, start, cap, args...) \
> +({ \
> + int __ttl = PCI_FIND_CAP_TTL; \
> + u8 __id, __found_pos = 0; \
> + u8 __pos = (start); \
> + u16 __ent; \
> + \
> + read_cfg(args, __pos, 1, (u32 *)&__pos); \
> + \
> + while (__ttl--) { \
> + if (__pos < PCI_STD_HEADER_SIZEOF) \
> + break; \
I guess this is just lifted directly from __pci_find_next_cap_ttl(),
but wow, I wish it weren't quite *so* subtle. Totally fine though.
Maybe this could be split into one patch for standard capabilities and
a second for extended capabilities, just so the connection between
this and __pci_find_next_cap_ttl() would be clearer.
> + \
> + __pos = ALIGN_DOWN(__pos, 4); \
> + read_cfg(args, __pos, 2, (u32 *)&__ent); \
> + \
> + __id = FIELD_GET(PCI_CAP_ID_MASK, __ent); \
> + if (__id == 0xff) \
> + break; \
> + \
> + if (__id == (cap)) { \
> + __found_pos = __pos; \
> + break; \
> + } \
> + \
> + __pos = FIELD_GET(PCI_CAP_LIST_NEXT_MASK, __ent); \
> + } \
> + __found_pos; \
> +})
> +
> +/* Extended Capability finder */
> +/**
> + * PCI_FIND_NEXT_EXT_CAPABILITY - Find a PCI extended capability
Can this be named similarly to the above? PCI_FIND_NEXT_CAP_TTL() and
PCI_FIND_NEXT_EXT_CAPABILITY() seem needlessly different.
PCI_FIND_NEXT_CAP() and PCI_FIND_NEXT_EXT_CAP()?
> + * @read_cfg: Function pointer for reading PCI config space
> + * @start: Starting position to begin search (0 for initial search)
> + * @cap: Extended capability ID to find
> + * @args: Arguments to pass to read_cfg function
> + *
> + * Searches the extended capability space in PCI config registers
> + * for @cap. Implements TTL protection against infinite loops using
> + * a calculated maximum search count.
> + *
> + * Returns: Position of the capability if found, 0 otherwise.
> + */
> +#define PCI_FIND_NEXT_EXT_CAPABILITY(read_cfg, start, cap, args...) \
It would be *really* nice if you could make this fit nicely in 80
columns as PCI_FIND_NEXT_CAP_TTL() does.
> +({ \
> + u16 __pos = (start) ?: PCI_CFG_SPACE_SIZE; \
> + u16 __found_pos = 0; \
> + int __ttl, __ret; \
> + u32 __header; \
> + \
> + __ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8; \
> + while (__ttl-- > 0 && __pos >= PCI_CFG_SPACE_SIZE) { \
> + __ret = read_cfg(args, __pos, 4, &__header); \
> + if (__ret != PCIBIOS_SUCCESSFUL) \
> + break; \
> + \
> + if (__header == 0) \
> + break; \
> + \
> + if (PCI_EXT_CAP_ID(__header) == (cap) && __pos != start) { \
> + __found_pos = __pos; \
> + break; \
> + } \
> + \
> + __pos = PCI_EXT_CAP_NEXT(__header); \
> + } \
> + __found_pos; \
> +})
> +
> /* Functions internal to the PCI core code */
>
> #ifdef CONFIG_DMI
> --
> 2.25.1
>
Powered by blists - more mailing lists