Linux PCI subsystem development
 help / color / mirror / Atom feed
* [PATCH v2] PCI/AER: Map a raw AER Capability image field by field
@ 2026-09-09 16:31 Dave Jiang
  2026-09-09 16:45 ` sashiko-bot
  0 siblings, 1 reply; 3+ messages in thread
From: Dave Jiang @ 2026-09-09 16:31 UTC (permalink / raw)
  To: linux-cxl, linux-pci, linux-acpi
  Cc: bhelgaas, ilpo.jarvinen, rafael, lenb, bp, guohanjun, mchehab,
	xueshuai, jic23, terry.bowman, sashiko-bot

struct aer_capability_regs is not the hardware layout: the embedded
struct pcie_tlp_log spans 60 bytes where the Header Log it stands in for
is 16, so a flat copy misplaces everything behind it.

extlog_print_pcie(), ghes_handle_aer() and cxl_rch_get_aer_info() work
around that by stopping at the Header Log, dropping everything past
offset 44. Flit mode is never decoded either: the Flit bit and Logged
TLP Size sit at offset 0x18 in all three images, but nothing reads them,
so the Flit DWORDs at 0x38 are lost and the log prints as non-Flit.

Add aer_cap_regs_unpack() to map the registers individually, taking the
TLP Log layout from the Flit bit as pcie_read_tlp_log() does. Clamp the
logged length: Logged TLP Size is 5 bits wide, so an untrusted value
reaches 31 where dw[] holds 14 and pcie_print_tlp_log() walks
header_len unbounded.

Convert all three callers. cxl_rch_get_aer_info() now reads the whole
MMIO block, so size its ioremap by the hardware registers (96 bytes)
rather than by sizeof(struct aer_capability_regs) (100). No functional
change there: only the first 44 bytes were ever read.

Reported-by: sashiko-bot@kernel.org
Closes: https://sashiko.dev/#/patchset/20260904172337.1409775-1-dave.jiang@intel.com?part=5
Link: https://lore.kernel.org/linux-cxl/CAJZ5v0hDv11cPuztPZsaDd7uwD_49KznJy=tzuRO+dZc=CnAEQ@mail.gmail.com/
Assisted-by: LLM
Signed-off-by: Dave Jiang <dave.jiang@intel.com>

---
v2, all from Jonathan's review:
- Assisted-by: LLM, per Documentation/process/coding-assistants.rst
- Fold the aer_cap_regs_unpack() calls onto one line
- Say why cxl_dport_map_rch_aer()'s ioremap size changes
- Declare the readl() loop counter in the loop
- FIELD_GET() for the Flit bit; min() for both clamps
- Note in the kernel-doc why @raw_len is bounded

Depends on "[PATCH v6 00/13] ACPI: APEI: GHES: Collection of fixes for
issues reported by sashiko", now picked up by Rafael; please apply after
it. Patches 5 and 12 are build dependencies - this patch rewrites the
aer_info copies they add - and patch 7 is functional: without its
ACPI_APEI_PCIEAER guard fix, extlog_print_pcie() compiles out. The
drivers/pci and drivers/cxl changes are independent of that series.

No Fixes: tag on purpose. Patches 5 and 12 closed the out-of-bounds
reads; what is left is diagnostic completeness, and this adds an
exported helper and rewrites three call sites.

This is the follow-up Rafael asked for in the Link: above, and also
covers the pre-existing ras_rch.c findings reported there.
---
 drivers/acpi/acpi_extlog.c | 14 +-----
 drivers/acpi/apei/ghes.c   | 15 +------
 drivers/cxl/core/ras_rch.c | 28 +++---------
 drivers/pci/pcie/tlp.c     | 88 ++++++++++++++++++++++++++++++++++++++
 include/linux/aer.h        |  9 ++++
 5 files changed, 106 insertions(+), 48 deletions(-)

diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index 9e61354a807b..bf5f7b1a9aea 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -156,19 +156,7 @@ static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
 
 	aer_severity = cper_severity_to_aer(severity);
 
-	/*
-	 * struct pcie_tlp_log is larger than the hardware layout, so aer_info
-	 * only maps onto the struct up to the four Header Log DWORDs. Copy that
-	 * much, then place the TLP Prefix Log from where the hardware keeps it.
-	 * Everything else stays zero: nothing reads root_command, root_status or
-	 * the error source IDs, and header_len and flit are software-only.
-	 */
-	memcpy(&aer_regs, pcie_err->aer_info,
-	       offsetof(struct aer_capability_regs, header_log) +
-	       PCIE_STD_NUM_TLP_HEADERLOG * sizeof(u32));
-	memcpy(aer_regs.header_log.prefix,
-	       pcie_err->aer_info + PCI_ERR_PREFIX_LOG,
-	       sizeof(aer_regs.header_log.prefix));
+	aer_cap_regs_unpack(&aer_regs, pcie_err->aer_info, sizeof(pcie_err->aer_info));
 
 	domain = pcie_err->device_id.segment;
 	bus = pcie_err->device_id.bus;
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 08c985e729d6..ade1cc8c9f8d 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -668,20 +668,7 @@ static void ghes_handle_aer(struct acpi_hest_generic_data *gdata)
 		if (!aer_info)
 			return;
 
-		/*
-		 * Map aer_info onto the struct as extlog_print_pcie() does:
-		 * copy up to the four Header Log DWORDs, then place the TLP
-		 * Prefix Log from where the hardware keeps it. The rest stays
-		 * zero, so firmware cannot drive the pcie_print_tlp_log() loop
-		 * over dw[] out of bounds.
-		 */
-		memset(aer_info, 0, sizeof(struct aer_capability_regs));
-		memcpy(aer_info, pcie_err->aer_info,
-		       offsetof(struct aer_capability_regs, header_log) +
-		       PCIE_STD_NUM_TLP_HEADERLOG * sizeof(u32));
-		memcpy(aer_info->header_log.prefix,
-		       pcie_err->aer_info + PCI_ERR_PREFIX_LOG,
-		       sizeof(aer_info->header_log.prefix));
+		aer_cap_regs_unpack(aer_info, pcie_err->aer_info, sizeof(pcie_err->aer_info));
 
 		aer_recover_queue(pcie_err->device_id.segment,
 				  pcie_err->device_id.bus,
diff --git a/drivers/cxl/core/ras_rch.c b/drivers/cxl/core/ras_rch.c
index e0e01aa5eba6..4005344dda55 100644
--- a/drivers/cxl/core/ras_rch.c
+++ b/drivers/cxl/core/ras_rch.c
@@ -19,7 +19,7 @@ void cxl_dport_map_rch_aer(struct cxl_dport *dport)
 		aer_phys = aer_cap + dport->rcrb.base;
 		dport->regs.dport_aer =
 			devm_cxl_iomap_block(host, aer_phys,
-					     sizeof(struct aer_capability_regs));
+					     PCIE_AER_CAP_HW_SIZE);
 	}
 }
 
@@ -58,31 +58,17 @@ void cxl_disable_rch_root_ints(struct cxl_dport *dport)
 static bool cxl_rch_get_aer_info(void __iomem *aer_base,
 				 struct aer_capability_regs *aer_regs)
 {
-	/*
-	 * Bound the copy to the physically-defined AER registers (header
-	 * through the 16-byte Header Log). struct aer_capability_regs is a
-	 * software layout whose embedded struct pcie_tlp_log is larger than
-	 * the on-wire AER capability; copying sizeof(*aer_regs) would
-	 * over-read the RCRB-mapped MMIO block.
-	 */
-	int read_cnt = (PCI_ERR_HEADER_LOG + 16) / sizeof(u32);
-	u32 *aer_regs_buf = (u32 *)aer_regs;
-	int n;
+	/* A flat copy cannot fill the struct; aer_cap_regs_unpack() places it. */
+	u32 raw[PCIE_AER_CAP_HW_SIZE / sizeof(u32)];
 
 	if (!aer_base)
 		return false;
 
-	/*
-	 * Zero the destination so the software-only tail fields
-	 * (e.g. header_log.header_len) are deterministic rather than
-	 * left as uninitialized stack, which could drive a bogus loop
-	 * length in pcie_print_tlp_log().
-	 */
-	memset(aer_regs, 0, sizeof(*aer_regs));
-
 	/* Use readl() to guarantee 32-bit accesses */
-	for (n = 0; n < read_cnt; n++)
-		aer_regs_buf[n] = readl(aer_base + n * sizeof(u32));
+	for (int n = 0; n < ARRAY_SIZE(raw); n++)
+		raw[n] = readl(aer_base + n * sizeof(u32));
+
+	aer_cap_regs_unpack(aer_regs, raw, sizeof(raw));
 
 	writel(aer_regs->uncor_status, aer_base + PCI_ERR_UNCOR_STATUS);
 	writel(aer_regs->cor_status, aer_base + PCI_ERR_COR_STATUS);
diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
index 71f8fc9ea2ed..bba80c380358 100644
--- a/drivers/pci/pcie/tlp.c
+++ b/drivers/pci/pcie/tlp.c
@@ -8,6 +8,8 @@
 #include <linux/aer.h>
 #include <linux/array_size.h>
 #include <linux/bitfield.h>
+#include <linux/build_bug.h>
+#include <linux/minmax.h>
 #include <linux/pci.h>
 #include <linux/string.h>
 
@@ -92,6 +94,92 @@ int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
 	return 0;
 }
 
+/* Only the registers ahead of the Header Log map 1:1. */
+static_assert(offsetof(struct aer_capability_regs, header_log) ==
+	      PCI_ERR_HEADER_LOG);
+/* The Prefix Log registers hold dw[4..13] in Flit mode. */
+static_assert((PCI_ERR_PREFIX_LOG +
+	       (PCIE_STD_MAX_TLP_HEADERLOG - PCIE_STD_NUM_TLP_HEADERLOG) *
+	       sizeof(u32)) == PCIE_AER_CAP_HW_SIZE);
+
+/**
+ * aer_cap_regs_unpack - Convert a raw AER Capability image to the kernel layout
+ * @regs: Destination, fully initialised
+ * @raw: AER Capability register block in hardware order
+ * @raw_len: Bytes readable at @raw
+ *
+ * struct aer_capability_regs is not the hardware layout: struct pcie_tlp_log
+ * spans 60 bytes where the Header Log it stands in for is 16, so a flat copy
+ * misplaces everything behind it. Map the registers one by one instead,
+ * taking the TLP Log layout from the Flit bit as pcie_read_tlp_log() does.
+ *
+ * Registers past @raw_len are left zero, so a short image cannot be read
+ * past.
+ */
+void aer_cap_regs_unpack(struct aer_capability_regs *regs, const void *raw,
+			 size_t raw_len)
+{
+	unsigned int i, tlp_len;
+	bool flit;
+
+	memset(regs, 0, sizeof(*regs));
+
+	if (raw_len < PCI_ERR_HEADER_LOG)
+		return;
+	memcpy(regs, raw, PCI_ERR_HEADER_LOG);
+
+	flit = FIELD_GET(PCI_ERR_CAP_TLP_LOG_FLIT, regs->cap_control);
+	if (flit) {
+		tlp_len = FIELD_GET(PCI_ERR_CAP_TLP_LOG_SIZE, regs->cap_control);
+	} else {
+		/*
+		 * dw[4..7] alias the Prefix Log. Take all four whatever
+		 * eetlp_prefix_max says; pcie_print_tlp_log() stops at the
+		 * first zero one.
+		 */
+		tlp_len = PCIE_STD_NUM_TLP_HEADERLOG + PCIE_STD_MAX_TLP_PREFIXLOG;
+	}
+
+	tlp_len = min(tlp_len, ARRAY_SIZE(regs->header_log.dw));
+
+	for (i = 0; i < tlp_len; i++) {
+		unsigned int off;
+
+		if (i < PCIE_STD_NUM_TLP_HEADERLOG)
+			off = PCI_ERR_HEADER_LOG + i * sizeof(u32);
+		else
+			off = PCI_ERR_PREFIX_LOG +
+			      (i - PCIE_STD_NUM_TLP_HEADERLOG) * sizeof(u32);
+
+		if (off + sizeof(u32) > raw_len)
+			break;
+		memcpy(&regs->header_log.dw[i], raw + off, sizeof(u32));
+	}
+
+	/*
+	 * @i is what got filled. Non-Flit length needs the TLP parsed, so cap
+	 * it at 4 as the native path does.
+	 */
+	regs->header_log.header_len = flit ? i : min(i, PCIE_STD_NUM_TLP_HEADERLOG);
+	regs->header_log.flit = flit;
+
+	if (raw_len >= PCI_ERR_ROOT_COMMAND + sizeof(regs->root_command))
+		memcpy(&regs->root_command, raw + PCI_ERR_ROOT_COMMAND,
+		       sizeof(regs->root_command));
+	if (raw_len >= PCI_ERR_ROOT_STATUS + sizeof(regs->root_status))
+		memcpy(&regs->root_status, raw + PCI_ERR_ROOT_STATUS,
+		       sizeof(regs->root_status));
+	if (raw_len >= PCI_ERR_ROOT_ERR_SRC + sizeof(regs->cor_err_source) +
+		       sizeof(regs->uncor_err_source)) {
+		memcpy(&regs->cor_err_source, raw + PCI_ERR_ROOT_ERR_SRC,
+		       sizeof(regs->cor_err_source));
+		memcpy(&regs->uncor_err_source,
+		       raw + PCI_ERR_ROOT_ERR_SRC + sizeof(regs->cor_err_source),
+		       sizeof(regs->uncor_err_source));
+	}
+}
+EXPORT_SYMBOL_GPL(aer_cap_regs_unpack);
+
 #define EE_PREFIX_STR " E-E Prefixes:"
 
 /**
diff --git a/include/linux/aer.h b/include/linux/aer.h
index df0f5c382286..043ee76b6f53 100644
--- a/include/linux/aer.h
+++ b/include/linux/aer.h
@@ -24,6 +24,13 @@
 #define PCIE_STD_MAX_TLP_PREFIXLOG     4
 #define PCIE_STD_MAX_TLP_HEADERLOG	(PCIE_STD_NUM_TLP_HEADERLOG + 10)
 
+/*
+ * Size of the AER Capability register block in hardware, long enough for the
+ * longest Flit mode TLP Log. struct aer_capability_regs below is larger and
+ * laid out differently; use aer_cap_regs_unpack() to convert.
+ */
+#define PCIE_AER_CAP_HW_SIZE		96
+
 struct pci_dev;
 
 struct pcie_tlp_log {
@@ -68,6 +75,8 @@ static inline void pci_aer_unmask_internal_errors(struct pci_dev *dev) { }
 
 void pci_print_aer(struct pci_dev *dev, int aer_severity,
 		    struct aer_capability_regs *aer);
+void aer_cap_regs_unpack(struct aer_capability_regs *regs, const void *raw,
+			 size_t raw_len);
 int cper_severity_to_aer(int cper_severity);
 void aer_recover_queue(int domain, unsigned int bus, unsigned int devfn,
 		       int severity, struct aer_capability_regs *aer_regs);

base-commit: 86adf9d0f7b074e0a1310c1bc4db66cac5c83eec
-- 
2.54.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] PCI/AER: Map a raw AER Capability image field by field
  2026-09-09 16:31 [PATCH v2] PCI/AER: Map a raw AER Capability image field by field Dave Jiang
@ 2026-09-09 16:45 ` sashiko-bot
  2026-09-09 17:46   ` Dave Jiang
  0 siblings, 1 reply; 3+ messages in thread
From: sashiko-bot @ 2026-09-09 16:45 UTC (permalink / raw)
  To: Dave Jiang; +Cc: linux-cxl, linux-pci

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] aer_cap_regs_unpack() causes data corruption and mis-parsing of AER registers on big-endian architectures due to mixed endianness inputs and improper byte-offset access.

Pre-existing issues:
- [High] Memory allocated for `aer_info` from `ghes_estatus_pool` is permanently leaked when `aer_recover_queue()` fails to enqueue the error due to ring buffer overflow.
--

commit 5fdf5d4c5bbdafcab0279ec4f99b6ca67a1876af
Author: Dave Jiang <dave.jiang@intel.com>

PCI/AER: Map a raw AER Capability image field by field

This commit introduces aer_cap_regs_unpack() to map raw AER capability
registers field-by-field into the kernel's struct aer_capability_regs layout.
It updates ACPI GHES, ACPI extlog, and CXL RCH error handling to use this
unpacking routine instead of flat memory copies.

Link: https://lore.kernel.org/linux-cxl/CAJZ5v0hDv11cPuztPZsaDd7uwD_49KznJy=tzuRO+dZc=CnAEQ@mail.gmail.com/
Link: https://sashiko.dev/#/patchset/20260904172337.1409775-1-dave.jiang@intel.com?part=5

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 08c985e729d6c..ade1cc8c9f8d1 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
[ ... ]
> @@ -668,20 +668,7 @@ static void ghes_handle_aer(struct acpi_hest_generic_data *gdata)
>  		if (!aer_info)
>  			return;
>  
[ ... ]
> +		aer_cap_regs_unpack(aer_info, pcie_err->aer_info, sizeof(pcie_err->aer_info));

[Severity: High]
Will this corrupt the AER registers on big-endian architectures?

The pcie_err->aer_info buffer contains a strictly little-endian CPER payload.
However, aer_cap_regs_unpack() receives this raw byte stream and populates
native u32 fields with it, which can lead to mis-parsing.

>  
>  		aer_recover_queue(pcie_err->device_id.segment,
>  				  pcie_err->device_id.bus,

[Severity: High]
This is a pre-existing issue, but does aer_recover_queue() permanently leak
the aer_info allocation if the ring buffer overflows?

The aer_info is allocated from the fixed-size ghes_estatus_pool via
gen_pool_alloc() earlier in this function. When passed to
aer_recover_queue(), memory ownership is dropped without freeing it if
the queue operation fails:

drivers/pci/pcie/aer.c:aer_recover_queue() {
    if (kfifo_in_spinlocked(&aer_recover_ring, &entry, 1,
                            &aer_recover_ring_lock))
        schedule_work(&aer_recover_work);
    else
        pr_err("buffer overflow in recovery...");
        /* aer_regs pointer dropped here */
}

Since this pool is small and fixed-size, could an error storm that overflows
the queue permanently deplete the pool and disable further ACPI GHES error
reporting?

> diff --git a/drivers/cxl/core/ras_rch.c b/drivers/cxl/core/ras_rch.c
> index e0e01aa5eba6c..4005344dda55f 100644
> --- a/drivers/cxl/core/ras_rch.c
> +++ b/drivers/cxl/core/ras_rch.c
[ ... ]
> @@ -58,31 +58,17 @@ void cxl_disable_rch_root_ints(struct cxl_dport *dport)
>  static bool cxl_rch_get_aer_info(void __iomem *aer_base,
>  				 struct aer_capability_regs *aer_regs)
>  {
[ ... ]
> -	/* Use readl() to guarantee 32-bit accesses */
> -	for (n = 0; n < read_cnt; n++)
> -		aer_regs_buf[n] = readl(aer_base + n * sizeof(u32));
> +	for (int n = 0; n < ARRAY_SIZE(raw); n++)
> +		raw[n] = readl(aer_base + n * sizeof(u32));
> +
> +	aer_cap_regs_unpack(aer_regs, raw, sizeof(raw));

[Severity: High]
Could populating the raw array using readl() cause data corruption on
big-endian systems?

By using readl(), the raw array is filled with host-endian values. When
aer_cap_regs_unpack() later extracts byte-offsets via memcpy(), this will
read from the wrong half of the u32 on big-endian architectures, swapping
fields like the correctable and uncorrectable error sources.

> diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
> index 71f8fc9ea2edc..bba80c3803587 100644
> --- a/drivers/pci/pcie/tlp.c
> +++ b/drivers/pci/pcie/tlp.c
[ ... ]
> +void aer_cap_regs_unpack(struct aer_capability_regs *regs, const void *raw,
> +			 size_t raw_len)
> +{
[ ... ]
> +	if (raw_len < PCI_ERR_HEADER_LOG)
> +		return;
> +	memcpy(regs, raw, PCI_ERR_HEADER_LOG);
> +
> +	flit = FIELD_GET(PCI_ERR_CAP_TLP_LOG_FLIT, regs->cap_control);

[Severity: High]
Will this extract the wrong bits on big-endian systems when processing
little-endian CPER streams?

If raw comes from ACPI GHES (where it is little-endian), the memcpy() just
above populates regs->cap_control with little-endian bytes. On a big-endian
system, this results in a byte-swapped integer, causing FIELD_GET() to read
incorrect bits for the FLIT status and TLP log size.

[ ... ]
> +	if (raw_len >= PCI_ERR_ROOT_ERR_SRC + sizeof(regs->cor_err_source) +
> +		       sizeof(regs->uncor_err_source)) {
> +		memcpy(&regs->cor_err_source, raw + PCI_ERR_ROOT_ERR_SRC,
> +		       sizeof(regs->cor_err_source));

[Severity: High]
Does this byte-level copy extract the wrong half of the register on
big-endian systems?

When raw is populated as a host-endian u32 array (as is done in
cxl_rch_get_aer_info), copying 2 bytes from the PCI_ERR_ROOT_ERR_SRC offset
will extract the upper 16 bits instead of the lower 16 bits on big-endian
machines. This swaps the correctable and uncorrectable error source fields.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909163140.2723395-1-dave.jiang@intel.com?part=1

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] PCI/AER: Map a raw AER Capability image field by field
  2026-09-09 16:45 ` sashiko-bot
@ 2026-09-09 17:46   ` Dave Jiang
  0 siblings, 0 replies; 3+ messages in thread
From: Dave Jiang @ 2026-09-09 17:46 UTC (permalink / raw)
  To: sashiko-reviews; +Cc: linux-cxl, linux-pci



On 9/9/26 9:45 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] aer_cap_regs_unpack() causes data corruption and mis-parsing of AER registers on big-endian architectures due to mixed endianness inputs and improper byte-offset access.

Will fix in v3.

DJ

> 
> Pre-existing issues:
> - [High] Memory allocated for `aer_info` from `ghes_estatus_pool` is permanently leaked when `aer_recover_queue()` fails to enqueue the error due to ring buffer overflow.
> --
> 
> commit 5fdf5d4c5bbdafcab0279ec4f99b6ca67a1876af
> Author: Dave Jiang <dave.jiang@intel.com>
> 
> PCI/AER: Map a raw AER Capability image field by field
> 
> This commit introduces aer_cap_regs_unpack() to map raw AER capability
> registers field-by-field into the kernel's struct aer_capability_regs layout.
> It updates ACPI GHES, ACPI extlog, and CXL RCH error handling to use this
> unpacking routine instead of flat memory copies.
> 
> Link: https://lore.kernel.org/linux-cxl/CAJZ5v0hDv11cPuztPZsaDd7uwD_49KznJy=tzuRO+dZc=CnAEQ@mail.gmail.com/
> Link: https://sashiko.dev/#/patchset/20260904172337.1409775-1-dave.jiang@intel.com?part=5
> 
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 08c985e729d6c..ade1cc8c9f8d1 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
> [ ... ]
>> @@ -668,20 +668,7 @@ static void ghes_handle_aer(struct acpi_hest_generic_data *gdata)
>>  		if (!aer_info)
>>  			return;
>>  
> [ ... ]
>> +		aer_cap_regs_unpack(aer_info, pcie_err->aer_info, sizeof(pcie_err->aer_info));
> 
> [Severity: High]
> Will this corrupt the AER registers on big-endian architectures?
> 
> The pcie_err->aer_info buffer contains a strictly little-endian CPER payload.
> However, aer_cap_regs_unpack() receives this raw byte stream and populates
> native u32 fields with it, which can lead to mis-parsing.
> 
>>  
>>  		aer_recover_queue(pcie_err->device_id.segment,
>>  				  pcie_err->device_id.bus,
> 
> [Severity: High]
> This is a pre-existing issue, but does aer_recover_queue() permanently leak
> the aer_info allocation if the ring buffer overflows?
> 
> The aer_info is allocated from the fixed-size ghes_estatus_pool via
> gen_pool_alloc() earlier in this function. When passed to
> aer_recover_queue(), memory ownership is dropped without freeing it if
> the queue operation fails:
> 
> drivers/pci/pcie/aer.c:aer_recover_queue() {
>     if (kfifo_in_spinlocked(&aer_recover_ring, &entry, 1,
>                             &aer_recover_ring_lock))
>         schedule_work(&aer_recover_work);
>     else
>         pr_err("buffer overflow in recovery...");
>         /* aer_regs pointer dropped here */
> }
> 
> Since this pool is small and fixed-size, could an error storm that overflows
> the queue permanently deplete the pool and disable further ACPI GHES error
> reporting?
> 
>> diff --git a/drivers/cxl/core/ras_rch.c b/drivers/cxl/core/ras_rch.c
>> index e0e01aa5eba6c..4005344dda55f 100644
>> --- a/drivers/cxl/core/ras_rch.c
>> +++ b/drivers/cxl/core/ras_rch.c
> [ ... ]
>> @@ -58,31 +58,17 @@ void cxl_disable_rch_root_ints(struct cxl_dport *dport)
>>  static bool cxl_rch_get_aer_info(void __iomem *aer_base,
>>  				 struct aer_capability_regs *aer_regs)
>>  {
> [ ... ]
>> -	/* Use readl() to guarantee 32-bit accesses */
>> -	for (n = 0; n < read_cnt; n++)
>> -		aer_regs_buf[n] = readl(aer_base + n * sizeof(u32));
>> +	for (int n = 0; n < ARRAY_SIZE(raw); n++)
>> +		raw[n] = readl(aer_base + n * sizeof(u32));
>> +
>> +	aer_cap_regs_unpack(aer_regs, raw, sizeof(raw));
> 
> [Severity: High]
> Could populating the raw array using readl() cause data corruption on
> big-endian systems?
> 
> By using readl(), the raw array is filled with host-endian values. When
> aer_cap_regs_unpack() later extracts byte-offsets via memcpy(), this will
> read from the wrong half of the u32 on big-endian architectures, swapping
> fields like the correctable and uncorrectable error sources.
> 
>> diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
>> index 71f8fc9ea2edc..bba80c3803587 100644
>> --- a/drivers/pci/pcie/tlp.c
>> +++ b/drivers/pci/pcie/tlp.c
> [ ... ]
>> +void aer_cap_regs_unpack(struct aer_capability_regs *regs, const void *raw,
>> +			 size_t raw_len)
>> +{
> [ ... ]
>> +	if (raw_len < PCI_ERR_HEADER_LOG)
>> +		return;
>> +	memcpy(regs, raw, PCI_ERR_HEADER_LOG);
>> +
>> +	flit = FIELD_GET(PCI_ERR_CAP_TLP_LOG_FLIT, regs->cap_control);
> 
> [Severity: High]
> Will this extract the wrong bits on big-endian systems when processing
> little-endian CPER streams?
> 
> If raw comes from ACPI GHES (where it is little-endian), the memcpy() just
> above populates regs->cap_control with little-endian bytes. On a big-endian
> system, this results in a byte-swapped integer, causing FIELD_GET() to read
> incorrect bits for the FLIT status and TLP log size.
> 
> [ ... ]
>> +	if (raw_len >= PCI_ERR_ROOT_ERR_SRC + sizeof(regs->cor_err_source) +
>> +		       sizeof(regs->uncor_err_source)) {
>> +		memcpy(&regs->cor_err_source, raw + PCI_ERR_ROOT_ERR_SRC,
>> +		       sizeof(regs->cor_err_source));
> 
> [Severity: High]
> Does this byte-level copy extract the wrong half of the register on
> big-endian systems?
> 
> When raw is populated as a host-endian u32 array (as is done in
> cxl_rch_get_aer_info), copying 2 bytes from the PCI_ERR_ROOT_ERR_SRC offset
> will extract the upper 16 bits instead of the lower 16 bits on big-endian
> machines. This swaps the correctable and uncorrectable error source fields.
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-09-09 17:46 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-09 16:31 [PATCH v2] PCI/AER: Map a raw AER Capability image field by field Dave Jiang
2026-09-09 16:45 ` sashiko-bot
2026-09-09 17:46   ` Dave Jiang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox