| 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: <ef0237d9-f5da-44d7-ab45-2be037cf0f25@163.com>
Date: Fri, 4 Apr 2025 00:35:40 +0800
From: Hans Zhang <18255117159@....com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: lpieralisi@...nel.org, bhelgaas@...gle.com, kw@...ux.com,
manivannan.sadhasivam@...aro.org, robh@...nel.org, jingoohan1@...il.com,
thomas.richard@...tlin.com, linux-pci@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [v7 2/5] PCI: Refactor capability search functions to eliminate
code duplication
On 2025/4/3 20:24, Hans Zhang wrote:
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(pci_find_next_ext_capability);
>>>>> @@ -648,7 +614,6 @@ 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;
>>>>> u8 cap, mask;
>>>>> if (ht_cap == HT_CAPTYPE_SLAVE || ht_cap == HT_CAPTYPE_HOST)
>>>>> @@ -657,7 +622,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)
>>>>> @@ -668,7 +633,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);
>>>>
>>>> This function kind of had the idea to share the ttl but I suppose
>>>> that was
>>>> just a final safeguard to make sure the loop will always terminate
>>>> in case
>>>> the config space is corrupted so the unsharing is not a big issue.
>>>>
>>>
>>> __pci_find_next_cap_ttl
>>> // This macro definition already has ttl loop restrictions inside it.
>>> PCI_FIND_NEXT_CAP_TTL
>>>
>>> Do I understand that you agree to remove ttl initialization and
>>> parameter
>>> passing?
>>
>> Yes, I agree with it but doing anything like this (although I'd mention
>> the reasoning in the changelog myself).
>>
>
> Ok, I see. I will give the reasons.
Hi Ilpo,
The [v9 3/6]patch I plan to submit is as follows, please review it.
From 6da415d130e76b57ecf401f14bf0b66f20407839 Mon Sep 17 00:00:00 2001
From: Hans Zhang <18255117159@....com>
Date: Fri, 4 Apr 2025 00:20:29 +0800
Subject: [v9 3/6] PCI: Refactor capability search into common macros
- Capability search is done both in PCI core and some controller drivers.
- PCI core's cap search func requires PCI device and bus structs exist.
- Controller drivers cannot use PCI core's cap search func as they
need to find capabilities before they instantiated the PCI device & bus
structs.
- Move capability search into a macro so it can be reused where normal
PCI config space accessors cannot yet be used due to lack of the
instantiated PCI dev.
- Instead, give the config space reading function as an argument to the
new macro.
- Convert PCI core to use the new macro.
The macros now implement, parameterized by the config access method. The
PCI core functions are converted to utilize these macros with the standard
pci_bus_read_config accessors. 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>
---
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 e4d3719b653d..bef242d84ab4 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 6a7c88b9cd35..b204ebeeb1cf 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); \
+ \
+ __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
Best regards,
Hans
Powered by blists - more mailing lists