| 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: <62b35061-ca77-40ce-b10b-709dd862285e@163.com>
Date: Tue, 3 Jun 2025 23:55:27 +0800
From: Hans Zhang <18255117159@....com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: lpieralisi@...nel.org, bhelgaas@...gle.com,
manivannan.sadhasivam@...aro.org, kw@...ux.com, cassel@...nel.org,
robh@...nel.org, jingoohan1@...il.com, linux-pci@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v12 3/6] PCI: Refactor capability search into common
macros
On 2025/6/3 17:38, Ilpo Järvinen wrote:
> On Thu, 15 May 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. 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.
>
> PCI_FIND_NEXT_CAP_TTL()
Dear Ilpo,
Will change.
>
>> 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 v11:
>> - Add #include <linux/bitfield.h>, solve the compilation warnings caused by the subsequent patch calls.
>>
>> Changes since v10:
>> - Remove #include <uapi/linux/pci_regs.h>.
>> - The patch commit message were modified.
>>
>> Changes since v9:
>> - None
>>
>> Changes since v8:
>> - The patch commit message were modified.
>> ---
>> drivers/pci/pci.c | 69 +++++--------------------------------
>> drivers/pci/pci.h | 86 +++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 95 insertions(+), 60 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 27d2adb18a30..271d922abdcc 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>
>> @@ -425,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);
>> }
Will delete __pci_find_next_cap_ttl function.
>>
>> 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);
>> }
>
> Please just get rid of the ttl variant, use PCI_FIND_NEXT_CAP_TTL()
> directly here, and adjust the other callers of the ttl variable to call
> this one instead.
>
Will change.
>>
>> u8 pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap)
>> @@ -554,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);
>>
>> @@ -649,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)
>> @@ -658,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,
Will use PCI_FIND_NEXT_CAP_TTL().
>> - PCI_CAP_ID_HT, &ttl);
>> + PCI_CAP_ID_HT);
>> while (pos) {
>> rc = pci_read_config_byte(dev, pos + 3, &cap);
>> if (rc != PCIBIOS_SUCCESSFUL)
>> @@ -669,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,
Will use PCI_FIND_NEXT_CAP_TTL().
>> 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..f9cf45026e6e 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;
>> @@ -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
>
> to find @cap.
Will change.
>
>> + * 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); \
>> + \
>> + __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
>
> for @cap.
Will change.
Best regards,
Hans
>
>> + * 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
>>
>
Powered by blists - more mailing lists