[PATCH v5 00/13] ACPI: APEI: GHES: Collection of fixes for issues reported by sashiko

[PATCH v5 00/13] ACPI: APEI: GHES: Collection of fixes for issues reported by sashiko

[Dave Jiang]

Fixes for pre-existing issues sashiko-bot found while reviewing patches in the
CPER, extlog and GHES paths. v1 through v3 fixed successive batches as the
review widened; see the links below.

The series is grouped in three parts, plus a cleanup.

Bound the record before anything walks it:

1/13:  Reject CPER records with an out-of-range error_data_length.
2/13:  Reject an error status block length that wraps a u32.
3/13:  Validate the extlog record length before walking sections.

Fix the extlog error paths, then enable them:

4/13:  Defer CXL protocol error handling to avoid a lock inversion.
5/13:  Avoid populating software AER metadata from the raw hardware buffer.
6/13:  Validate the PCIe error section length before payload access.
7/13:  Fix the CONFIG_ACPI_APEI_PCIEAER guard typo in extlog.c.

Bound each section payload before its consumers read it:

8/13:  Bound the CXL event record copy to the firmware section length.
9/13:  Validate the CXL protocol error section length before the RAS cap copy.
10/13: Read only validated fields in cper_mem_err_pack().
11/13: Validate the memory error section length before payload access.
12/13: Bound the AER info copy and sanitize software metadata in ghes.c.

Then drop an export patch 4 made redundant:

13/13: Make cxl_cper_handle_prot_err() static.

Patches 1, 2 and 10 touch drivers/firmware/efi/cper.c, closing the holes at
the shared choke point the rest of the series relies on. Patch 2 also fixes an
infinite loop in bert_print_all(), unrelated to this series but the same root
cause.

Known gaps, left for separate patches:

  - cxl_cper_print_prot_err() in drivers/firmware/efi/cper_cxl.c uses
    dvsec_len without bounding it against the section length.
  - cxl_rch_get_aer_info() in drivers/cxl/core/ras_rch.c reads the RCH AER
    capability from MMIO without clearing header_len/flit, the same class as
    patches 5 and 12.
  - struct pcie_tlp_log grew to 60 bytes for Flit mode, so the 96-byte CPER
    AER info no longer maps 1:1 onto struct aer_capability_regs past the
    Header Log. Patches 5 and 12 now copy only the 44 bytes that do map and
    leave the rest zero, which keeps the Root Error registers out of
    header_log.prefix[] - but nothing here decodes the Flit-mode header
    DWORDs, which live at payload offset 56 while the struct expects them at
    44. Doing that properly wants a field-by-field mapping shared with
    cxl_rch_get_aer_info(), plus a clamp: pcie_print_tlp_log() trusts
    header_len against a 14-entry dw[], and PCI_ERR_CAP_TLP_LOG_SIZE is five
    bits wide.

v1: https://lore.kernel.org/linux-cxl/20260709162807.1957783-1-dave.jiang@intel.com/
v2: https://lore.kernel.org/linux-cxl/20260714231835.303081-1-dave.jiang@intel.com/
v3: https://lore.kernel.org/linux-cxl/20260717161647.1493259-1-dave.jiang@intel.com/
v4: https://lore.kernel.org/linux-cxl/20260824174936.939059-1-dave.jiang@intel.com/

Review tags from the v3 and v4 threads are carried over, and Jonathan reviewed
patches 2, 4, 6, 7, 8 and 9 on v4. Patches 1, 10, 11 and 13 carry none: 10 and
13 are new since v3, and 1 and 11 were reworked far enough that Alison's and
Shuai's v3 tags no longer applied. Patches 5 and 12 keep theirs, but the copy
length changed in v5 - same intent, different bound - so please re-check if you
disagree.

Changes since v4
----------------
- Patch 1: use check_add_overflow() instead of a u64 sum plus an INT_MAX
  test, and drop the now-redundant acpi_hest_get_size() bound, since
  record_size is never smaller than the header (Jonathan Cameron).
- Patches 5 and 12: copy only the 44 bytes of aer_info that map onto struct
  aer_capability_regs - the leading registers and the four Header Log DWORDs
  - and leave the rest zero, instead of copying all 96 bytes and then
  clearing header_len and flit (Jonathan Cameron). This also keeps the Root
  Error Command, Root Error Status and Error Source ID out of
  header_log.prefix[], where pcie_print_tlp_log() was printing them as
  end-to-end prefixes.
- Patch 3: kept the length bound ahead of cper_estatus_check() and expanded
  the comment to say why. Swapping them would let cper_estatus_check() walk
  sections over an unbounded data_length, past the end of elog_buf
  (Jonathan Cameron).

Dave Jiang (13):
  efi/cper: Reject CPER records with an out-of-range error_data_length
  efi/cper: Reject an error status block length that wraps a u32
  ACPI: extlog: Validate elog record length before walking sections
  ACPI: extlog: Defer CXL protocol error handling to avoid lock
    inversion
  ACPI: extlog: Avoid populating software AER metadata from raw hardware
    buffer
  ACPI: extlog: Validate PCIe error section length before payload access
  ACPI: extlog: Fix CONFIG_ACPI_APEI_PCIEAER guard typo
  ACPI: APEI: GHES: Bound CXL event record copy to the firmware section
    length
  ACPI: APEI: GHES: Validate CXL protocol error section length before
    RAS cap copy
  efi/cper: Read only validated fields in cper_mem_err_pack()
  ACPI: APEI: GHES: Validate memory error section length before payload
    access
  ACPI: APEI: GHES: Bound AER info copy and sanitize software metadata
  cxl/ras: Make cxl_cper_handle_prot_err() static

 drivers/acpi/acpi_extlog.c       | 60 +++++++++++++---------
 drivers/acpi/apei/ghes.c         | 88 +++++++++++++++++++++++++++-----
 drivers/acpi/apei/ghes_helpers.c | 18 ++++++-
 drivers/cxl/core/ras.c           |  3 +-
 drivers/firmware/efi/cper.c      | 51 +++++++++++++++---
 include/acpi/ghes.h              |  4 ++
 include/cxl/event.h              |  6 +--
 7 files changed, 178 insertions(+), 52 deletions(-)


base-commit: 075b74841bd0065a3bda3440873c747938e69b68
-- 
2.54.0

[PATCH v5 01/13] efi/cper: Reject CPER records with an out-of-range error_data_length

[Dave Jiang]

cper_estatus_check() sizes each section with acpi_hest_get_record_size(),
which adds the firmware-controlled u32 error_data_length to the header size
as a signed int (see <acpi/ghes.h>). A value in the top sizeof(*gdata)
bytes of the u32 range wraps the sum small rather than large, so it slips
past the "record_size > data_len" check: against the 72-byte v300 header,
0xffffffb9 sizes the record at 1 and acpi_hest_get_next() walks it a byte
at a time, off the end. 0xffffffb8 sizes it at 0 and loops forever.

Use check_add_overflow() to reject a sum that will not fit the int those
helpers return, since the walk advances by that value. That subsumes the
"acpi_hest_get_size(gdata) > data_len" test above it, because record_size
is never smaller than the header.

The later "len < sizeof(*foo)" guards rely on this per-section upper bound;
they are lower bounds only.

Reported-by: sashiko-bot@kernel.org
Closes: https://sashiko.dev/#/patchset/20260714231835.303081-1-dave.jiang@intel.com?part=1
Fixes: 45b14a4ffcc1 ("efi: cper: Fix possible out-of-bounds access")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v5:
- Use check_add_overflow() rather than a u64 sum plus an INT_MAX test, and
  drop the now-redundant acpi_hest_get_size() bound (Jonathan Cameron).
---
 drivers/firmware/efi/cper.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index 06b4fdb59917..332c78f2d561 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -752,7 +752,7 @@ EXPORT_SYMBOL_GPL(cper_estatus_check_header);
 int cper_estatus_check(const struct acpi_hest_generic_status *estatus)
 {
 	struct acpi_hest_generic_data *gdata;
-	unsigned int data_len, record_size;
+	unsigned int data_len;
 	int rc;
 
 	rc = cper_estatus_check_header(estatus);
@@ -762,10 +762,18 @@ int cper_estatus_check(const struct acpi_hest_generic_status *estatus)
 	data_len = estatus->data_length;
 
 	apei_estatus_for_each_section(estatus, gdata) {
-		if (acpi_hest_get_size(gdata) > data_len)
+		int record_size;
+
+		/*
+		 * The <acpi/ghes.h> helpers sum these as a signed int, so a
+		 * huge error_data_length wraps small rather than large and the
+		 * walk then advances by that wrapped value. Reject a size an
+		 * int cannot carry.
+		 */
+		if (check_add_overflow(acpi_hest_get_size(gdata),
+				       gdata->error_data_length, &record_size))
 			return -EINVAL;
 
-		record_size = acpi_hest_get_record_size(gdata);
 		if (record_size > data_len)
 			return -EINVAL;
 
-- 
2.54.0

Re: [PATCH v5 01/13] efi/cper: Reject CPER records with an out-of-range error_data_length

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing lower bounds check for the section header in `cper_estatus_check()` allows out-of-bounds reads when iterating truncated CPER records.
--

commit 2032c31f790eb3dad96643cacd189ae773ea581e
Author: Dave Jiang <dave.jiang@intel.com>
efi/cper: Reject CPER records with an out-of-range error_data_length

This commit modifies the size validation in cper_estatus_check to use
check_add_overflow. It addresses an issue where a large error_data_length
could wrap around as a signed int, bypassing the original size checks and
causing an infinite loop or out-of-bounds access.

Closes: https://sashiko.dev/#/patchset/20260714231835.303081-1-dave.jiang@intel.com?part=1
Fixes: 45b14a4ffcc1 ("efi: cper: Fix possible out-of-bounds access")

> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 06b4fdb59917a..332c78f2d5617 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c

[ ... ]

> @@ -762,10 +762,18 @@ int cper_estatus_check(const struct acpi_hest_generic_status *estatus)
>  	data_len = estatus->data_length;
>  
>  	apei_estatus_for_each_section(estatus, gdata) {
> -		if (acpi_hest_get_size(gdata) > data_len)
> +		int record_size;
> +
> +		/*
> +		 * The <acpi/ghes.h> helpers sum these as a signed int, so a
> +		 * huge error_data_length wraps small rather than large and the
> +		 * walk then advances by that wrapped value. Reject a size an
> +		 * int cannot carry.
> +		 */
> +		if (check_add_overflow(acpi_hest_get_size(gdata),
> +				       gdata->error_data_length, &record_size))
>  			return -EINVAL;

[Severity: High]
This is a pre-existing issue, but does this code risk an out-of-bounds read
if the CPER record is truncated?

The apei_estatus_for_each_section loop condition only checks if the start of
the section is within estatus->data_length.

If a truncated record leaves fewer than sizeof(struct acpi_hest_generic_data)
bytes for the final section, the loop will still iterate.

Inside the loop, acpi_hest_get_size(gdata) reads gdata->revision, and
check_add_overflow reads gdata->error_data_length before confirming if
data_len actually has enough space for the struct header itself.

Could this result in page faults or memory corruption if malformed firmware
leaves us with a truncated section header?

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

[PATCH v5 02/13] efi/cper: Reject an error status block length that wraps a u32

[Dave Jiang]

cper_estatus_len() sums the firmware-controlled data_length (or
raw_data_offset plus raw_data_length) into a u32. A data_length of
0xffffffec wraps that sum to 0, and a length that reads back short defeats
every bound built on it: bert_print_all() passes its "remain <
estatus_len" check, then advances "estatus += estatus_len" by zero and
loops forever. GHES survives only because __ghes_check_estatus() rejects a
length below sizeof(*estatus) first.

Reject a length that cannot be expressed in a u32 in
cper_estatus_check_header(), which both of today's callers reach: GHES via
__ghes_check_estatus() and BERT via cper_estatus_check(). extlog reaches
neither yet; a later patch routes it through cper_estatus_check(), whose
ELOG_ENTRY_LEN bound needs this to hold.

Reported-by: sashiko-bot@kernel.org
Closes: https://sashiko.dev/#/patchset/20260714231835.303081-1-dave.jiang@intel.com?part=1
Fixes: 06d65deade9a ("ACPI, APEI, UEFI Common Platform Error Record (CPER) header")
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/firmware/efi/cper.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index 332c78f2d561..3dcef618ad39 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -745,6 +745,17 @@ int cper_estatus_check_header(const struct acpi_hest_generic_status *estatus)
 	    estatus->raw_data_offset < sizeof(*estatus) + estatus->data_length)
 		return -EINVAL;
 
+	/*
+	 * cper_estatus_len() sums these into a u32, and a wrapped sum reads
+	 * back smaller than the record. Reject a length that cannot be
+	 * expressed so no caller is handed the short value.
+	 */
+	if ((u64)sizeof(*estatus) + estatus->data_length > U32_MAX)
+		return -EINVAL;
+	if (estatus->raw_data_length &&
+	    (u64)estatus->raw_data_offset + estatus->raw_data_length > U32_MAX)
+		return -EINVAL;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(cper_estatus_check_header);
-- 
2.54.0

Re: [PATCH v5 02/13] efi/cper: Reject an error status block length that wraps a u32

[sashiko-bot]

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

Pre-existing issues:
- [High] Double-fetch (TOCTOU) vulnerability in GHES error record parsing leading to out-of-bounds reads in the kernel heap.
--

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

efi/cper: Reject an error status block length that wraps a u32

This commit adds bounds checking to cper_estatus_check_header() to prevent
a u32 wrap-around when calculating the total length of the error status
block. It rejects lengths that cannot be expressed in a u32 to protect
callers like GHES and BERT from processing short values and looping forever.

Closes: https://sashiko.dev/#/patchset/20260714231835.303081-1-dave.jiang@intel.com?part=1
Fixes: 06d65deade9a ("ACPI, APEI, UEFI Common Platform Error Record (CPER) header")

> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 332c78f2d5617..3dcef618ad39f 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -745,6 +745,17 @@ int cper_estatus_check_header(const struct acpi_hest_generic_status *estatus)
>  	    estatus->raw_data_offset < sizeof(*estatus) + estatus->data_length)
>  		return -EINVAL;
>  
> +	/*
> +	 * cper_estatus_len() sums these into a u32, and a wrapped sum reads
> +	 * back smaller than the record. Reject a length that cannot be
> +	 * expressed so no caller is handed the short value.
> +	 */
> +	if ((u64)sizeof(*estatus) + estatus->data_length > U32_MAX)
> +		return -EINVAL;
> +	if (estatus->raw_data_length &&
> +	    (u64)estatus->raw_data_offset + estatus->raw_data_length > U32_MAX)
> +		return -EINVAL;
> +

[Severity: High]
This isn't a bug introduced by this patch, but there is a pre-existing 
double-fetch (TOCTOU) vulnerability that these new checks do not mitigate.

The GHES driver fetches the error status header from shared physical memory
twice. The first fetch in ghes_in_nmi_queue_one_entry() determines the
buffer allocation size:

drivers/acpi/apei/ghes.c:ghes_in_nmi_queue_one_entry() {
        ...
        rc = __ghes_peek_estatus(ghes, &tmp_header, &buf_paddr, fixmap_idx);
        ...
        len = cper_estatus_len(&tmp_header);

The second fetch copies exactly len bytes from shared memory, overwriting
the previously validated header:

        if (__ghes_read_estatus(estatus, buf_paddr, fixmap_idx, len)) {
        ...
}

If the data_length field in physical memory is maliciously or erroneously
increased between these two reads, the newly copied header will contain a
data_length that exceeds the originally allocated buffer size (len).

The new untrusted data_length is then passed to cper_estatus_check():

drivers/firmware/efi/cper.c:cper_estatus_check() {
        ...
        data_len = estatus->data_length;
        apei_estatus_for_each_section(estatus, gdata) {
        ...
}

Because the checks here in cper_estatus_check_header() do not verify if
the new data_length exceeds the actual copied buffer size (len), the loop
in cper_estatus_check() will read out of bounds into the kernel heap.

This can lead to an out-of-bounds memory disclosure to dmesg, or a kernel
panic if it crosses into unmapped memory during NMI or IRQ context.

Does it make sense to update the caller to pass the allocated buffer size
into cper_estatus_check_header() so we can ensure the newly fetched length
does not exceed the buffer?

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

[PATCH v5 03/13] ACPI: extlog: Validate elog record length before walking sections

[Dave Jiang]

extlog_print() copies a fixed ELOG_ENTRY_LEN (4096) bytes from the elog
record into elog_buf, then walks the sections using the firmware-controlled
data_length. Nothing keeps data_length inside the buffer, so a malformed
record walks the section pointer past elog_buf and reads adjacent memory.
Unlike the GHES paths, extlog never calls cper_estatus_check().

Reject a record longer than ELOG_ENTRY_LEN and run cper_estatus_check()
before walking the sections. The length test alone is not enough: a wrapped
length reads back short and passes it, which cper_estatus_check() catches
via the header check added earlier. Drop a malformed record with
NOTIFY_DONE and without MCE_HANDLED_EXTLOG, since extlog did not consume
it.

Reported-by: sashiko-bot@kernel.org
Closes: https://sashiko.dev/#/patchset/20260714231835.303081-1-dave.jiang@intel.com?part=6
Fixes: f6ec01da40e4 ("ACPI: extlog: Handle multiple records")
Reviewed-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Shuai Xue <xueshuai@linux.alibaba.com>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v5:
- Expanded the comment to say why the length bound runs before
  cper_estatus_check() rather than after (Jonathan Cameron).
---
 drivers/acpi/acpi_extlog.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index 7ad3b36013cc..046e92ee195f 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -208,6 +208,15 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
 
 	tmp = (struct acpi_hest_generic_status *)elog_buf;
 
+	/*
+	 * Bound the length before cper_estatus_check() walks the sections: it
+	 * iterates over data_length, which is not yet known to fit elog_buf.
+	 * cper_estatus_check_header() then rejects a length that wrapped, which
+	 * the bound cannot see.
+	 */
+	if (cper_estatus_len(tmp) > ELOG_ENTRY_LEN || cper_estatus_check(tmp))
+		return NOTIFY_DONE;
+
 	if (!ras_userspace_consumers()) {
 		print_extlog_rcd(NULL, tmp, cpu);
 		goto out;
-- 
2.54.0

Re: [PATCH v5 03/13] ACPI: extlog: Validate elog record length before walking sections

[sashiko-bot]

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

Pre-existing issues:
- [High] The `cper_estatus_check()` function performs out-of-bounds memory reads when validating a malformed record with truncated section headers.
- [High] The firmware-provided physical address for the error log entry is not bounds-checked before being mapped to a virtual pointer, leading to arbitrary memory read and write.
--

commit 7b87e88b60eed844f533528aa716b4ef4d19ea04
Author: Dave Jiang <dave.jiang@intel.com>
Subject: ACPI: extlog: Validate elog record length before walking sections

This commit adds bounds checking to extlog_print() to ensure the firmware
provided data length does not exceed ELOG_ENTRY_LEN before walking the
sections. It introduces a call to cper_estatus_check() to perform additional
header and section validation.

Closes: https://sashiko.dev/#/patchset/20260714231835.303081-1-dave.jiang@intel.com?part=6

> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> index 7ad3b36013cc6..046e92ee195f9 100644
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c
> @@ -208,6 +208,15 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
>  
>  	tmp = (struct acpi_hest_generic_status *)elog_buf;
>  

[Severity: High]
This is a pre-existing issue, but does the lack of bounds checking on the
firmware-provided physical address in extlog_elog_entry_check() allow for
an arbitrary memory read and write here?

When extlog_elog_entry_check() retrieves the physical address, it converts it
to a virtual address without verifying that it falls within the mapped
window. If a buggy or malicious firmware supplies an out-of-bounds address,
the resulting virtual pointer will point outside the intended
acpi_os_map_iomem region.

The kernel then dereferences this unvalidated pointer in extlog_print() right
above this patch's context:

extlog_print() {
    ...
    memcpy(elog_buf, (void *)estatus, ELOG_ENTRY_LEN);
    estatus->block_status = 0;
    ...
}

Could this lead to a fatal page fault or the leakage and corruption of
arbitrary kernel memory?

> +	/*
> +	 * Bound the length before cper_estatus_check() walks the sections: it
> +	 * iterates over data_length, which is not yet known to fit elog_buf.
> +	 * cper_estatus_check_header() then rejects a length that wrapped, which
> +	 * the bound cannot see.
> +	 */
> +	if (cper_estatus_len(tmp) > ELOG_ENTRY_LEN || cper_estatus_check(tmp))

[Severity: High]
This isn't a bug introduced by this patch, but does calling
cper_estatus_check() here perform out-of-bounds memory reads when validating
a malformed record with truncated section headers?

In cper_estatus_check(), the apei_estatus_for_each_section() macro iterates as
long as the current offset is strictly less than estatus->data_length. If a
malformed record leaves a very small remainder (e.g., 1 byte) in data_length,
the loop still enters for another iteration.

Inside the loop, acpi_hest_get_size(gdata) is evaluated, which dereferences
gdata->revision (at offset 20) and reads gdata->error_data_length:

cper_estatus_check() {
    ...
    apei_estatus_for_each_section(estatus, gdata) {
        if (check_add_overflow(acpi_hest_get_size(gdata),
                               gdata->error_data_length, &record_size))
            return -EINVAL;
    ...
}

Because the remaining valid data length is smaller than the size of the
generic data header itself, these reads access memory well past the end of
the estatus record.

If the record aligns with the end of a heap allocation (such as the 4096-byte
elog_buf used here), reading past the boundary can trigger a page fault if the
adjacent page is unmapped.

> +		return NOTIFY_DONE;
> +
>  	if (!ras_userspace_consumers()) {
>  		print_extlog_rcd(NULL, tmp, cpu);
>  		goto out;

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

[PATCH v5 04/13] ACPI: extlog: Defer CXL protocol error handling to avoid lock inversion

[Dave Jiang]

extlog_print() calls cxl_cper_handle_prot_err() synchronously while the
MCE notifier chain rwsem is held, and that path takes the PCI device_lock
via guard(device)(). The probe path takes the two in the opposite order,
holding device_lock while mce_register_decode_chain() takes the rwsem, so
they can deadlock AB-BA.

ghes.c already avoids this by posting protocol errors to a kfifo and
handling them from a workqueue via cxl_cper_post_prot_err(). Export that
function and call it instead.

Declare it with the other CONFIG_ACPI_APEI_GHES exports rather than at the
end of the header. No #else stub: ACPI_EXTLOG selects ACPI_APEI_GHES, so
the only caller cannot exist without it.

Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/linux-cxl/20260709165457.8BA181F000E9@smtp.kernel.org/
Reviewed-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Shuai Xue <xueshuai@linux.alibaba.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/acpi_extlog.c | 21 ++-------------------
 drivers/acpi/apei/ghes.c   |  5 +++--
 include/acpi/ghes.h        |  4 ++++
 3 files changed, 9 insertions(+), 21 deletions(-)

diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index 046e92ee195f..2451362e696d 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -163,23 +163,6 @@ static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
 #endif
 }
 
-static void
-extlog_cxl_cper_handle_prot_err(struct cxl_cper_sec_prot_err *prot_err,
-				int severity)
-{
-#ifdef ACPI_APEI_PCIEAER
-	struct cxl_cper_prot_err_work_data wd;
-
-	if (cxl_cper_sec_prot_err_valid(prot_err))
-		return;
-
-	if (cxl_cper_setup_prot_err_work_data(&wd, prot_err, severity))
-		return;
-
-	cxl_cper_handle_prot_err(&wd);
-#endif
-}
-
 static int extlog_print(struct notifier_block *nb, unsigned long val,
 			void *data)
 {
@@ -244,8 +227,8 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
 			struct cxl_cper_sec_prot_err *prot_err =
 				acpi_hest_get_payload(gdata);
 
-			extlog_cxl_cper_handle_prot_err(prot_err,
-							gdata->error_severity);
+			cxl_cper_post_prot_err(prot_err,
+					       gdata->error_severity);
 		} else if (guid_equal(sec_type, &CPER_SEC_PCIE)) {
 			struct cper_sec_pcie *pcie_err = acpi_hest_get_payload(gdata);
 
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 3236a3ce79d6..a382aabf9835 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -752,8 +752,8 @@ static DEFINE_KFIFO(cxl_cper_prot_err_fifo, struct cxl_cper_prot_err_work_data,
 static DEFINE_SPINLOCK(cxl_cper_prot_err_work_lock);
 struct work_struct *cxl_cper_prot_err_work;
 
-static void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
-				   int severity)
+void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
+			    int severity)
 {
 #ifdef CONFIG_ACPI_APEI_PCIEAER
 	struct cxl_cper_prot_err_work_data wd;
@@ -777,6 +777,7 @@ static void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
 	schedule_work(cxl_cper_prot_err_work);
 #endif
 }
+EXPORT_SYMBOL_FOR_MODULES(cxl_cper_post_prot_err, "acpi_extlog");
 
 int cxl_cper_register_prot_err_work(struct work_struct *work)
 {
diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
index 8d7e5caef3f1..be496bc0386f 100644
--- a/include/acpi/ghes.h
+++ b/include/acpi/ghes.h
@@ -85,6 +85,10 @@ int devm_ghes_register_vendor_record_notifier(struct device *dev,
 struct list_head *ghes_get_devices(void);
 
 void ghes_estatus_pool_region_free(unsigned long addr, u32 size);
+
+struct cxl_cper_sec_prot_err;
+void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
+			    int severity);
 #else
 static inline struct list_head *ghes_get_devices(void) { return NULL; }
 
-- 
2.54.0

Re: [PATCH v5 04/13] ACPI: extlog: Defer CXL protocol error handling to avoid lock inversion

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing IRQ disable in work registration functions causes spinlock-in-IRQ deadlock with hardirq context.
--

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

ACPI: extlog: Defer CXL protocol error handling to avoid lock inversion

This commit fixes a lock inversion between the MCE notifier chain rwsem
and the PCI device_lock by deferring CXL protocol error handling to a
workqueue. It exports cxl_cper_post_prot_err() from the GHES driver to
be used by the extlog driver.

Link: https://lore.kernel.org/linux-cxl/20260709165457.8BA181F000E9@smtp.kernel.org/

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 3236a3ce79d6b..a382aabf9835e 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c

[ ... ]

> @@ -777,6 +777,7 @@ static void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
>  	schedule_work(cxl_cper_prot_err_work);
>  #endif
>  }
> +EXPORT_SYMBOL_FOR_MODULES(cxl_cper_post_prot_err, "acpi_extlog");
>  
>  int cxl_cper_register_prot_err_work(struct work_struct *work)
>  {

[Severity: High]
This is a pre-existing issue, but since this patch exports and expands the
usage of cxl_cper_post_prot_err(), could this lock lead to a hardirq
deadlock?

In cxl_cper_register_prot_err_work() and
cxl_cper_unregister_prot_err_work(), the lock is taken without disabling
interrupts:

    guard(spinlock)(&cxl_cper_prot_err_work_lock);

If a GHES interrupt arrives on the same CPU while this lock is held,
ghes_irq_func() will process the event and call cxl_cper_post_prot_err(),
which attempts to acquire the same lock:

    guard(spinlock_irqsave)(&cxl_cper_prot_err_work_lock);

Does this cause the IRQ handler to spin forever waiting for the lock held
by the interrupted process? A similar pattern appears to exist for
cxl_cper_work_lock in cxl_cper_register_work().

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

[PATCH v5 05/13] ACPI: extlog: Avoid populating software AER metadata from raw hardware buffer

[Dave Jiang]

extlog_print_pcie() casts pcie_err->aer_info straight to struct
aer_capability_regs *. That struct embeds struct pcie_tlp_log, whose
software-only header_len and flit fields sit at offset 84 - inside the
96-byte aer_info buffer - so the cast fills them with raw firmware bytes.
pcie_print_tlp_log() uses both to bound a loop over dw[], and a large
header_len walks past the end of the array.

Copy into a zeroed local struct, and only the part of aer_info that maps
onto it: the leading registers and the four Header Log DWORDs. The rest
stays zero, which covers header_len and flit and keeps the Root Error
registers out of the TLP prefix log, where pcie_print_tlp_log() would print
them as end-to-end prefixes.

Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/linux-cxl/20260709165457.8BA181F000E9@smtp.kernel.org/
Fixes: e778ffefa34d ("ACPI: extlog: Trace CPER PCI Express Error Section")
Reviewed-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Shuai Xue <xueshuai@linux.alibaba.com>
Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v5:
- Copy only the registers that match the hardware layout instead of all 96
  bytes plus explicit clears, so the rest falls out zero (Jonathan
  Cameron).
---
 drivers/acpi/acpi_extlog.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index 2451362e696d..feb7c002a713 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -137,7 +137,7 @@ static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
 			      int severity)
 {
 #ifdef ACPI_APEI_PCIEAER
-	struct aer_capability_regs *aer;
+	struct aer_capability_regs aer_regs = {};
 	struct pci_dev *pdev;
 	unsigned int devfn;
 	unsigned int bus;
@@ -149,7 +149,17 @@ static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
 		return;
 
 	aer_severity = cper_severity_to_aer(severity);
-	aer = (struct aer_capability_regs *)pcie_err->aer_info;
+
+	/*
+	 * struct pcie_tlp_log is larger than the hardware layout, so only the
+	 * leading registers and the four Header Log DWORDs of aer_info map onto
+	 * the struct. Copy that much and leave the rest zero, which covers the
+	 * software-only header_len and flit.
+	 */
+	memcpy(&aer_regs, pcie_err->aer_info,
+	       offsetof(struct aer_capability_regs, header_log) +
+	       PCIE_STD_NUM_TLP_HEADERLOG * sizeof(u32));
+
 	domain = pcie_err->device_id.segment;
 	bus = pcie_err->device_id.bus;
 	devfn = PCI_DEVFN(pcie_err->device_id.device,
@@ -158,7 +168,7 @@ static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
 	if (!pdev)
 		return;
 
-	pci_print_aer(pdev, aer_severity, aer);
+	pci_print_aer(pdev, aer_severity, &aer_regs);
 	pci_dev_put(pdev);
 #endif
 }
-- 
2.54.0

Re: [PATCH v5 05/13] ACPI: extlog: Avoid populating software AER metadata from raw hardware buffer

[sashiko-bot]

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

New issues:
- [Medium] Data loss of TLP Prefix Log / Extended Flit Log due to arbitrary payload truncation.

Pre-existing issues:
- [High] Out-of-bounds MMIO read and stack array bounds poisoning in `cxl_rch_get_aer_info()`.
--

commit 99282f77735a9b69f6cccf74ca39147b80930bf8
Author: Dave Jiang <dave.jiang@intel.com>
ACPI: extlog: Avoid populating software AER metadata from raw hardware buffer

This commit addresses an issue in extlog_print_pcie() where casting the raw
CPER aer_info buffer to a software struct populates software-only fields
with raw hardware bytes. It copies only the leading registers and the
Header Log DWORDs into a zeroed local struct to prevent out-of-bounds loop
reads.

Link: https://lore.kernel.org/linux-cxl/20260709165457.8BA181F000E9@smtp.kernel.org/

> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c
[ ... ]
> @@ -149,7 +149,17 @@ static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
>  		return;
>  
>  	aer_severity = cper_severity_to_aer(severity);
> -	aer = (struct aer_capability_regs *)pcie_err->aer_info;
> +
> +	/*
> +	 * struct pcie_tlp_log is larger than the hardware layout, so only the
> +	 * leading registers and the four Header Log DWORDs of aer_info map onto
> +	 * the struct. Copy that much and leave the rest zero, which covers the
> +	 * software-only header_len and flit.
> +	 */
> +	memcpy(&aer_regs, pcie_err->aer_info,
> +	       offsetof(struct aer_capability_regs, header_log) +
> +	       PCIE_STD_NUM_TLP_HEADERLOG * sizeof(u32));

[Severity: Medium]
Will this truncated copy result in the permanent loss of the TLP Prefix Log?

The CPER aer_info array contains a raw 96-byte snapshot of the hardware AER
capability, with the TLP Prefix Log located at offset 0x38 (56 bytes). By
truncating the memcpy to 44 bytes, the remainder of the CPER payload is
ignored, which silently drops the hardware-provided TLP Prefix Logs or
Extended Flit Logs during PCIe error handling.

Would it be better to explicitly extract the prefix log from offset 56 in
the CPER record into aer_regs.header_log.prefix to avoid data loss?

[ ... ]

This isn't a bug introduced by this patch, but I noticed a similar 
pre-existing AER software struct issue in drivers/cxl/core/ras_rch.c.

drivers/cxl/core/ras_rch.c:cxl_rch_get_aer_info()
> 	int read_cnt = sizeof(struct aer_capability_regs) / sizeof(u32);
> 	u32 *aer_regs_buf = (u32 *)aer_regs;
> 	int n;
> ...
> 	for (n = 0; n < read_cnt; n++)
> 		aer_regs_buf[n] = readl(aer_base + n * sizeof(u32));

[Severity: High]
Does this loop perform an out-of-bounds MMIO read by using the size of the
software structure?

Using sizeof(struct aer_capability_regs) (104 bytes) instead of the actual
hardware capability size directly maps MMIO data into software fields
(header_len and flit at offset 84). Later, when pcie_print_tlp_log() runs,
it uses the MMIO-poisoned header_len (which can be up to 255) as a loop
bound over the 14-element stack array log->dw[]. This leads to a massive
out-of-bounds stack read that can log arbitrary stack memory.

Additionally, in the same file:

drivers/cxl/core/ras_rch.c:cxl_rch_get_aer_severity()
> 		if (aer_regs->uncor_status & PCI_ERR_ROOT_FATAL_RCV)
> 			*severity = AER_FATAL;

[Severity: High]
Does this code misinterpret the flow control protocol error bit?

It tests the Root Error Status flag PCI_ERR_ROOT_FATAL_RCV (0x40) against
the uncor_status register. In the uncor_status register context, 0x40
actually represents PCI_ERR_UNC_FCP, meaning the severity is incorrectly
computed.

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

Re: [PATCH v5 05/13] ACPI: extlog: Avoid populating software AER metadata from raw hardware buffer

[Dave Jiang]

On 8/27/26 1:58 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Medium] Data loss of TLP Prefix Log / Extended Flit Log due to arbitrary payload truncation.

Yup. Will fix in v6.

DJ

> 
> Pre-existing issues:
> - [High] Out-of-bounds MMIO read and stack array bounds poisoning in `cxl_rch_get_aer_info()`.
> --
> 
> commit 99282f77735a9b69f6cccf74ca39147b80930bf8
> Author: Dave Jiang <dave.jiang@intel.com>
> ACPI: extlog: Avoid populating software AER metadata from raw hardware buffer
> 
> This commit addresses an issue in extlog_print_pcie() where casting the raw
> CPER aer_info buffer to a software struct populates software-only fields
> with raw hardware bytes. It copies only the leading registers and the
> Header Log DWORDs into a zeroed local struct to prevent out-of-bounds loop
> reads.
> 
> Link: https://lore.kernel.org/linux-cxl/20260709165457.8BA181F000E9@smtp.kernel.org/
> 
>> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
>> --- a/drivers/acpi/acpi_extlog.c
>> +++ b/drivers/acpi/acpi_extlog.c
> [ ... ]
>> @@ -149,7 +149,17 @@ static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
>>  		return;
>>  
>>  	aer_severity = cper_severity_to_aer(severity);
>> -	aer = (struct aer_capability_regs *)pcie_err->aer_info;
>> +
>> +	/*
>> +	 * struct pcie_tlp_log is larger than the hardware layout, so only the
>> +	 * leading registers and the four Header Log DWORDs of aer_info map onto
>> +	 * the struct. Copy that much and leave the rest zero, which covers the
>> +	 * software-only header_len and flit.
>> +	 */
>> +	memcpy(&aer_regs, pcie_err->aer_info,
>> +	       offsetof(struct aer_capability_regs, header_log) +
>> +	       PCIE_STD_NUM_TLP_HEADERLOG * sizeof(u32));
> 
> [Severity: Medium]
> Will this truncated copy result in the permanent loss of the TLP Prefix Log?
> 
> The CPER aer_info array contains a raw 96-byte snapshot of the hardware AER
> capability, with the TLP Prefix Log located at offset 0x38 (56 bytes). By
> truncating the memcpy to 44 bytes, the remainder of the CPER payload is
> ignored, which silently drops the hardware-provided TLP Prefix Logs or
> Extended Flit Logs during PCIe error handling.
> 
> Would it be better to explicitly extract the prefix log from offset 56 in
> the CPER record into aer_regs.header_log.prefix to avoid data loss?
> 
> [ ... ]
> 
> This isn't a bug introduced by this patch, but I noticed a similar 
> pre-existing AER software struct issue in drivers/cxl/core/ras_rch.c.
> 
> drivers/cxl/core/ras_rch.c:cxl_rch_get_aer_info()
>> 	int read_cnt = sizeof(struct aer_capability_regs) / sizeof(u32);
>> 	u32 *aer_regs_buf = (u32 *)aer_regs;
>> 	int n;
>> ...
>> 	for (n = 0; n < read_cnt; n++)
>> 		aer_regs_buf[n] = readl(aer_base + n * sizeof(u32));
> 
> [Severity: High]
> Does this loop perform an out-of-bounds MMIO read by using the size of the
> software structure?
> 
> Using sizeof(struct aer_capability_regs) (104 bytes) instead of the actual
> hardware capability size directly maps MMIO data into software fields
> (header_len and flit at offset 84). Later, when pcie_print_tlp_log() runs,
> it uses the MMIO-poisoned header_len (which can be up to 255) as a loop
> bound over the 14-element stack array log->dw[]. This leads to a massive
> out-of-bounds stack read that can log arbitrary stack memory.
> 
> Additionally, in the same file:
> 
> drivers/cxl/core/ras_rch.c:cxl_rch_get_aer_severity()
>> 		if (aer_regs->uncor_status & PCI_ERR_ROOT_FATAL_RCV)
>> 			*severity = AER_FATAL;
> 
> [Severity: High]
> Does this code misinterpret the flow control protocol error bit?
> 
> It tests the Root Error Status flag PCI_ERR_ROOT_FATAL_RCV (0x40) against
> the uncor_status register. In the uncor_status register context, 0x40
> actually represents PCI_ERR_UNC_FCP, meaning the severity is incorrectly
> computed.
> 

[PATCH v5 06/13] ACPI: extlog: Validate PCIe error section length before payload access

[Dave Jiang]

extlog_print_pcie() reads pcie_err->validation_bits and device_id and
copies the 96-byte aer_info buffer without checking that
gdata->error_data_length is big enough for a struct cper_sec_pcie. The
cper_estatus_check() call added earlier keeps the read inside the estatus
block, but a short section still gets stale adjacent bytes treated as PCIe
error data.

Reject a section too small to hold the record before touching any field,
and warn: a truncated section means firmware is emitting malformed records.

Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/linux-cxl/20260709165457.8BA181F000E9@smtp.kernel.org/
Fixes: e778ffefa34d ("ACPI: extlog: Trace CPER PCI Express Error Section")
Reviewed-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Shuai Xue <xueshuai@linux.alibaba.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/acpi_extlog.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index feb7c002a713..7c45a9865726 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -134,7 +134,7 @@ static int print_extlog_rcd(const char *pfx,
 }
 
 static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
-			      int severity)
+			      int severity, u32 len)
 {
 #ifdef ACPI_APEI_PCIEAER
 	struct aer_capability_regs aer_regs = {};
@@ -144,6 +144,12 @@ static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
 	int aer_severity;
 	int domain;
 
+	if (len < sizeof(*pcie_err)) {
+		pr_warn_ratelimited(FW_WARN
+				    "PCIe error section too small (%u)\n", len);
+		return;
+	}
+
 	if (!(pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID &&
 	      pcie_err->validation_bits & CPER_PCIE_VALID_AER_INFO))
 		return;
@@ -242,7 +248,8 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
 		} else if (guid_equal(sec_type, &CPER_SEC_PCIE)) {
 			struct cper_sec_pcie *pcie_err = acpi_hest_get_payload(gdata);
 
-			extlog_print_pcie(pcie_err, gdata->error_severity);
+			extlog_print_pcie(pcie_err, gdata->error_severity,
+					  gdata->error_data_length);
 		} else {
 			void *err = acpi_hest_get_payload(gdata);
 
-- 
2.54.0

[PATCH v5 07/13] ACPI: extlog: Fix CONFIG_ACPI_APEI_PCIEAER guard typo

[Dave Jiang]

The guard reads "#ifdef ACPI_APEI_PCIEAER" rather than
"#ifdef CONFIG_ACPI_APEI_PCIEAER". That symbol is never defined, so the
extlog PCIe AER handling is always compiled out.

Use the CONFIG_ prefixed symbol.

Fixes: e778ffefa34d ("ACPI: extlog: Trace CPER PCI Express Error Section")
Reviewed-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Shuai Xue <xueshuai@linux.alibaba.com>
Reviewed-by: Ben Cheatham <benjamin.cheatham@amd.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/acpi_extlog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index 7c45a9865726..ab46fa2adc53 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -136,7 +136,7 @@ static int print_extlog_rcd(const char *pfx,
 static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
 			      int severity, u32 len)
 {
-#ifdef ACPI_APEI_PCIEAER
+#ifdef CONFIG_ACPI_APEI_PCIEAER
 	struct aer_capability_regs aer_regs = {};
 	struct pci_dev *pdev;
 	unsigned int devfn;
-- 
2.54.0

[PATCH v5 08/13] ACPI: APEI: GHES: Bound CXL event record copy to the firmware section length

[Dave Jiang]

cxl_cper_post_event() copies a fixed sizeof(struct cxl_cper_event_rec)
out of the firmware CPER section without checking how long the section
actually is, so a short one reads past the record.

Pass gdata->error_data_length in and reject a section too small to hold
the record before the copy.

Reported-by: sashiko-bot@kernel.org
Link: https://sashiko.dev/#/patchset/20260617-topics-ahmtib01-ras_ffh_arm_internal_review-v6-0-91f725174aa0@arm.com?part=6
Fixes: 5e4a264bf8b5 ("acpi/ghes: Process CXL Component Events")
Reviewed-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Shuai Xue <xueshuai@linux.alibaba.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/apei/ghes.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index a382aabf9835..e5f8dbd17017 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -816,10 +816,15 @@ static DEFINE_SPINLOCK(cxl_cper_work_lock);
 struct work_struct *cxl_cper_work;
 
 static void cxl_cper_post_event(enum cxl_event_type event_type,
-				struct cxl_cper_event_rec *rec)
+				struct cxl_cper_event_rec *rec, u32 len)
 {
 	struct cxl_cper_work_data wd;
 
+	if (len < sizeof(*rec)) {
+		pr_err(FW_WARN "CXL CPER section too small (%u)\n", len);
+		return;
+	}
+
 	if (rec->hdr.length <= sizeof(rec->hdr) ||
 	    rec->hdr.length > sizeof(*rec)) {
 		pr_err(FW_WARN "CXL CPER Invalid section length (%u)\n",
@@ -950,15 +955,18 @@ static void ghes_do_proc(struct ghes *ghes,
 		} else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID)) {
 			struct cxl_cper_event_rec *rec = acpi_hest_get_payload(gdata);
 
-			cxl_cper_post_event(CXL_CPER_EVENT_GEN_MEDIA, rec);
+			cxl_cper_post_event(CXL_CPER_EVENT_GEN_MEDIA, rec,
+					    gdata->error_data_length);
 		} else if (guid_equal(sec_type, &CPER_SEC_CXL_DRAM_GUID)) {
 			struct cxl_cper_event_rec *rec = acpi_hest_get_payload(gdata);
 
-			cxl_cper_post_event(CXL_CPER_EVENT_DRAM, rec);
+			cxl_cper_post_event(CXL_CPER_EVENT_DRAM, rec,
+					    gdata->error_data_length);
 		} else if (guid_equal(sec_type, &CPER_SEC_CXL_MEM_MODULE_GUID)) {
 			struct cxl_cper_event_rec *rec = acpi_hest_get_payload(gdata);
 
-			cxl_cper_post_event(CXL_CPER_EVENT_MEM_MODULE, rec);
+			cxl_cper_post_event(CXL_CPER_EVENT_MEM_MODULE, rec,
+					    gdata->error_data_length);
 		} else {
 			void *err = acpi_hest_get_payload(gdata);
 
-- 
2.54.0

Re: [PATCH v5 08/13] ACPI: APEI: GHES: Bound CXL event record copy to the firmware section length

[sashiko-bot]

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

Pre-existing issues:
- [High] Potential AB-BA deadlock due to missing IRQ disablement when acquiring spinlocks shared with hardirq context.
--

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

ACPI: APEI: GHES: Bound CXL event record copy to the firmware section length

cxl_cper_post_event() copies a fixed sizeof(struct cxl_cper_event_rec)
out of the firmware CPER section without checking how long the section
actually is. This patch passes the section length in and rejects sections
too small to hold the record before the copy.

Link: https://sashiko.dev/#/patchset/20260617-topics-ahmtib01-ras_ffh_arm_internal_review-v6-0-91f725174aa0@arm.com?part=6

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index a382aabf9835e..e5f8dbd17017e 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -816,10 +816,15 @@ static DEFINE_SPINLOCK(cxl_cper_work_lock);
>  struct work_struct *cxl_cper_work;
>  
>  static void cxl_cper_post_event(enum cxl_event_type event_type,
> -				struct cxl_cper_event_rec *rec)
> +				struct cxl_cper_event_rec *rec, u32 len)
>  {

[Severity: High]
This is a pre-existing issue, but does the locking strategy here expose the
kernel to a potential deadlock?

cxl_cper_post_event() and cxl_cper_post_prot_err() execute in hardirq context
(e.g., via ghes_proc_in_irq() or ghes_irq_func()) and acquire
cxl_cper_work_lock or cxl_cper_prot_err_work_lock.

However, the registration functions in drivers/acpi/apei/ghes.c such as
cxl_cper_register_work(), cxl_cper_unregister_work(),
cxl_cper_register_prot_err_work(), and cxl_cper_unregister_prot_err_work()
execute in process context and acquire these same spinlocks using
guard(spinlock), which expands to spin_lock() and does not disable local
interrupts.

If a CPU executing one of these registration functions is interrupted by a
hardware error IRQ while holding the lock, the interrupt handler will attempt
to acquire the exact same lock in cxl_cper_post_event() or
cxl_cper_post_prot_err(), resulting in an immediate deadlock.

Should the registration functions use guard(spinlock_irqsave) instead of
guard(spinlock) to prevent this?

>  	struct cxl_cper_work_data wd;
>  
> +	if (len < sizeof(*rec)) {
> +		pr_err(FW_WARN "CXL CPER section too small (%u)\n", len);
> +		return;
> +	}
> +

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

[PATCH v5 09/13] ACPI: APEI: GHES: Validate CXL protocol error section length before RAS cap copy

[Dave Jiang]

cxl_cper_setup_prot_err_work_data() locates the RAS Capability block at
prot_err + sizeof(*prot_err) + dvsec_len and copies it, but dvsec_len is
firmware controlled and never validated, so it can point the copy outside
the section.

Extend cxl_cper_sec_prot_err_valid() to check that the section can hold
the header, and that the header, DVSEC and RAS Capability block together
fit the reported section length.

Reported-by: sashiko-bot@kernel.org
Link: https://sashiko.dev/#/patchset/20260617-topics-ahmtib01-ras_ffh_arm_internal_review-v6-0-91f725174aa0@arm.com?part=6
Link: https://lore.kernel.org/linux-cxl/20260709165457.8BA181F000E9@smtp.kernel.org/
Fixes: 315c2f0b53ba ("acpi/ghes, cper: Recognize and cache CXL Protocol errors")
Reviewed-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Shuai Xue <xueshuai@linux.alibaba.com>
Reviewed-by: Ben Cheatham <benjamin.cheatham@amd.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/acpi_extlog.c       |  3 ++-
 drivers/acpi/apei/ghes.c         |  7 ++++---
 drivers/acpi/apei/ghes_helpers.c | 18 +++++++++++++++++-
 include/acpi/ghes.h              |  2 +-
 include/cxl/event.h              |  4 ++--
 5 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index ab46fa2adc53..3e0d21fb287c 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -244,7 +244,8 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
 				acpi_hest_get_payload(gdata);
 
 			cxl_cper_post_prot_err(prot_err,
-					       gdata->error_severity);
+					       gdata->error_severity,
+					       gdata->error_data_length);
 		} else if (guid_equal(sec_type, &CPER_SEC_PCIE)) {
 			struct cper_sec_pcie *pcie_err = acpi_hest_get_payload(gdata);
 
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index e5f8dbd17017..b8dbd99da47e 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -753,12 +753,12 @@ static DEFINE_SPINLOCK(cxl_cper_prot_err_work_lock);
 struct work_struct *cxl_cper_prot_err_work;
 
 void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
-			    int severity)
+			    int severity, u32 len)
 {
 #ifdef CONFIG_ACPI_APEI_PCIEAER
 	struct cxl_cper_prot_err_work_data wd;
 
-	if (cxl_cper_sec_prot_err_valid(prot_err))
+	if (cxl_cper_sec_prot_err_valid(prot_err, len))
 		return;
 
 	guard(spinlock_irqsave)(&cxl_cper_prot_err_work_lock);
@@ -951,7 +951,8 @@ static void ghes_do_proc(struct ghes *ghes,
 		} else if (guid_equal(sec_type, &CPER_SEC_CXL_PROT_ERR)) {
 			struct cxl_cper_sec_prot_err *prot_err = acpi_hest_get_payload(gdata);
 
-			cxl_cper_post_prot_err(prot_err, gdata->error_severity);
+			cxl_cper_post_prot_err(prot_err, gdata->error_severity,
+					       gdata->error_data_length);
 		} else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID)) {
 			struct cxl_cper_event_rec *rec = acpi_hest_get_payload(gdata);
 
diff --git a/drivers/acpi/apei/ghes_helpers.c b/drivers/acpi/apei/ghes_helpers.c
index bc7111b740af..df41b993f413 100644
--- a/drivers/acpi/apei/ghes_helpers.c
+++ b/drivers/acpi/apei/ghes_helpers.c
@@ -5,8 +5,15 @@
 #include <linux/aer.h>
 #include <cxl/event.h>
 
-int cxl_cper_sec_prot_err_valid(struct cxl_cper_sec_prot_err *prot_err)
+int cxl_cper_sec_prot_err_valid(struct cxl_cper_sec_prot_err *prot_err, u32 len)
 {
+	if (len < sizeof(*prot_err)) {
+		pr_err_ratelimited(FW_WARN
+				   "CXL CPER prot err section too small (%u)\n",
+				   len);
+		return -EINVAL;
+	}
+
 	if (!(prot_err->valid_bits & PROT_ERR_VALID_AGENT_ADDRESS)) {
 		pr_err_ratelimited("CXL CPER invalid agent type\n");
 		return -EINVAL;
@@ -23,6 +30,15 @@ int cxl_cper_sec_prot_err_valid(struct cxl_cper_sec_prot_err *prot_err)
 		return -EINVAL;
 	}
 
+	/* The RAS Capability block sits after a firmware-sized DVSEC. */
+	if (sizeof(*prot_err) + prot_err->dvsec_len +
+	    sizeof(struct cxl_ras_capability_regs) > len) {
+		pr_err_ratelimited(FW_WARN
+				   "CXL CPER prot err DVSEC (%u) overruns section (%u)\n",
+				   prot_err->dvsec_len, len);
+		return -EINVAL;
+	}
+
 	if ((prot_err->agent_type == RCD || prot_err->agent_type == DEVICE ||
 	     prot_err->agent_type == LD || prot_err->agent_type == FMLD) &&
 	    !(prot_err->valid_bits & PROT_ERR_VALID_SERIAL_NUMBER))
diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
index be496bc0386f..7acf209061ea 100644
--- a/include/acpi/ghes.h
+++ b/include/acpi/ghes.h
@@ -88,7 +88,7 @@ void ghes_estatus_pool_region_free(unsigned long addr, u32 size);
 
 struct cxl_cper_sec_prot_err;
 void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
-			    int severity);
+			    int severity, u32 len);
 #else
 static inline struct list_head *ghes_get_devices(void) { return NULL; }
 
diff --git a/include/cxl/event.h b/include/cxl/event.h
index ff97fea718d2..912305bee3bc 100644
--- a/include/cxl/event.h
+++ b/include/cxl/event.h
@@ -321,13 +321,13 @@ static inline int cxl_cper_prot_err_kfifo_get(struct cxl_cper_prot_err_work_data
 #endif
 
 #ifdef CONFIG_ACPI_APEI_PCIEAER
-int cxl_cper_sec_prot_err_valid(struct cxl_cper_sec_prot_err *prot_err);
+int cxl_cper_sec_prot_err_valid(struct cxl_cper_sec_prot_err *prot_err, u32 len);
 int cxl_cper_setup_prot_err_work_data(struct cxl_cper_prot_err_work_data *wd,
 				      struct cxl_cper_sec_prot_err *prot_err,
 				      int severity);
 #else
 static inline int
-cxl_cper_sec_prot_err_valid(struct cxl_cper_sec_prot_err *prot_err)
+cxl_cper_sec_prot_err_valid(struct cxl_cper_sec_prot_err *prot_err, u32 len)
 {
 	return -EOPNOTSUPP;
 }
-- 
2.54.0

Re: [PATCH v5 09/13] ACPI: APEI: GHES: Validate CXL protocol error section length before RAS cap copy

[sashiko-bot]

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

Pre-existing issues:
- [High] Potential deadlock because `cxl_cper_prot_err_work_lock` is acquired in process context without disabling hardware interrupts.
--

commit 1416c4f9e4b0f4dc84b6e812f70c5f34a5255acc
Author: Dave Jiang <dave.jiang@intel.com>

ACPI: APEI: GHES: Validate CXL protocol error section length before RAS cap copy

cxl_cper_setup_prot_err_work_data() locates the RAS Capability block using an
unvalidated firmware-controlled length, which can point the copy outside the
section. This commit extends cxl_cper_sec_prot_err_valid() to check that the
section can hold the header, DVSEC, and RAS Capability block within the reported
section length.

Link: https://sashiko.dev/#/patchset/20260617-topics-ahmtib01-ras_ffh_arm_internal_review-v6-0-91f725174aa0@arm.com?part=6
Link: https://lore.kernel.org/linux-cxl/20260709165457.8BA181F000E9@smtp.kernel.org/

[ ... ]

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index e5f8dbd17017e..b8dbd99da47ef 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -753,12 +753,12 @@ static DEFINE_SPINLOCK(cxl_cper_prot_err_work_lock);
>  struct work_struct *cxl_cper_prot_err_work;
>  
>  void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
> -			    int severity)
> +			    int severity, u32 len)
>  {
>  #ifdef CONFIG_ACPI_APEI_PCIEAER
>  	struct cxl_cper_prot_err_work_data wd;
>  
> -	if (cxl_cper_sec_prot_err_valid(prot_err))
> +	if (cxl_cper_sec_prot_err_valid(prot_err, len))
>  		return;
>  
>  	guard(spinlock_irqsave)(&cxl_cper_prot_err_work_lock);

[Severity: High]
This isn't a bug introduced by this patch, but does acquiring this lock in
hardirq context cause a potential deadlock?

If the module initialization thread executes cxl_cper_register_prot_err_work()
(or unregister) in process context, it acquires cxl_cper_prot_err_work_lock
with interrupts enabled:

drivers/acpi/apei/ghes.c:cxl_cper_register_prot_err_work() {
    ...
	guard(spinlock)(&cxl_cper_prot_err_work_lock);
    ...
}

If a GHES hardware error interrupt fires on the same CPU, executing
ghes_do_proc() -> cxl_cper_post_prot_err(), it will attempt to acquire the
same lock here using guard(spinlock_irqsave).

Would it spin forever waiting for the interrupted process context to release
the lock?

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

[PATCH v5 10/13] efi/cper: Read only validated fields in cper_mem_err_pack()

[Dave Jiang]

cper_mem_err_pack() copies extended, rank, mem_array_handle and
mem_dev_handle unconditionally. Those live at offsets 73 to 79, past the
end of struct cper_sec_mem_err_old, the 73-byte UEFI 2.1/2.2 layout that
older firmware still emits and that cper_estatus_print_section() admits.
On such a record the copy reads up to seven bytes past the payload, and off
the end of the error status block when that section is the last one.

Copy each of the four only when its validation bit is set, and zero it
otherwise. Nothing is lost: a 2.1/2.2 record leaves those bits clear, and
every consumer of struct cper_mem_err_compact already gates the fields on
the same bits. Zeroing also stops callers reading them back out of the
uninitialised on-stack struct they pass in.

Reported-by: sashiko-bot@kernel.org
Closes: https://sashiko.dev/#/patchset/20260714231835.303081-1-dave.jiang@intel.com?part=7
Fixes: 2dfb7d51a61d ("trace, RAS: Add eMCA trace event interface")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/firmware/efi/cper.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index 3dcef618ad39..13b3e72c3b62 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -389,10 +389,28 @@ void cper_mem_err_pack(const struct cper_sec_mem_err *mem,
 	cmem->requestor_id = mem->requestor_id;
 	cmem->responder_id = mem->responder_id;
 	cmem->target_id = mem->target_id;
-	cmem->extended = mem->extended;
-	cmem->rank = mem->rank;
-	cmem->mem_array_handle = mem->mem_array_handle;
-	cmem->mem_dev_handle = mem->mem_dev_handle;
+
+	/*
+	 * These four sit past the end of the UEFI 2.1/2.2 layout, which older
+	 * firmware still emits, so reading them unconditionally runs off a
+	 * short record. Every consumer of the compact record gates them on the
+	 * same validation bits, so leave them zero when firmware does not
+	 * claim them.
+	 */
+	cmem->extended = 0;
+	cmem->rank = 0;
+	cmem->mem_array_handle = 0;
+	cmem->mem_dev_handle = 0;
+
+	if (mem->validation_bits &
+	    (CPER_MEM_VALID_ROW_EXT | CPER_MEM_VALID_CHIP_ID))
+		cmem->extended = mem->extended;
+	if (mem->validation_bits & CPER_MEM_VALID_RANK_NUMBER)
+		cmem->rank = mem->rank;
+	if (mem->validation_bits & CPER_MEM_VALID_CARD_HANDLE)
+		cmem->mem_array_handle = mem->mem_array_handle;
+	if (mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE)
+		cmem->mem_dev_handle = mem->mem_dev_handle;
 }
 EXPORT_SYMBOL_GPL(cper_mem_err_pack);
 
-- 
2.54.0

Re: [PATCH v5 10/13] efi/cper: Read only validated fields in cper_mem_err_pack()

[sashiko-bot]

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

Pre-existing issues:
- [Medium] cper_mem_err_pack() remains vulnerable to out-of-bounds reads because cper_print_mem() bypasses length validation for records slightly larger than the base structure.
--

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

efi/cper: Read only validated fields in cper_mem_err_pack()

cper_mem_err_pack() copies extended, rank, mem_array_handle and
mem_dev_handle unconditionally. Those live at offsets 73 to 79, past the
end of struct cper_sec_mem_err_old, the 73-byte UEFI 2.1/2.2 layout that
older firmware still emits and that cper_estatus_print_section() admits.
On such a record the copy reads up to seven bytes past the payload, and off
the end of the error status block when that section is the last one.

Link: https://sashiko.dev/#/patchset/20260714231835.303081-1-dave.jiang@intel.com?part=7

> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 3dcef618ad39f..13b3e72c3b62f 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -389,10 +389,28 @@ void cper_mem_err_pack(const struct cper_sec_mem_err *mem,
>  	cmem->requestor_id = mem->requestor_id;
>  	cmem->responder_id = mem->responder_id;
>  	cmem->target_id = mem->target_id;
> -	cmem->extended = mem->extended;
> -	cmem->rank = mem->rank;
> -	cmem->mem_array_handle = mem->mem_array_handle;
> -	cmem->mem_dev_handle = mem->mem_dev_handle;
> +
> +	/*
> +	 * These four sit past the end of the UEFI 2.1/2.2 layout, which older
> +	 * firmware still emits, so reading them unconditionally runs off a
> +	 * short record. Every consumer of the compact record gates them on the
> +	 * same validation bits, so leave them zero when firmware does not
> +	 * claim them.
> +	 */
> +	cmem->extended = 0;
> +	cmem->rank = 0;
> +	cmem->mem_array_handle = 0;
> +	cmem->mem_dev_handle = 0;
> +
> +	if (mem->validation_bits &
> +	    (CPER_MEM_VALID_ROW_EXT | CPER_MEM_VALID_CHIP_ID))
> +		cmem->extended = mem->extended;
> +	if (mem->validation_bits & CPER_MEM_VALID_RANK_NUMBER)
> +		cmem->rank = mem->rank;
> +	if (mem->validation_bits & CPER_MEM_VALID_CARD_HANDLE)
> +		cmem->mem_array_handle = mem->mem_array_handle;
> +	if (mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE)
> +		cmem->mem_dev_handle = mem->mem_dev_handle;

[Severity: Medium]
This is a pre-existing issue, but does this conditional read still allow an
out-of-bounds read if the record length is between 74 and 79 bytes?

In cper_print_mem(), the length validation only checks if the length is
exactly sizeof(struct cper_sec_mem_err_old) (which is 73 bytes):

drivers/firmware/efi/cper.c:cper_print_mem() {
	...
	/* Don't trust UEFI 2.1/2.2 structure with bad validation bits */
	if (len == sizeof(struct cper_sec_mem_err_old) &&
	    (mem->validation_bits & ~(CPER_MEM_VALID_RANK_NUMBER - 1))) {
		pr_err(FW_WARN "valid bits set for fields beyond structure\n");
		return;
	}
	...
	cper_mem_err_pack(mem, &cmem);
	...
}

If the length is slightly larger, such as 74 bytes, this check is completely
bypassed. If firmware provides a record of length 74 with extended validation
bits set (e.g., CPER_MEM_VALID_MODULE_HANDLE), cper_mem_err_pack() will trust
these validation bits and read out of bounds up to 6 bytes, leaking adjacent
memory content.

While a follow-up patch ("ACPI: APEI: GHES: Validate memory error section
length before payload access") fixed this vulnerability for consumers reached
via ghes_do_proc(), cper_print_mem() remains unchanged.

Could cper_print_mem() be updated to ensure the length covers the fields
indicated by the validation bits?

>  }
>  EXPORT_SYMBOL_GPL(cper_mem_err_pack);

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

[PATCH v5 11/13] ACPI: APEI: GHES: Validate memory error section length before payload access

[Dave Jiang]

ghes_do_proc() hands the CPER_SEC_PLATFORM_MEM payload to the report chain,
arch_apei_report_mem_error() and ghes_handle_memory_failure() without
checking gdata->error_data_length. All three read validation_bits and
physical_addr, at offsets 0 and 16, so a shorter section reads past the
record.

Check the length once in ghes_do_proc(), before any consumer runs. Take the
73-byte struct cper_sec_mem_err_old as the floor: older firmware
legitimately emits that UEFI 2.1/2.2 layout, and it makes validation_bits
safe to read.

The fields from "extended" on are absent from that layout, so also require
whatever length the validation bits claim. Derive it per field rather than
demanding the full 80 bytes: rank needs only 76, and the BANK_GROUP and
BANK_ADDRESS bits describe "bank" at offset 38, which every record carries.
Testing the whole mask against one size, as cper_print_mem() does, gets it
wrong both ways.

Reported-by: sashiko-bot@kernel.org
Closes: https://sashiko.dev/#/patchset/20260714231835.303081-1-dave.jiang@intel.com?part=7
Fixes: ca104edc1784 ("ACPI, APEI, GHES: Cleanup ghes memory error handling")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/apei/ghes.c | 41 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index b8dbd99da47e..0cc1e6383635 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -915,6 +915,28 @@ static void ghes_log_hwerr(int sev, guid_t *sec_type)
 	hwerr_log_error_type(HWERR_RECOV_OTHERS);
 }
 
+/*
+ * The fields from "extended" on are absent from the 73-byte UEFI 2.1/2.2
+ * layout that older firmware still emits. Return the length needed for the
+ * fields the validation bits claim, so over-claiming is rejected without
+ * rejecting an honest short record.
+ */
+static u32 ghes_mem_err_min_len(u64 validation_bits)
+{
+	u32 len = sizeof(struct cper_sec_mem_err_old);
+
+	if (validation_bits & (CPER_MEM_VALID_ROW_EXT | CPER_MEM_VALID_CHIP_ID))
+		len = offsetof(struct cper_sec_mem_err, rank);
+	if (validation_bits & CPER_MEM_VALID_RANK_NUMBER)
+		len = offsetof(struct cper_sec_mem_err, mem_array_handle);
+	if (validation_bits & CPER_MEM_VALID_CARD_HANDLE)
+		len = offsetof(struct cper_sec_mem_err, mem_dev_handle);
+	if (validation_bits & CPER_MEM_VALID_MODULE_HANDLE)
+		len = sizeof(struct cper_sec_mem_err);
+
+	return len;
+}
+
 static void ghes_do_proc(struct ghes *ghes,
 			 const struct acpi_hest_generic_status *estatus)
 {
@@ -940,6 +962,25 @@ static void ghes_do_proc(struct ghes *ghes,
 		if (guid_equal(sec_type, &CPER_SEC_PLATFORM_MEM)) {
 			struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
 
+			/*
+			 * Check once for all three consumers below. The 73-byte
+			 * UEFI 2.1/2.2 layout is the floor, matching
+			 * cper_estatus_print_section() and making
+			 * validation_bits safe to read.
+			 */
+			if (gdata->error_data_length <
+			    sizeof(struct cper_sec_mem_err_old))
+				continue;
+
+			/* Then require what the claimed fields actually need. */
+			if (gdata->error_data_length <
+			    ghes_mem_err_min_len(mem_err->validation_bits)) {
+				pr_warn_ratelimited(FW_WARN GHES_PFX
+						    "memory error section too small (%u) for the fields it claims\n",
+						    gdata->error_data_length);
+				continue;
+			}
+
 			atomic_notifier_call_chain(&ghes_report_chain, sev, mem_err);
 
 			arch_apei_report_mem_error(sev, mem_err);
-- 
2.54.0

Re: [PATCH v5 11/13] ACPI: APEI: GHES: Validate memory error section length before payload access

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch identifies but fails to fix an out-of-bounds read vulnerability in `cper_print_mem()` and `cper_mem_err_pack()` when handling malformed memory error sections.
--

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

ACPI: APEI: GHES: Validate memory error section length before payload access

ghes_do_proc() hands the CPER_SEC_PLATFORM_MEM payload to the report chain,
arch_apei_report_mem_error() and ghes_handle_memory_failure() without
checking gdata->error_data_length. All three read validation_bits and
physical_addr, at offsets 0 and 16, so a shorter section reads past the
record.

Check the length once in ghes_do_proc(), before any consumer runs. Take the
73-byte struct cper_sec_mem_err_old as the floor: older firmware
legitimately emits that UEFI 2.1/2.2 layout, and it makes validation_bits
safe to read.

Closes: https://sashiko.dev/#/patchset/20260714231835.303081-1-dave.jiang@intel.com?part=7

> The fields from "extended" on are absent from that layout, so also require
> whatever length the validation bits claim. Derive it per field rather than
> demanding the full 80 bytes: rank needs only 76, and the BANK_GROUP and
> BANK_ADDRESS bits describe "bank" at offset 38, which every record carries.
> Testing the whole mask against one size, as cper_print_mem() does, gets it
> wrong both ways.

[Severity: High]
This is a pre-existing issue, but since the commit message notes the flaw in
cper_print_mem(), should this patch also fix the out-of-bounds read there?

A CPER memory error record containing a CPER_SEC_PLATFORM_MEM section with a
length between 74 and 79 bytes and CPER_MEM_VALID_MODULE_HANDLE set can trigger
an out-of-bounds read of up to 6 bytes.

When cper_estatus_print_section() in drivers/firmware/efi/cper.c processes such
a record:

drivers/firmware/efi/cper.c:cper_estatus_print_section() {
...
		if (gdata->error_data_length >=
		    sizeof(struct cper_sec_mem_err_old))
			cper_print_mem(newpfx, mem_err,
				       gdata->error_data_length);
...
}

It calls cper_print_mem(), which bypasses its invalid bits check because
len != 73:

drivers/firmware/efi/cper.c:cper_print_mem() {
...
	if (len == sizeof(struct cper_sec_mem_err_old) &&
	    (mem->validation_bits & ~(CPER_MEM_VALID_RANK_NUMBER - 1))) {
		pr_err(FW_WARN "valid bits set for fields beyond structure\n");
		return;
	}
...
	cper_mem_err_pack(mem, &cmem);
...
}

It then unconditionally calls cper_mem_err_pack(), which reads
mem->mem_dev_handle at offset 78 regardless of the record length:

drivers/firmware/efi/cper.c:cper_mem_err_pack() {
...
	if (mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE)
		cmem->mem_dev_handle = mem->mem_dev_handle;
...
}

Can this lead to a kernel oops if the struct is located exactly at the end of a
mapped page, or leak kernel heap memory into dmesg via DMI handle lookups?

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

[PATCH v5 12/13] ACPI: APEI: GHES: Bound AER info copy and sanitize software metadata

[Dave Jiang]

ghes_handle_aer() copies sizeof(struct aer_capability_regs) out of the
fixed 96-byte pcie_err->aer_info. The struct is larger, so the copy reads
past the section, and it fills the software-only header_len and flit fields
of the embedded struct pcie_tlp_log from firmware bytes.
pcie_print_tlp_log() uses both to bound a loop over dw[], so a large value
walks past the array. Nothing checks the section can hold a struct
cper_sec_pcie either.

Validate error_data_length, zero the destination, and copy only what maps
onto the struct: the leading registers and the four Header Log DWORDs,
mirroring the extlog_print_pcie() fix. The rest stays zero, covering
header_len and flit.

Reported-by: sashiko-bot@kernel.org
Closes: https://sashiko.dev/#/patchset/20260714231835.303081-1-dave.jiang@intel.com?part=3
Fixes: 7e077e6707b3 ("PCI/ERR: Handle TLP Log in Flit mode")
Reviewed-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Shuai Xue <xueshuai@linux.alibaba.com>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v5:
- Copy only the registers that match the hardware layout rather than all 96
  bytes plus explicit clears, matching the same change in the extlog path
  (Jonathan Cameron).
- Note the copy length changed, so the printed TLP prefix log changes too;
  Alison's and Shuai's Reviewed-by are kept since the intent and location
  are the same, but please re-check if you disagree.
---
 drivers/acpi/apei/ghes.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 0cc1e6383635..3df62ec20cf9 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -642,11 +642,14 @@ static void ghes_handle_aer(struct acpi_hest_generic_data *gdata)
 #ifdef CONFIG_ACPI_APEI_PCIEAER
 	struct cper_sec_pcie *pcie_err = acpi_hest_get_payload(gdata);
 
+	if (gdata->error_data_length < sizeof(*pcie_err))
+		return;
+
 	if (pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID &&
 	    pcie_err->validation_bits & CPER_PCIE_VALID_AER_INFO) {
+		struct aer_capability_regs *aer_info;
 		unsigned int devfn;
 		int aer_severity;
-		u8 *aer_info;
 
 		devfn = PCI_DEVFN(pcie_err->device_id.device,
 				  pcie_err->device_id.function);
@@ -664,13 +667,21 @@ static void ghes_handle_aer(struct acpi_hest_generic_data *gdata)
 						  sizeof(struct aer_capability_regs));
 		if (!aer_info)
 			return;
-		memcpy(aer_info, pcie_err->aer_info, sizeof(struct aer_capability_regs));
+
+		/*
+		 * Copy only what maps onto the struct, as extlog_print_pcie()
+		 * does: the leading registers and the four Header Log DWORDs.
+		 * 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));
 
 		aer_recover_queue(pcie_err->device_id.segment,
 				  pcie_err->device_id.bus,
-				  devfn, aer_severity,
-				  (struct aer_capability_regs *)
-				  aer_info);
+				  devfn, aer_severity, aer_info);
 	}
 #endif
 }
-- 
2.54.0

Re: [PATCH v5 12/13] ACPI: APEI: GHES: Bound AER info copy and sanitize software metadata

[sashiko-bot]

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

Pre-existing issues:
- [High] Pre-existing memory leak of dynamically allocated `aer_info` in `aer_recover_queue()` when the recovery kfifo is full.
- [High] Pre-existing memory leak of pool region in `aer_recover_work_func()` when the PCI device cannot be found.
--

commit 3fa389697c596cc80036725c9cd2026267df97d9
Author: Dave Jiang <dave.jiang@intel.com>

ACPI: APEI: GHES: Bound AER info copy and sanitize software metadata

ghes_handle_aer() copies sizeof(struct aer_capability_regs) out of the
fixed 96-byte pcie_err->aer_info. The struct is larger, so the copy reads
past the section, and it fills the software-only header_len and flit fields
of the embedded struct pcie_tlp_log from firmware bytes. This commit
validates error_data_length, zeroes the destination, and copies only what
maps onto the struct to prevent out-of-bounds loop execution.

Closes: https://sashiko.dev/#/patchset/20260714231835.303081-1-dave.jiang@intel.com?part=3

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 0cc1e63836356..3df62ec20cf96 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -664,13 +667,21 @@ static void ghes_handle_aer(struct acpi_hest_generic_data *gdata)
>  						  sizeof(struct aer_capability_regs));
>  		if (!aer_info)
>  			return;
> -		memcpy(aer_info, pcie_err->aer_info, sizeof(struct aer_capability_regs));
> +
> +		/*
> +		 * Copy only what maps onto the struct, as extlog_print_pcie()
> +		 * does: the leading registers and the four Header Log DWORDs.
> +		 * 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));
>  
>  		aer_recover_queue(pcie_err->device_id.segment,
>  				  pcie_err->device_id.bus,
> -				  devfn, aer_severity,
> -				  (struct aer_capability_regs *)
> -				  aer_info);
> +				  devfn, aer_severity, aer_info);

[Severity: High]
This is a pre-existing issue, but does aer_recover_queue() leak the
dynamically allocated aer_info if the recovery kfifo is full?

In ghes_handle_aer(), aer_info is allocated from the ghes_estatus_pool and
passed to aer_recover_queue(). If the aer_recover_ring is full,
kfifo_in_spinlocked() returns 0 and aer_recover_queue() logs an error:

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 for %04x:%02x:%02x.%x\n",
		       domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
}

The function returns without freeing aer_regs, permanently leaking memory
from the lockless GHES estatus pool.

[Severity: High]
This is also a pre-existing issue, but does aer_recover_work_func() leak
this pool region when a PCI device cannot be found?

When aer_recover_work_func() dequeues entries, it attempts to resolve the
pci_dev. If it fails, it executes continue, skipping the rest of the loop:

drivers/pci/pcie/aer.c:aer_recover_work_func() {
    ...
		pdev = pci_get_domain_bus_and_slot(entry.domain, entry.bus,
						   entry.devfn);
		if (!pdev) {
			pr_err_ratelimited("%04x:%02x:%02x.%x: no pci_dev found\n",
					   entry.domain, entry.bus,
					   PCI_SLOT(entry.devfn),
					   PCI_FUNC(entry.devfn));
			continue;
		}
    ...
}

The call to ghes_estatus_pool_region_free() is at the end of the loop body,
so the continue statement bypasses it, permanently leaking the memory from
the GHES estatus pool.

>  	}
>  #endif
>  }

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

[PATCH v5 13/13] cxl/ras: Make cxl_cper_handle_prot_err() static

[Dave Jiang]

The extlog caller went away when extlog switched to
cxl_cper_post_prot_err(), leaving cxl_cper_prot_err_work_fn() in the same
file as the only caller.

Drop the export and the declaration in <cxl/event.h>, and make it static.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/ras.c | 3 +--
 include/cxl/event.h    | 2 --
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
index 99fb00949c2f..eceb33bc534b 100644
--- a/drivers/cxl/core/ras.c
+++ b/drivers/cxl/core/ras.c
@@ -77,7 +77,7 @@ static int match_memdev_by_parent(struct device *dev, const void *uport)
 	return 0;
 }
 
-void cxl_cper_handle_prot_err(struct cxl_cper_prot_err_work_data *data)
+static void cxl_cper_handle_prot_err(struct cxl_cper_prot_err_work_data *data)
 {
 	unsigned int devfn = PCI_DEVFN(data->prot_err.agent_addr.device,
 				       data->prot_err.agent_addr.function);
@@ -118,7 +118,6 @@ void cxl_cper_handle_prot_err(struct cxl_cper_prot_err_work_data *data)
 	else
 		cxl_cper_trace_uncorr_prot_err(cxlmd, data->ras_cap);
 }
-EXPORT_SYMBOL_GPL(cxl_cper_handle_prot_err);
 
 static void cxl_cper_prot_err_work_fn(struct work_struct *work)
 {
diff --git a/include/cxl/event.h b/include/cxl/event.h
index 912305bee3bc..894eccbe5cbe 100644
--- a/include/cxl/event.h
+++ b/include/cxl/event.h
@@ -340,6 +340,4 @@ cxl_cper_setup_prot_err_work_data(struct cxl_cper_prot_err_work_data *wd,
 }
 #endif
 
-void cxl_cper_handle_prot_err(struct cxl_cper_prot_err_work_data *wd);
-
 #endif /* _LINUX_CXL_EVENT_H */
-- 
2.54.0

Re: [PATCH v5 13/13] cxl/ras: Make cxl_cper_handle_prot_err() static

[sashiko-bot]

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

Pre-existing issues:
- [High] CXL protocol error handling for memdevs silently fails because `match_memdev_by_parent` incorrectly compares a `struct device *` to a `struct pci_dev *`.
--

commit 4ee43d8db9fe927d9aa6ae55ad362c92db62074c
Author: Dave Jiang <dave.jiang@intel.com>

cxl/ras: Make cxl_cper_handle_prot_err() static

This commit drops the export and declaration of cxl_cper_handle_prot_err()
and makes it static, since its only caller remains in the same file.

> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index 99fb00949c2fa..eceb33bc534b3 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c
> @@ -77,7 +77,7 @@ static int match_memdev_by_parent(struct device *dev, const void *uport)
>  	return 0;
>  }
>  
> -void cxl_cper_handle_prot_err(struct cxl_cper_prot_err_work_data *data)
> +static void cxl_cper_handle_prot_err(struct cxl_cper_prot_err_work_data *data)
>  {

[Severity: High]
This is a pre-existing issue, but does this function silently fail to handle 
memory device protocol errors due to a pointer mismatch?

In cxl_cper_handle_prot_err(), bus_find_device() is called with pdev 
(a struct pci_dev *) as the match data:

	struct device *mem_dev __free(put_device) = bus_find_device(
		&cxl_bus_type, NULL, pdev, match_memdev_by_parent);

However, match_memdev_by_parent() expects uport to match dev->parent, which 
is a struct device *:

static int match_memdev_by_parent(struct device *dev, const void *uport)
{
	if (is_cxl_memdev(dev) && dev->parent == uport)
		return 1;
	return 0;
}

Because the struct device is embedded inside struct pci_dev at a non-zero 
offset, dev->parent will never equal pdev. This causes bus_find_device() to 
always return NULL, which silently ignores the protocol error instead of 
tracing it. 

Should bus_find_device() be passed &pdev->dev instead of pdev?

[ ... ]

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