| 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: <df929c0d-f318-023e-cf7d-97a2b344f6fc@linux.intel.com>
Date: Wed, 30 Apr 2025 11:03:30 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Hans Zhang <18255117159@....com>
cc: lpieralisi@...nel.org, bhelgaas@...gle.com,
manivannan.sadhasivam@...aro.org, kw@...ux.com, cassel@...nel.org,
robh@...nel.org, jingoohan1@...il.com, thomas.richard@...tlin.com,
linux-pci@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v10 3/6] PCI: Refactor capability search into common
macros
On Tue, 29 Apr 2025, 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.
>
> The macros now implement, parameterized by the config access method. The
This sentence is incomplete (and sounds pretty much duplicated
information anyway).
> PCI core functions are converted to utilize these macros with the standard
> pci_bus_read_config accessors.
This info is duplicated.
> 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.
>
> Signed-off-by: Hans Zhang <18255117159@....com>
> ---
> Changes since v9:
> - None
>
> Changes since v8:
> - The patch commit message were modified.
> ---
> drivers/pci/pci.c | 70 +++++---------------------------------
> drivers/pci/pci.h | 86 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 95 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 1c29e8f20cb5..8a1f47e8f5cc 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -9,7 +9,6 @@
> */
>
> #include <linux/acpi.h>
> -#include <linux/align.h>
> #include <linux/kernel.h>
> #include <linux/delay.h>
> #include <linux/dmi.h>
> @@ -31,7 +30,6 @@
> #include <asm/dma.h>
> #include <linux/aer.h>
> #include <linux/bitfield.h>
> -#include <uapi/linux/pci_regs.h>
> #include "pci.h"
>
> DEFINE_MUTEX(pci_slot_mutex);
> @@ -426,35 +424,16 @@ static int pci_dev_str_match(struct pci_dev *dev, const char *p,
> }
>
> static u8 __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int devfn,
> - u8 pos, int cap, int *ttl)
> + u8 pos, int cap)
> {
> - 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;
> + return PCI_FIND_NEXT_CAP_TTL(pci_bus_read_config, pos, cap, bus,
> + devfn);
> }
>
> 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(bus, devfn, pos, cap);
> }
>
> u8 pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap)
> @@ -555,42 +534,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);
>
> @@ -650,7 +598,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)
> @@ -659,7 +607,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
> mask = HT_5BIT_CAP_MASK;
>
> pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn, pos,
> - PCI_CAP_ID_HT, &ttl);
> + PCI_CAP_ID_HT);
> while (pos) {
> rc = pci_read_config_byte(dev, pos + 3, &cap);
> if (rc != PCIBIOS_SUCCESSFUL)
> @@ -670,7 +618,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
>
> pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn,
> pos + PCI_CAP_LIST_NEXT,
> - PCI_CAP_ID_HT, &ttl);
> + PCI_CAP_ID_HT);
> }
>
> return 0;
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 5e1477d6e254..79cd6402ba8d 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -2,7 +2,9 @@
> #ifndef DRIVERS_PCI_H
> #define DRIVERS_PCI_H
>
> +#include <linux/align.h>
> #include <linux/pci.h>
> +#include <uapi/linux/pci_regs.h>
>
> struct pcie_tlp_log;
>
> @@ -91,6 +93,90 @@ 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
> + * @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
> + * the specified capability. 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; \
> + \
> + __pos = ALIGN_DOWN(__pos, 4); \
> + read_cfg(args, __pos, 2, (u32 *)&__ent); \
You seem to have dropped the return value checks?
> + \
> + __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
> + * @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 the specified capability. 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...) \
> +({ \
> + 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
>
--
i.
Powered by blists - more mailing lists