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

[PATCH v4 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.

v4 answers Tony Luck's question on v3: is there a common early point where an
oversized gdata->error_data_length gets rejected, rather than each caller
checking only "len < sizeof(*foo)"?

cper_estatus_check() walks every section and runs before any handler
on all three paths - the IRQ path via ghes_read_estatus(), the NMI path via
ghes_in_nmi_queue_one_entry(), and bert_print_all() for BERT. Patch 1 bounds
error_data_length there, so the handlers' own checks are lower bounds only, by
design, and ghes_do_proc() and ghes_handle_aer() need nothing extra.

Patch 1 also stops that bound depending on the signed helpers the question
points at. The wrap goes the wrong way to be caught: against the 72-byte v300
header, an error_data_length of 0xffffffb9 makes acpi_hest_get_record_size()
return 1 rather than something huge, so it passes "record_size > data_len" and
acpi_hest_get_next() advances by that 1. Summing in u64 and rejecting what an
int cannot carry removes the dependency.

A record needs a second, independent bound, since cper_estatus_check() sees
the record alone and never the buffer it was read into. That one belongs to
each caller: bert_print_all() bounds against the remaining BERT region, and
extlog_print() gains an ELOG_ENTRY_LEN bound in patch 3.

GHES is the one caller without it - __ghes_read_estatus() copies buf_len bytes
over the header buf_len came from, and never re-checks. No patch here:
reaching it needs something to rewrite the block while block_status is still
set, which the ACPI read-acknowledge handshake forbids, and I have no instance
of firmware doing that. Better as a separate look than a Fixes: tag and a trip
to stable for a window nobody can show is real.

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: Root Error
    Command, Root Error Status and Error Source ID land in
    header_log.prefix[0..2] and print as end-to-end TLP prefixes. Patch 7
    makes this visible in extlog by fixing the guard typo, but the misdecode
    is the same before and after; patches 5 and 12 only stop the
    out-of-bounds reads around it.

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/

Review tags from the v3 thread are carried over. Patches 2, 10 and 13 are new
in v4 and have none. Patches 1 and 11 were reworked, so their v3 Reviewed-by
tags are dropped. Patches 4, 5, 6, 7 and 9 changed in smaller ways and keep
theirs. Patches 3, 8 and 12 are unchanged.

Changes since v3
----------------
- Regrouped into three parts: bound the record, fix and enable the extlog
  paths, then bound each section payload. v3 interleaved them, so patch 1
  checked a section before the patch establishing that contract. No code
  changed.
- Moved the extlog lock inversion fix (patch 4) ahead of the CXL protocol
  error length validation (patch 9). The former deletes
  extlog_cxl_cper_handle_prot_err(), which the v3 order plumbed a new len
  argument through three patches before removing it. No functional change.
- Patch 1: sum in u64 and reject a size the int helpers cannot carry, so the
  choke point no longer depends on acpi_hest_get_record_size() not wrapping
  (Tony Luck). The v3 "< 0" check was dead code, and bounding the u32 alone
  would have left the sum unchecked. Reviewed-by dropped; not the same check.
- New patch 2: reject an error status block length that wraps the u32 sum in
  cper_estatus_len(). A data_length of 0xffffffec makes it read back as 0,
  slipping past the extlog bound in patch 3 and leaving bert_print_all()
  advancing by zero forever (sashiko). Its commit message no longer claims
  extlog already reached cper_estatus_check_header() (sashiko).
- Patch 4: moved the cxl_cper_post_prot_err() declaration inside the
  CONFIG_ACPI_APEI_GHES block in <acpi/ghes.h> (Shuai Xue). The GHES=n plus
  EXTLOG=m failure that prompted it is unreachable, since ACPI_EXTLOG selects
  ACPI_APEI_GHES, but the declaration belongs there anyway.
- Patch 5: dropped a pointless aer alias; the local aer_regs goes straight to
  pci_print_aer().
- Patch 6: warn instead of returning silently on a short PCIe section (Shuai
  Xue), and added the Closes: link v3 omitted.
- Patch 7: corrected the Fixes tag to e778ffefa34d, the commit that added the
  "#ifdef ACPI_APEI_PCIEAER" guard, and dropped its Reported-by; sashiko-bot
  reviewed that patch rather than reporting the typo.
- Patch 9: made the two prot-err length messages distinguishable; both printed
  the same text. The second now reports dvsec_len (Shuai Xue).
- New patch 10: read only validated fields in cper_mem_err_pack(). It copied
  extended, rank, mem_array_handle and mem_dev_handle unconditionally from
  offsets 73 to 79, past the end of the 73-byte UEFI 2.1/2.2 layout (sashiko).
- Patch 11: derive the required length from the claimed validation bits. A
  single size gets it wrong both ways: 73 bytes let a 74 to 79 byte record
  through, so patch 10 could read mem_dev_handle at offset 78 off the end, and
  80 bytes rejected a 76-byte record legitimately carrying rank, or one
  claiming only bank at offset 38 (sashiko).
- New patch 13: make cxl_cper_handle_prot_err() static. Patch 4 removed its
  last external caller.
- Condensed the commit logs and comments throughout; prose only.

Changes since v2
----------------
- Dropped the standalone spin_lock_irqsave() change; a separate issue being
  handled by Terry Bowman.
- New patch 2: reject a section whose error_data_length is out of range in
  cper_estatus_check(), closing the signed-int overflow that let a crafted
  section defeat the per-caller size guards (sashiko).
- New patch 9: apply the same AER buffer sanitization to ghes_handle_aer()
  that patch 4 applies to extlog (sashiko).
- New patch 10: bound the extlog record and run cper_estatus_check() before
  walking its sections, which extlog did not do (sashiko).
- Patch 8: move the memory error length check into ghes_do_proc() so the
  report chain and arch reporter are covered too, and bound against the older
  UEFI 2.1/2.2 layout rather than the full struct (sashiko).
- Reordered so the extlog PCIe path is hardened, and CXL protocol error
  handling deferred to the workqueue, before the typo fix that activates it.

Changes since v1
----------------
- See the v2 posting for the full v1 -> v2 changelog.

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       | 49 +++++++++---------
 drivers/acpi/apei/ghes.c         | 89 +++++++++++++++++++++++++++-----
 drivers/acpi/apei/ghes_helpers.c | 18 ++++++-
 drivers/cxl/core/ras.c           |  3 +-
 drivers/firmware/efi/cper.c      | 53 ++++++++++++++++---
 include/acpi/ghes.h              |  4 ++
 include/cxl/event.h              |  6 +--
 7 files changed, 170 insertions(+), 52 deletions(-)


base-commit: 075b74841bd0065a3bda3440873c747938e69b68
-- 
2.54.0

[PATCH v4 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.

Sum in u64 so the check sees the real size, and reject a size the int
helpers cannot carry, since the walk advances by their return value.

This is the per-section upper bound the later "len < sizeof(*foo)" guards
rely on; 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>
---
v4:
- Do the arithmetic in u64 at the choke point instead of bounding the u32
  error_data_length against data_len, so the check no longer depends on the
  signed helpers in <acpi/ghes.h> behaving (Tony Luck). Bounding the u32
  still left a window when data_length itself was within a header of
  U32_MAX, and it did not stop acpi_hest_get_next() advancing by a wrapped
  int. The v3 "< 0" arm was dead either way, since data_len is unsigned and
  promoted the int back (Tony Luck, Shuai Xue).
- Dropped Alison's and Shuai's Reviewed-by; the check was reworked after
  they reviewed it.
---
 drivers/firmware/efi/cper.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index 06b4fdb59917..ec092729cacc 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,11 +762,21 @@ int cper_estatus_check(const struct acpi_hest_generic_status *estatus)
 	data_len = estatus->data_length;
 
 	apei_estatus_for_each_section(estatus, gdata) {
+		u64 record_size;
+
 		if (acpi_hest_get_size(gdata) > data_len)
 			return -EINVAL;
 
-		record_size = acpi_hest_get_record_size(gdata);
-		if (record_size > data_len)
+		/*
+		 * acpi_hest_get_record_size() sums these as a signed int (see
+		 * <acpi/ghes.h>), which wraps small for a huge
+		 * error_data_length and slips past the check below. Sum in u64,
+		 * and reject what those helpers cannot carry, since the walk
+		 * advances by their return value.
+		 */
+		record_size = (u64)acpi_hest_get_size(gdata) +
+			      gdata->error_data_length;
+		if (record_size > data_len || record_size > INT_MAX)
 			return -EINVAL;
 
 		data_len -= record_size;
-- 
2.54.0

Re: [PATCH v4 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 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Time-of-Check to Time-of-Use (TOCTOU) double-fetch vulnerability in GHES record parsing allows bypassing length validation.
- [Medium] Out-of-bounds read in CPER record validation due to partial trailing sections.
--

--- Patch [1]: [PATCH v4 01/13] efi/cper: Reject CPER records with an out-of-range error_data_length ---
commit b61df2c17060fd5f4040a122aa6b8bc641dbf1f3
Author: Dave Jiang <dave.jiang@intel.com>

efi/cper: Reject CPER records with an out-of-range error_data_length

This patch prevents acpi_hest_get_record_size() from wrapping small when
adding a large error_data_length. By performing the arithmetic in u64, it
ensures the size check against data_len catches oversized records correctly.

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

> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 06b4fdb59917a..ec092729caccb 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)
>  {

[Severity: High]
This is a pre-existing issue, but does cper_estatus_check() lack sufficient
context to prevent an out-of-bounds read if the record is modified
concurrently?

When drivers/acpi/apei/ghes.c:ghes_in_nmi_queue_one_entry() allocates the
buffer, it sizes it exactly to a peeked length. Then it fetches the record a
second time from physical memory via __ghes_read_estatus():

    ghes_copy_tofrom_phys(estatus, buf_paddr, buf_len, 1, fixmap_idx);
    if (cper_estatus_check(estatus)) {

If the data_length is maliciously increased in physical memory between these
fetches, the newly copied header will contain an unvalidated length. Because
cper_estatus_check() doesn't verify the length against the originally peeked
allocation size, the loop below can iterate far beyond the bounds of the
allocated buffer.

Would it be possible to pass max_len into cper_estatus_check() to re-verify
it against the newly fetched data_length?

>  	struct acpi_hest_generic_data *gdata;
> -	unsigned int data_len, record_size;
> +	unsigned int data_len;
>  	int rc;

[ ... ]

> @@ -762,11 +762,21 @@ int cper_estatus_check(const struct acpi_hest_generic_status *estatus)
>  	data_len = estatus->data_length;
>  
>  	apei_estatus_for_each_section(estatus, gdata) {
> +		u64 record_size;
> +
>  		if (acpi_hest_get_size(gdata) > data_len)
>  			return -EINVAL;

[Severity: Medium]
This is a pre-existing issue, but does this loop safely handle trailing
partial sections?

If a malformed CPER record leaves fewer than the required bytes for the
header, calling acpi_hest_get_size(gdata) here will read gdata->revision
beyond the end of the estatus payload before checking against data_len.

Also, in include/acpi/ghes.h, apei_estatus_for_each_section() advances using
acpi_hest_get_next(section). Since the loop update calls
acpi_hest_get_record_size() which calls acpi_hest_get_size(), it could
similarly read out of bounds before the loop terminates if the remaining
data_len is too small.

Could data_len be checked against the minimum section header size before
attempting to access gdata fields?

> -		record_size = acpi_hest_get_record_size(gdata);
> -		if (record_size > data_len)
> +		/*
> +		 * acpi_hest_get_record_size() sums these as a signed int (see

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

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

[Jonathan Cameron]

On Mon, 24 Aug 2026 10:49:24 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> 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.
> 
> Sum in u64 so the check sees the real size, and reject a size the int
> helpers cannot carry, since the walk advances by their return value.
> 
> This is the per-section upper bound the later "len < sizeof(*foo)" guards
> rely on; 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>
I know I'm late to the discussion but maybe it would just be simpler to
use check_add_overflow()?

> ---
> v4:
> - Do the arithmetic in u64 at the choke point instead of bounding the u32
>   error_data_length against data_len, so the check no longer depends on the
>   signed helpers in <acpi/ghes.h> behaving (Tony Luck). Bounding the u32
>   still left a window when data_length itself was within a header of
>   U32_MAX, and it did not stop acpi_hest_get_next() advancing by a wrapped
>   int. The v3 "< 0" arm was dead either way, since data_len is unsigned and
>   promoted the int back (Tony Luck, Shuai Xue).
> - Dropped Alison's and Shuai's Reviewed-by; the check was reworked after
>   they reviewed it.
> ---
>  drivers/firmware/efi/cper.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 06b4fdb59917..ec092729cacc 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,11 +762,21 @@ int cper_estatus_check(const struct acpi_hest_generic_status *estatus)
>  	data_len = estatus->data_length;
>  
>  	apei_estatus_for_each_section(estatus, gdata) {
> +		u64 record_size;
> +
>  		if (acpi_hest_get_size(gdata) > data_len)
>  			return -EINVAL;
>  
> -		record_size = acpi_hest_get_record_size(gdata);
> -		if (record_size > data_len)
> +		/*
> +		 * acpi_hest_get_record_size() sums these as a signed int (see
> +		 * <acpi/ghes.h>), which wraps small for a huge
> +		 * error_data_length and slips past the check below. Sum in u64,
> +		 * and reject what those helpers cannot carry, since the walk
> +		 * advances by their return value.
> +		 */
> +		record_size = (u64)acpi_hest_get_size(gdata) +
> +			      gdata->error_data_length;

I'm late to the game obviously and what you have works but could this have
used some explicit overflow checking?  Something like

		if (check_add_overflow(acpi_hest_get_size(gdata),
				       gdata->error_data_length, &record_size))
			return -EINVAL;

		if (record_size > data_len)
			return -EINVAL;

That uses the compiler __builtin_add_overflow() which checks if the infinite
precision result of the sum of the parameters would have wrapped when written
to the output one.

I think you could then drop the earlier check as well as
record_size is at least as big as acpi_hest_get_size() and we know there
was no wrap around.

	
> +		if (record_size > data_len || record_size > INT_MAX)
>  			return -EINVAL;
>  
>  		data_len -= record_size;

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

[Dave Jiang]

On 8/24/26 2:57 PM, Jonathan Cameron wrote:
> On Mon, 24 Aug 2026 10:49:24 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> 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.
>>
>> Sum in u64 so the check sees the real size, and reject a size the int
>> helpers cannot carry, since the walk advances by their return value.
>>
>> This is the per-section upper bound the later "len < sizeof(*foo)" guards
>> rely on; 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>
> I know I'm late to the discussion but maybe it would just be simpler to
> use check_add_overflow()?
> 
>> ---
>> v4:
>> - Do the arithmetic in u64 at the choke point instead of bounding the u32
>>   error_data_length against data_len, so the check no longer depends on the
>>   signed helpers in <acpi/ghes.h> behaving (Tony Luck). Bounding the u32
>>   still left a window when data_length itself was within a header of
>>   U32_MAX, and it did not stop acpi_hest_get_next() advancing by a wrapped
>>   int. The v3 "< 0" arm was dead either way, since data_len is unsigned and
>>   promoted the int back (Tony Luck, Shuai Xue).
>> - Dropped Alison's and Shuai's Reviewed-by; the check was reworked after
>>   they reviewed it.
>> ---
>>  drivers/firmware/efi/cper.c | 16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
>> index 06b4fdb59917..ec092729cacc 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,11 +762,21 @@ int cper_estatus_check(const struct acpi_hest_generic_status *estatus)
>>  	data_len = estatus->data_length;
>>  
>>  	apei_estatus_for_each_section(estatus, gdata) {
>> +		u64 record_size;
>> +
>>  		if (acpi_hest_get_size(gdata) > data_len)
>>  			return -EINVAL;
>>  
>> -		record_size = acpi_hest_get_record_size(gdata);
>> -		if (record_size > data_len)
>> +		/*
>> +		 * acpi_hest_get_record_size() sums these as a signed int (see
>> +		 * <acpi/ghes.h>), which wraps small for a huge
>> +		 * error_data_length and slips past the check below. Sum in u64,
>> +		 * and reject what those helpers cannot carry, since the walk
>> +		 * advances by their return value.
>> +		 */
>> +		record_size = (u64)acpi_hest_get_size(gdata) +
>> +			      gdata->error_data_length;
> 
> I'm late to the game obviously and what you have works but could this have
> used some explicit overflow checking?  Something like

Not late at all.

> 
> 		if (check_add_overflow(acpi_hest_get_size(gdata),
> 				       gdata->error_data_length, &record_size))
> 			return -EINVAL;
> 
> 		if (record_size > data_len)
> 			return -EINVAL;
> 
> That uses the compiler __builtin_add_overflow() which checks if the infinite
> precision result of the sum of the parameters would have wrapped when written
> to the output one.
> 
> I think you could then drop the earlier check as well as
> record_size is at least as big as acpi_hest_get_size() and we know there
> was no wrap around.
> 

Yup I'll do that.

DJ

> 	
>> +		if (record_size > data_len || record_size > INT_MAX)
>>  			return -EINVAL;
>>  
>>  		data_len -= record_size;
> 

[PATCH v4 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")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v4:
- New patch. sashiko-bot's review of v4 pointed out that the u32 sum in
  cper_estatus_len() wraps, which bypasses the bounds added by the
  surrounding patches.
---
 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 ec092729cacc..ea1c999089bc 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 v4 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] Missing bounds check before reading section header fields in cper_estatus_check(), leading to an out-of-bounds read.
--

--- Patch [2]: [PATCH v4 02/13] efi/cper: Reject an error status block length that wraps a u32 ---
commit 50e02caac680576dbb8a9d80c6f2e251318a9c08
Author: Dave Jiang <dave.jiang@intel.com>

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

This commit prevents an integer overflow in cper_estatus_len() by adding
bounds checks to cper_estatus_check_header(). The new checks ensure that a
firmware-controlled data length cannot wrap a u32, avoiding infinite loops
in callers like bert_print_all().

> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index ec092729caccb..ea1c999089bc1 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);

[Severity: High]
This is a pre-existing issue, but there appears to be a missing bounds
check in cper_estatus_check() that could lead to an out-of-bounds read.

The function iterates over error status sections:

drivers/firmware/efi/cper.c:cper_estatus_check() {
    ...
    apei_estatus_for_each_section(estatus, gdata) {
        u64 record_size;

        if (acpi_hest_get_size(gdata) > data_len)
            return -EINVAL;
    ...
}

The loop condition in apei_estatus_for_each_section() only checks that the
next section starts within the total data_length. If a malformed record
leaves a remaining data_len greater than 0 but less than
sizeof(struct acpi_hest_generic_data), the loop will still process it.

Inside the loop, the very first operation is acpi_hest_get_size(gdata). This
internally evaluates gdata->revision via acpi_hest_get_version():

include/acpi/ghes.h:acpi_hest_get_version() {
    return gdata->revision >> 8;
}

Since gdata->revision is at offset 20, reading it when the remaining
data_len is too small (for example, if only 10 bytes remain) reads memory
beyond the end of estatus->data_length.

Could this cause a kernel panic if the read crosses into an unmapped page?

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

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

[Jonathan Cameron]

On Mon, 24 Aug 2026 10:49:25 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> 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")
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Seems fine to me and to use the overflow.h stuff here we'd have to invent some
local variables which rather outweighs their documentation benefit.

Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

> ---
> v4:
> - New patch. sashiko-bot's review of v4 pointed out that the u32 sum in
>   cper_estatus_len() wraps, which bypasses the bounds added by the
>   surrounding patches.
> ---
>  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 ec092729cacc..ea1c999089bc 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);

[PATCH v4 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>
---
 drivers/acpi/acpi_extlog.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index 7ad3b36013cc..9ad0052aa20c 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -208,6 +208,10 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
 
 	tmp = (struct acpi_hest_generic_status *)elog_buf;
 
+	/* Keep the firmware-controlled data_length inside elog_buf. */
+	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 v4 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] cper_estatus_check() fails to verify remaining data_len before calling acpi_hest_get_size(gdata), leading to an out-of-bounds read of gdata->revision.
- [High] Missing bounds check on physical address provided by firmware, leading to arbitrary memory read and write.
--

--- Patch [3]: [PATCH v4 03/13] ACPI: extlog: Validate elog record length before walking sections ---
commit b1da2ee38edf690076a81fda05c21b313452f06b
Author: Dave Jiang <dave.jiang@intel.com>

ACPI: extlog: Validate elog record length before walking sections

extlog_print() copies a fixed 4096 bytes from the elog record into
elog_buf and walks the sections using data_length. This patch rejects
records longer than ELOG_ENTRY_LEN and runs cper_estatus_check() to
validate the records and prevent walking past the buffer.

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..9ad0052aa20c9 100644
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c
> @@ -208,6 +208,10 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
>  

[Severity: High]
This is a pre-existing issue, but is the physical address provided by firmware
validated before it is accessed here?

Looking at extlog_elog_entry_check() called earlier in this function:

drivers/acpi/acpi_extlog.c:extlog_elog_entry_check() {
    ...
    data &= EXT_ELOG_ENTRY_MASK;
    estatus = (struct acpi_hest_generic_status *)ELOG_ENTRY_ADDR(data);

    /* if no valid data in elog entry, just return */
    if (estatus->block_status == 0)
        return NULL;

The virtual address computed by ELOG_ENTRY_ADDR() doesn't appear to be
checked against the mapped bounds.

Furthermore, in extlog_print(), this unvalidated pointer is read from and
written to:

drivers/acpi/acpi_extlog.c:extlog_print() {
    ...
    memcpy(elog_buf, (void *)estatus, ELOG_ENTRY_LEN);
    /* clear record status to enable BIOS to update it again */
    estatus->block_status = 0;

Could a malformed firmware L1 directory provide an out-of-bounds physical
address, leading to arbitrary memory read and write when extlog_print() copies
the buffer and clears block_status?

>  	tmp = (struct acpi_hest_generic_status *)elog_buf;
>  
> +	/* Keep the firmware-controlled data_length inside elog_buf. */
> +	if (cper_estatus_len(tmp) > ELOG_ENTRY_LEN || cper_estatus_check(tmp))

[Severity: High]
This is a pre-existing issue, but does cper_estatus_check() have an
out-of-bounds read when verifying the sections?

Looking at cper_estatus_check():

drivers/firmware/efi/cper.c:cper_estatus_check() {
    ...
    apei_estatus_for_each_section(estatus, gdata) {
        ...
        if (acpi_hest_get_size(gdata) > data_len)
            return -EINVAL;

And acpi_hest_get_size() calls acpi_hest_get_version(), which unconditionally
reads gdata->revision:

include/acpi/ghes.h:acpi_hest_get_size() {
    ...
    if (acpi_hest_get_version(gdata) >= 3)

If a firmware-provided malformed record contains a section where the
remaining data_len is less than the size of the section header, wouldn't
this dereference memory past the end of the payload?

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

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

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

[Jonathan Cameron]

On Mon, 24 Aug 2026 10:49:26 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> 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>
> ---
>  drivers/acpi/acpi_extlog.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> index 7ad3b36013cc..9ad0052aa20c 100644
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c
> @@ -208,6 +208,10 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
>  
>  	tmp = (struct acpi_hest_generic_status *)elog_buf;
>  
> +	/* Keep the firmware-controlled data_length inside elog_buf. */
> +	if (cper_estatus_len(tmp) > ELOG_ENTRY_LEN || cper_estatus_check(tmp))

Why this order?  To me checking if we are in crazy world (overflow) before
doing anything with the overflowed value makes more sense. So that would be swapping
the two conditions.

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

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

[Dave Jiang]

On 8/24/26 3:28 PM, Jonathan Cameron wrote:
> On Mon, 24 Aug 2026 10:49:26 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> 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>
>> ---
>>  drivers/acpi/acpi_extlog.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
>> index 7ad3b36013cc..9ad0052aa20c 100644
>> --- a/drivers/acpi/acpi_extlog.c
>> +++ b/drivers/acpi/acpi_extlog.c
>> @@ -208,6 +208,10 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
>>  
>>  	tmp = (struct acpi_hest_generic_status *)elog_buf;
>>  
>> +	/* Keep the firmware-controlled data_length inside elog_buf. */
>> +	if (cper_estatus_len(tmp) > ELOG_ENTRY_LEN || cper_estatus_check(tmp))
> 
> Why this order?  To me checking if we are in crazy world (overflow) before
> doing anything with the overflowed value makes more sense. So that would be swapping
> the two conditions.

Swapping it lets cper_estatus_check() walk the sections with data_length unbounded relative to elog_buf. elog_buf is a 4k buffer via kmalloc(ELOG_ENTRY_LEN). cper_estatus_check() iterates sections bounded by data_length, over a fixed kmalloc(ELOG_ENTRY_LEN) of 4096 bytes, reading gdata->revision at +20 and gdata->error_data_length at +24 each time. In the current order it only runs once cper_estatus_len() <= 4096 has passed, which caps data_length at 4076. Swapped, the only thing standing before the walk is cper_estatus_check_header(), and that admits data_length up to 4294967275.

Expand the comment to:

	/*
	 * 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.
	 */

DJ

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

[PATCH v4 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>
Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v4:
- Moved the cxl_cper_post_prot_err() declaration inside the
  CONFIG_ACPI_APEI_GHES block (Shuai Xue).
---
 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 9ad0052aa20c..f6e3da4e13e7 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)
 {
@@ -239,8 +222,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 v4 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] Inconsistent IRQ protection on `cxl_cper_prot_err_work_lock` and `cxl_cper_work_lock` creates a hardirq lock inversion deadlock.
--

--- Patch [4]: [PATCH v4 04/13] ACPI: extlog: Defer CXL protocol error handling to avoid lock inversion ---
commit 720f77fd10f559d90d89b27d71e19c5f7b332287
Author: Dave Jiang <dave.jiang@intel.com>
ACPI: extlog: Defer CXL protocol error handling to avoid lock inversion

This commit defers CXL protocol error handling to a workqueue to avoid an
AB-BA deadlock during probe and MCE handling. It exports
cxl_cper_post_prot_err() from ghes.c and calls it instead of processing
synchronously in extlog_print().

Closes: 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
> @@ -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)
>  {

[Severity: High]
This is a pre-existing issue, but is it possible for a deadlock to occur if
cxl_cper_post_prot_err() interrupts process context?

Looking at drivers/acpi/apei/ghes.c, cxl_cper_register_prot_err_work()
acquires the lock without disabling interrupts:

    guard(spinlock)(&cxl_cper_prot_err_work_lock);

If a CXL protocol error occurs concurrently on the same CPU, the interrupt
or MCE handler will call cxl_cper_post_prot_err(), which tries to acquire
the same lock using:

    guard(spinlock_irqsave)(&cxl_cper_prot_err_work_lock);

Since the lock is already held by the interrupted process, will the interrupt
handler spin forever causing a hard lockup?

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

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

[Jonathan Cameron]

On Mon, 24 Aug 2026 10:49:27 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> 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>
> Assisted-by: Claude:claude-sonnet-4-6
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Makes sense to standardize irrespective of all the other reasons!
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

[PATCH v4 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 aer_info into a zeroed local struct aer_capability_regs and clear
header_len and flit before passing it on.

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>
---
v4:
- Dropped the now-pointless aer pointer alias.
---
 drivers/acpi/acpi_extlog.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index f6e3da4e13e7..6d5532ec0920 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,11 @@ 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;
+
+	memcpy(&aer_regs, pcie_err->aer_info, sizeof(pcie_err->aer_info));
+	aer_regs.header_log.header_len = 0;
+	aer_regs.header_log.flit = false;
+
 	domain = pcie_err->device_id.segment;
 	bus = pcie_err->device_id.bus;
 	devfn = PCI_DEVFN(pcie_err->device_id.device,
@@ -158,7 +162,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 v4 05/13] ACPI: extlog: Avoid populating software AER metadata from raw hardware buffer

[sashiko-bot]

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

Pre-existing issues:
- [High] Uninitialized AER metadata and invalid MMIO reads lead to an unbounded stack out-of-bounds read in CXL error handling.
--

--- Patch [5]: [PATCH v4 05/13] ACPI: extlog: Avoid populating software AER metadata from raw hardware buffer ---
commit 92c75de48c08c6f2371a87542f1e783f6c2d32a1
Author: Dave Jiang <dave.jiang@intel.com>

ACPI: extlog: Avoid populating software AER metadata from raw hardware buffer

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 aer_info into a zeroed local struct aer_capability_regs and clear
header_len and flit before passing it on.

Closes: 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
> index f6e3da4e13e78..6d5532ec09208 100644
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c
[ ... ]
> @@ -149,7 +149,11 @@ 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;
> +
> +	memcpy(&aer_regs, pcie_err->aer_info, sizeof(pcie_err->aer_info));
> +	aer_regs.header_log.header_len = 0;
> +	aer_regs.header_log.flit = false;

[Severity: High]
This isn't a bug introduced by this patch, but does the same pattern exist in
the CXL subsystem in drivers/cxl/core/ras_rch.c?

In cxl_rch_get_aer_info(), it appears to read raw MMIO directly into the
software structure, which could populate software-only fields like
header_len with unmapped MMIO values (0xFFFFFFFF):

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

Then in cxl_handle_rdport_errors(), this corrupted stack-allocated struct
is passed to pci_print_aer():

drivers/cxl/core/ras_rch.c:cxl_handle_rdport_errors() {
    ...
    struct aer_capability_regs aer_regs;
    ...
    pci_print_aer(pdev, severity, &aer_regs);
    ...
}

Could this lead to the same unbounded stack out-of-bounds read when
pcie_print_tlp_log() uses the garbage header_len to loop over the log->dw
array?

> +
>  	domain = pcie_err->device_id.segment;
>  	bus = pcie_err->device_id.bus;
>  	devfn = PCI_DEVFN(pcie_err->device_id.device,

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

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

[Jonathan Cameron]

On Mon, 24 Aug 2026 10:49:28 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> 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 aer_info into a zeroed local struct aer_capability_regs and clear
> header_len and flit before passing it on.

The existing code is very odd. It might be nice to clean it up more
generally so that we can handle the rest of the aer_info buffer
via a definition that matches the hardware spec. 

For now I'd be tempted to just copy the bit of the structure that
is matching the spec - side effect being the rest ends up as zero
including the two things you clear.

So just copy to first 4 dw of the header log. 

Jonathan



> 
> 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>
> ---
> v4:
> - Dropped the now-pointless aer pointer alias.
> ---
>  drivers/acpi/acpi_extlog.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> index f6e3da4e13e7..6d5532ec0920 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,11 @@ 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;
> +
> +	memcpy(&aer_regs, pcie_err->aer_info, sizeof(pcie_err->aer_info));
> +	aer_regs.header_log.header_len = 0;
> +	aer_regs.header_log.flit = false;
> +
>  	domain = pcie_err->device_id.segment;
>  	bus = pcie_err->device_id.bus;
>  	devfn = PCI_DEVFN(pcie_err->device_id.device,
> @@ -158,7 +162,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
>  }

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

[Dave Jiang]

On 8/24/26 4:05 PM, Jonathan Cameron wrote:
> On Mon, 24 Aug 2026 10:49:28 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> 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 aer_info into a zeroed local struct aer_capability_regs and clear
>> header_len and flit before passing it on.
> 
> The existing code is very odd. It might be nice to clean it up more
> generally so that we can handle the rest of the aer_info buffer
> via a definition that matches the hardware spec. 
> 
> For now I'd be tempted to just copy the bit of the structure that
> is matching the spec - side effect being the rest ends up as zero
> including the two things you clear.
> 
> So just copy to first 4 dw of the header log. 

Ok will do.

> 
> Jonathan
> 
> 
> 
>>
>> 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>
>> ---
>> v4:
>> - Dropped the now-pointless aer pointer alias.
>> ---
>>  drivers/acpi/acpi_extlog.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
>> index f6e3da4e13e7..6d5532ec0920 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,11 @@ 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;
>> +
>> +	memcpy(&aer_regs, pcie_err->aer_info, sizeof(pcie_err->aer_info));
>> +	aer_regs.header_log.header_len = 0;
>> +	aer_regs.header_log.flit = false;
>> +
>>  	domain = pcie_err->device_id.segment;
>>  	bus = pcie_err->device_id.bus;
>>  	devfn = PCI_DEVFN(pcie_err->device_id.device,
>> @@ -158,7 +162,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
>>  }
> 

[PATCH v4 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>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v4:
- Warn instead of returning silently, matching the other length
  rejections in the series (Shuai Xue).
- Added the Closes: link to the sashiko report, which v3 omitted.
---
 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 6d5532ec0920..3aec73187b51 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;
@@ -231,7 +237,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

Re: [PATCH v4 06/13] ACPI: extlog: Validate PCIe 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:
- [Medium] Structure layout mismatch when copying raw hardware AER capabilities into `struct aer_capability_regs`, corrupting TLP prefix logs and misaligning fields.
--

--- Patch [6]: [PATCH v4 06/13] ACPI: extlog: Validate PCIe error section length before payload access ---
commit 0c68ab7e511cca4ed3e0656d8522141bd2eb2d1b
Author: Dave Jiang <dave.jiang@intel.com>

ACPI: extlog: Validate PCIe error section length before payload access

extlog_print_pcie() reads fields and copies the aer_info buffer without
checking that the error data length is big enough for a struct cper_sec_pcie.
This commit adds a check to reject sections too small to hold the record
before touching any field.

Closes: 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
> index 6d5532ec09208..3aec73187b519 100644
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c
[ ... ]
> @@ -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;

[Severity: Medium]
This is a pre-existing issue, but does copying raw hardware AER capabilities
into struct aer_capability_regs corrupt TLP prefix logs and misalign fields?

Looking at extlog_print_pcie() right below this hunk:

drivers/acpi/acpi_extlog.c:extlog_print_pcie() {
    ...
	aer_severity = cper_severity_to_aer(severity);

	memcpy(&aer_regs, pcie_err->aer_info, sizeof(pcie_err->aer_info));
    ...
}

The memcpy blindly assumes struct aer_capability_regs matches the hardware
register layout, but the struct's header_log field is now much larger. This
causes subsequent hardware fields like root_command (offset 44) to overwrite
aer_regs.header_log.prefix.

Because struct aer_capability_regs uses a 60-byte struct pcie_tlp_log for its
header_log instead of the 16 bytes defined by the hardware specification, all
fields after header_log are misaligned. This occurs whenever a valid PCIe AER
CPER record is processed.

The GHES handler in drivers/acpi/apei/ghes.c:ghes_handle_aer() shares the
identical layout mismatch vulnerability:

drivers/acpi/apei/ghes.c:ghes_handle_aer() {
    ...
		memcpy(aer_info, pcie_err->aer_info, sizeof(pcie_err->aer_info));
    ...
}

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

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

[Jonathan Cameron]

On Mon, 24 Aug 2026 10:49:29 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> 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>
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

[PATCH v4 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>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v4:
- Corrected the Fixes tag: e778ffefa34d added the "#ifdef ACPI_APEI_PCIEAER"
  guard, not 95350effc3ad.
- Dropped Reported-by: sashiko-bot; it reviewed this patch rather than
  reporting the typo.
---
 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 3aec73187b51..ebedf3b136a8 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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] `cxl_rch_get_aer_info()` performs a MMIO read that exceeds the physical footprint of the AER capability, leading to potential out-of-bounds physical MMIO accesses.
- [High] Memory leak from the fixed-size `ghes_estatus_pool` when the AER recovery queue is full.
- [Medium] Activating the AER handler causes corrupted diagnostic output because `struct aer_capability_regs` no longer maps 1:1 to the hardware AER capability layout.
- [Medium] Concurrent lockless updates to u64 AER stats can lead to dropped counts.
--

--- Patch [7]: [PATCH v4 07/13] ACPI: extlog: Fix CONFIG_ACPI_APEI_PCIEAER guard typo ---
commit 6574c6d62d519f2f7bce0f053a2b55a5d05016ce
Author: Dave Jiang <dave.jiang@intel.com>
ACPI: extlog: Fix CONFIG_ACPI_APEI_PCIEAER guard typo

This commit fixes a typo in the preprocessor guard by adding the missing
CONFIG_ prefix. This change enables the previously compiled-out extlog
PCIe aer handling code.

> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> index 3aec73187b519..ebedf3b136a88 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

[Severity: Medium]
This isn't a bug introduced by this patch, but activating this aer handler
exposes a preexisting issue. Does the memcpy() in this block overwrite
misaligned software fields?

drivers/acpi/acpi_extlog.c:extlog_print_pcie() {
    ...
    memcpy(&aer_regs, pcie_err->aer_info, sizeof(pcie_err->aer_info));
    ...
}

Since struct aer_capability_regs contains struct pcie_tlp_log which was
expanded with software-only fields (header_len, flit), this pushes the
offset of subsequent fields beyond their hardware offsets. 

Copying the 96-byte hardware aer capability directly into the structure
misaligns these later fields, which could lead to corrupted diagnostic
output when pci_print_aer() reads them.

>  	struct aer_capability_regs aer_regs = {};
>  	struct pci_dev *pdev;
>  	unsigned int devfn;

[Severity: High]
This is a pre-existing issue, but I noticed a potential out-of-bounds mmio
read execution in the cxl core related to these aer structures.

drivers/cxl/core/ras_rch.c:cxl_rch_get_aer_info() {
    ...
    aer_regs_buf[n] = readl(aer_base + n * sizeof(u32));
    ...
}

Can this loop read past the physical footprint of the aer capability?

Because sizeof(struct aer_capability_regs) grew due to the addition of
header_len and flit, reading this size from the aer_base mmio address
could fetch past the end of the physical aer capability registers.

This might cause bus hangs or trigger kasan violations.

[Severity: Medium]
This is a pre-existing issue, but I noticed potential lockless increment
operations on a 64-bit statistic in the aer core.

drivers/pci/pcie/aer.c:pci_dev_aer_stats_incr() {
    ...
    aer_info->dev_total_cor_errs++;
    ...
}

Can concurrent lockless updates to these u64 aer stats lead to dropped
counts? 

Since this function is called locklessly from multiple contexts (such as
aer interrupt handlers and nmi-context ghes/extlog handlers), concurrent
read-modify-write operations on u64 variables without locking or atomics
could lead to dropped counts or torn reads/writes on 32-bit architectures.

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be a memory
leak in the aer core failure path when the recovery queue is full.

drivers/pci/pcie/aer.c:aer_recover_queue() {
    ...
    if (!kfifo_in_spinlocked(&aer_recover_ring, &entry, 1,
                             &aer_recover_ring_lock)) {
        pr_err("buffer overflow in recovery for %04x:%02x:%02x.%x\n",
               ...);
        return;
    }
    ...
}

Does this failure path drop allocated memory without freeing it?

ghes_handle_aer() allocates memory from the fixed-size ghes_estatus_pool
and passes it to aer_recover_queue(). 

If kfifo_in_spinlocked() fails, the function prints an error and returns. 

Since the pool is small and fixed-size, repeated overflow events could
permanently exhaust it, causing future hardware errors to be silently dropped.

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

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

[Jonathan Cameron]

On Mon, 24 Aug 2026 10:49:30 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> 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>
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Yikes.

Raises some testing questions. I guess a last minute edit perhaps.

Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>


> ---
> v4:
> - Corrected the Fixes tag: e778ffefa34d added the "#ifdef ACPI_APEI_PCIEAER"
>   guard, not 95350effc3ad.
> - Dropped Reported-by: sashiko-bot; it reviewed this patch rather than
>   reporting the typo.
> ---
>  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 3aec73187b51..ebedf3b136a8 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;

[PATCH v4 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>
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 v4 08/13] ACPI: APEI: GHES: Bound CXL event record copy to the firmware section length

[Jonathan Cameron]

On Mon, 24 Aug 2026 10:49:31 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> 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>
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

[PATCH v4 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>
Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v4:
- Made the two prot-err length messages distinguishable; the second now
  reports dvsec_len (Shuai Xue).
- Reordered after the extlog lock inversion fix, so the new len argument
  goes only to cxl_cper_post_prot_err() rather than also to the
  now-deleted extlog_cxl_cper_handle_prot_err().
---
 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 ebedf3b136a8..d32306c36511 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -233,7 +233,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 v4 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] `cxl_cper_print_prot_err()` reads out-of-bounds memory when dumping the CXL DVSEC and Error Log if `prot_err->dvsec_len` exceeds the actual error record data length.
--

--- Patch [9]: [PATCH v4 09/13] ACPI: APEI: GHES: Validate CXL protocol error section length before RAS cap copy ---
commit 86649d90c05d1f4d84b3cdb3e62e7482bd0800ac
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 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.

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_helpers.c b/drivers/acpi/apei/ghes_helpers.c
> index bc7111b740afb..df41b993f4130 100644
> --- a/drivers/acpi/apei/ghes_helpers.c
> +++ b/drivers/acpi/apei/ghes_helpers.c

[ ... ]

> @@ -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;
> +	}
> +

[Severity: High]
This is a pre-existing issue, but does this new bounds validation cover the
synchronous print path during CXL protocol error handling?

While this patch adds bounds validation via cxl_cper_sec_prot_err_valid()
for the workqueue execution path, the synchronous print path appears to
completely bypass this new validation.

Earlier in the flow, ghes_proc() calls ghes_print_estatus(), which leads to
cper_estatus_print_section():

drivers/firmware/efi/cper.c:cper_estatus_print_section() {
    ...
		if (gdata->error_data_length >= sizeof(*prot_err))
			cxl_cper_print_prot_err(newpfx, prot_err);
		else
			goto err_section_too_small;
    ...
}

This checks that the error section is large enough to hold the base header
structure, but fails to validate that it can also hold the variable-length
DVSEC or the RAS capabilities before passing it to the print routine.

The untrusted dvsec_len is then used directly in cxl_cper_print_prot_err(),
leading to an out-of-bounds memory read:

drivers/firmware/efi/cper_cxl.c:cxl_cper_print_prot_err() {
    ...
		print_hex_dump(pfx, "", DUMP_PREFIX_OFFSET, 16, 4, (prot_err + 1),
			       prot_err->dvsec_len, 0);
    ...
		cxl_ras = (struct cxl_ras_capability_regs *)((long)prot_err + size);
		pr_info("%s cxl_ras_uncor_status: 0x%08x", pfx,
			cxl_ras->uncor_status);
    ...
}

Could firmware still trigger an unhandled page fault or leak kernel memory
to the system log by providing an artificially large dvsec_len that reaches
this print path?

>  	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))

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

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

[Jonathan Cameron]

On Mon, 24 Aug 2026 10:49:32 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> 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>
> Assisted-by: Claude:claude-sonnet-4-6
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

[PATCH v4 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>
---
v4:
- New patch. sashiko-bot pointed out that bounding the memory error
  section at the 73-byte layout still leaves cper_mem_err_pack() reading
  offsets 73 to 79.
---
 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 ea1c999089bc..6c64a0f06a4e 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 v4 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:
- [High] Out-of-bounds read in cper_mem_err_pack() triggered by cper_print_mem() for records of length 74-79 bytes.
--

--- Patch [10]: [PATCH v4 10/13] efi/cper: Read only validated fields in cper_mem_err_pack() ---
commit bef2c0319d994ade71f221a3c685230d96b2ab0b
Author: Dave Jiang <dave.jiang@intel.com>

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

This patch updates cper_mem_err_pack() to only copy extended memory error
fields when their respective validation bits are set. This addresses
out-of-bounds reads on older UEFI 2.1/2.2 73-byte layouts where these
fields don't exist. By zeroing unvalidated fields, it also prevents reading
uninitialized stack data.

Closes: 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 ea1c999089bc1..6c64a0f06a4e6 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: High]
This is a pre-existing issue, but if firmware provides a CPER record with a
length between 74 and 79 bytes and sets validation bits for fields beyond its
length, can this still cause an out-of-bounds read?

In drivers/firmware/efi/cper.c:cper_print_mem(), the validation bits are only
sanitized if the length is exactly 73 bytes:

	if (len == sizeof(struct cper_sec_mem_err_old) &&
	    (mem->validation_bits & ~(CPER_MEM_VALID_RANK_NUMBER - 1))) {
		...

If the length is 74 to 79 bytes, this sanitization is bypassed.

Then, here in cper_mem_err_pack(), we trust the validation bits without length
context. If the CPER record is at the end of the mapped estatus block and
aligned at a page boundary, could reading mem_dev_handle cross into an
unmapped page and result in a kernel panic?

In drivers/acpi/apei/ghes.c:ghes_proc(), ghes_print_estatus() is called before
length validation is performed in ghes_do_proc():

	if (ghes_print_estatus(NULL, ghes->generic, estatus))
		ghes_estatus_cache_add(ghes->generic, estatus);
	}
	ghes_do_proc(ghes, estatus);

Does cper_mem_err_pack() need to consider the length of the record, or should
cper_print_mem() be updated to handle lengths between 74 and 79 bytes?

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

[PATCH v4 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.
cper_print_mem() tests the whole range above CPER_MEM_VALID_RANK_NUMBER
against the 73-byte size instead, which both admits 74 to 79 and rejects a
record claiming only "bank".

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>
---
v4:
- Derive the required length from the claimed validation bits instead of
  testing the whole mask against one size. The blanket form rejected a
  76-byte record that legitimately carries rank, and any short record
  claiming only bank (sashiko).
- Dropped Alison's and Shuai's Reviewed-by; the check gained a helper and
  is no longer the patch they reviewed.
---
 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 v4 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] Unfixed out-of-bounds read and valid record rejection in `cper_print_mem()` for CPER_SEC_PLATFORM_MEM records.
--

--- Patch [11]: [PATCH v4 11/13] ACPI: APEI: GHES: Validate memory error section length before payload access ---
commit 550e022dfc62676e66fb5e0818106e122ddfcbf2
Author: Dave Jiang <dave.jiang@intel.com>

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

This commit adds memory error section length validation to ghes_do_proc() 
to prevent out-of-bounds reads when handling CPER_SEC_PLATFORM_MEM payloads. 
It checks the validation bits to determine the minimum required length.

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

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c

[ ... ]

> @@ -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;
> +			}

[Severity: High]
This is a pre-existing issue, but does this validation happen too late to
prevent out-of-bounds reads when printing the error?

Looking at ghes_proc(), it prints the error status before calling
ghes_do_proc():

drivers/acpi/apei/ghes.c:ghes_proc() {
    ...
    if (!ghes_estatus_cached(estatus)) {
        if (ghes_print_estatus(NULL, ghes->generic, estatus))
            ghes_estatus_cache_add(ghes->generic, estatus);
    }
    ghes_do_proc(ghes, estatus);
    ...
}

The call chain goes through ghes_print_estatus() ->
cper_estatus_print_section() -> cper_print_mem(), which has a strict
73-byte length check that allows lengths 74-79 to bypass validation:

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;
    }
    ...
}

If a malformed payload has a length between 74 and 79 bytes and claims
extended fields, does this lead to an out-of-bounds read during
cper_mem_err_pack() before the new checks in ghes_do_proc() can reject it?

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;
    ...
}

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

[PATCH v4 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. It also fills the software-only header_len and flit
fields of the embedded struct pcie_tlp_log from raw firmware bytes, and
pcie_print_tlp_log() uses both to bound a loop over dw[], so a large value
walks past the array. Nothing checks that the section can hold a struct
cper_sec_pcie either.

Validate error_data_length, zero the destination, bound the copy to the
96-byte source, and clear header_len and flit, mirroring the
extlog_print_pcie() fix.

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>
---
 drivers/acpi/apei/ghes.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 0cc1e6383635..d6643127a2df 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,22 @@ 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));
+
+		/*
+		 * The CPER source is a fixed 96 bytes, shorter than struct
+		 * aer_capability_regs, so bound the copy to it. header_len and
+		 * flit are software-only and land inside those 96 bytes; clear
+		 * them 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, sizeof(pcie_err->aer_info));
+		aer_info->header_log.header_len = 0;
+		aer_info->header_log.flit = false;
 
 		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 v4 12/13] ACPI: APEI: GHES: Bound AER info copy and sanitize software metadata

[sashiko-bot]

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

New issues:
- [High] Hardcoding `header_len = 0` and `flit = false` in `ghes_handle_aer()` actively breaks the parsing and printing of legitimate Flit mode TLP logs from CPER AER records.

Pre-existing issues:
- [High] Memory allocated for `aer_capability_regs` from the fixed-size `ghes_estatus_pool` is permanently leaked when `aer_recover_queue()` fails to insert into the `aer_recover_ring`.
- [High] Memory allocated for `aer_capability_regs` is leaked in `aer_recover_work_func()` when the target PCI device cannot be found.
- [High] Direct `memcpy` of raw CPER AER hardware bytes into `struct aer_capability_regs` misaligns fields, causing hardware registers to be incorrectly parsed and printed as TLP Prefix Logs.
- [High] Uninitialized and unvalidated software-only fields in `cxl_rch_get_aer_info()` lead to a massive out-of-bounds kernel stack read in `pcie_print_tlp_log()`.
- [High] Direct `memcpy` of little-endian CPER AER info into native integer fields of `struct aer_capability_regs` corrupts values on big-endian architectures.
--

--- Patch [12]: [PATCH v4 12/13] ACPI: APEI: GHES: Bound AER info copy and sanitize software metadata ---
commit 91c35473296c2f22c740c76f30c6c146671ec39a
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. Validate error_data_length, zero the destination,
bound the copy to the 96-byte source, and clear header_len and flit.

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..d6643127a2df0 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c

[ ... ]

> @@ -664,13 +667,22 @@ 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));
> +
> +		/*
> +		 * The CPER source is a fixed 96 bytes, shorter than struct
> +		 * aer_capability_regs, so bound the copy to it. header_len and
> +		 * flit are software-only and land inside those 96 bytes; clear
> +		 * them 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, sizeof(pcie_err->aer_info));

[Severity: High]
This isn't a bug introduced by this patch, but does this direct memcpy
misalign fields in struct aer_capability_regs?

The CPER hardware payload expects Root Error Command and Root Error Status
at offset 44 and 48 respectively. However, struct aer_capability_regs
embeds the software struct pcie_tlp_log at offset 28.

Because pcie_tlp_log is padded for Flit mode and takes up ~60 bytes, doesn't
this incorrectly place the hardware's Root Error Command into
aer_info->header_log.dw[4], which aliases to prefix[0]?

This would cause pcie_print_tlp_log() to dump entirely unrelated hardware
registers as TLP Prefix logs.

[Severity: High]
This is another pre-existing issue, but does this direct copy of little-endian
CPER AER info into native integer fields corrupt the values on big-endian
architectures?

Without le32_to_cpu() conversions, parsing these fields on a big-endian
system will interpret the bytes in reverse order, breaking downstream
bitwise evaluations.

> +		aer_info->header_log.header_len = 0;
> +		aer_info->header_log.flit = false;

[Severity: High]
Does hardcoding header_len and flit to 0 and false unconditionally break the
parsing and printing of legitimate Flit mode TLP logs from CPER AER records?

If the hardware actually experienced an error in Flit mode, indicated by
PCI_ERR_CAP_TLP_LOG_FLIT in aer_info->cap_control, this forcibly discards
that state. Won't pcie_print_tlp_log() then ignore the extended Flit mode
header DWORDS entirely?

>  
>  		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
>  }

[Severity: High]
This is a pre-existing issue, but does this code leak the memory allocated
for aer_info from the fixed-size ghes_estatus_pool if aer_recover_queue()
fails to insert into the aer_recover_ring?

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 ...\n", ...);
}

Since aer_recover_queue() returns void, ghes_handle_aer() cannot detect
the failure to perform cleanup, permanently leaking the pool memory.

[Severity: High]
This is also a pre-existing issue, but is the memory for aer_info leaked
in aer_recover_work_func() if the target PCI device cannot be found?

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("... no pci_dev found\n");
        continue;
    }
    ...
}

The continue statement skips the ghes_estatus_pool_region_free() cleanup
call at the bottom of the loop, permanently leaking the memory.

[Severity: High]
This isn't a bug in this patch either, but does a similar issue with
uninitialized software-only fields exist in cxl_rch_get_aer_info()?

drivers/cxl/core/ras_rch.c:cxl_rch_get_aer_info() {
    ...
    for (n = 0; n < read_cnt; n++)
        aer_regs_buf[n] = readl(aer_base + n * sizeof(u32));
    ...
}

If it reads from unimplemented MMIO space, it returns 0xFFFFFFFF, which
populates the software-only metadata fields on the stack such that flit
evaluates to true and header_len becomes 255.

Doesn't this lead to an out-of-bounds stack read later?

drivers/pci/pcie/tlp.c:pcie_print_tlp_log() {
    ...
    for (i = PCIE_STD_NUM_TLP_HEADERLOG; i < log->header_len; i++) {
        len += scnprintf(buf + len, sizeof(buf) - len,
                         " %#010x", log->dw[i]);
    ...
}

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

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

[Dave Jiang]

On 8/24/26 11:17 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> 
> New issues:
> - [High] Hardcoding `header_len = 0` and `flit = false` in `ghes_handle_aer()` actively breaks the parsing and printing of legitimate Flit mode TLP logs from CPER AER records.
> 
> Pre-existing issues:
> - [High] Memory allocated for `aer_capability_regs` from the fixed-size `ghes_estatus_pool` is permanently leaked when `aer_recover_queue()` fails to insert into the `aer_recover_ring`.
> - [High] Memory allocated for `aer_capability_regs` is leaked in `aer_recover_work_func()` when the target PCI device cannot be found.
> - [High] Direct `memcpy` of raw CPER AER hardware bytes into `struct aer_capability_regs` misaligns fields, causing hardware registers to be incorrectly parsed and printed as TLP Prefix Logs.
> - [High] Uninitialized and unvalidated software-only fields in `cxl_rch_get_aer_info()` lead to a massive out-of-bounds kernel stack read in `pcie_print_tlp_log()`.
> - [High] Direct `memcpy` of little-endian CPER AER info into native integer fields of `struct aer_capability_regs` corrupts values on big-endian architectures.
> --
> 
> --- Patch [12]: [PATCH v4 12/13] ACPI: APEI: GHES: Bound AER info copy and sanitize software metadata ---
> commit 91c35473296c2f22c740c76f30c6c146671ec39a
> 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. Validate error_data_length, zero the destination,
> bound the copy to the 96-byte source, and clear header_len and flit.
> 
> 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..d6643127a2df0 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
> 
> [ ... ]
> 
>> @@ -664,13 +667,22 @@ 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));
>> +
>> +		/*
>> +		 * The CPER source is a fixed 96 bytes, shorter than struct
>> +		 * aer_capability_regs, so bound the copy to it. header_len and
>> +		 * flit are software-only and land inside those 96 bytes; clear
>> +		 * them 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, sizeof(pcie_err->aer_info));
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this direct memcpy
> misalign fields in struct aer_capability_regs?
> 
> The CPER hardware payload expects Root Error Command and Root Error Status
> at offset 44 and 48 respectively. However, struct aer_capability_regs
> embeds the software struct pcie_tlp_log at offset 28.
> 
> Because pcie_tlp_log is padded for Flit mode and takes up ~60 bytes, doesn't
> this incorrectly place the hardware's Root Error Command into
> aer_info->header_log.dw[4], which aliases to prefix[0]?
> 
> This would cause pcie_print_tlp_log() to dump entirely unrelated hardware
> registers as TLP Prefix logs.
> 
> [Severity: High]
> This is another pre-existing issue, but does this direct copy of little-endian
> CPER AER info into native integer fields corrupt the values on big-endian
> architectures?
> 
> Without le32_to_cpu() conversions, parsing these fields on a big-endian
> system will interpret the bytes in reverse order, breaking downstream
> bitwise evaluations.
> 
>> +		aer_info->header_log.header_len = 0;
>> +		aer_info->header_log.flit = false;
> 
> [Severity: High]
> Does hardcoding header_len and flit to 0 and false unconditionally break the
> parsing and printing of legitimate Flit mode TLP logs from CPER AER records?
> 
> If the hardware actually experienced an error in Flit mode, indicated by
> PCI_ERR_CAP_TLP_LOG_FLIT in aer_info->cap_control, this forcibly discards
> that state. Won't pcie_print_tlp_log() then ignore the extended Flit mode
> header DWORDS entirely?

It will, but there is nothing to discard. Flit mode logs are not decoded on this path today, and this patch does not change that. What it changes is that the failure stops being an out of bounds read.

DJ

> 
>>  
>>  		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
>>  }
> 
> [Severity: High]
> This is a pre-existing issue, but does this code leak the memory allocated
> for aer_info from the fixed-size ghes_estatus_pool if aer_recover_queue()
> fails to insert into the aer_recover_ring?
> 
> 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 ...\n", ...);
> }
> 
> Since aer_recover_queue() returns void, ghes_handle_aer() cannot detect
> the failure to perform cleanup, permanently leaking the pool memory.
> 
> [Severity: High]
> This is also a pre-existing issue, but is the memory for aer_info leaked
> in aer_recover_work_func() if the target PCI device cannot be found?
> 
> 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("... no pci_dev found\n");
>         continue;
>     }
>     ...
> }
> 
> The continue statement skips the ghes_estatus_pool_region_free() cleanup
> call at the bottom of the loop, permanently leaking the memory.
> 
> [Severity: High]
> This isn't a bug in this patch either, but does a similar issue with
> uninitialized software-only fields exist in cxl_rch_get_aer_info()?
> 
> drivers/cxl/core/ras_rch.c:cxl_rch_get_aer_info() {
>     ...
>     for (n = 0; n < read_cnt; n++)
>         aer_regs_buf[n] = readl(aer_base + n * sizeof(u32));
>     ...
> }
> 
> If it reads from unimplemented MMIO space, it returns 0xFFFFFFFF, which
> populates the software-only metadata fields on the stack such that flit
> evaluates to true and header_len becomes 255.
> 
> Doesn't this lead to an out-of-bounds stack read later?
> 
> drivers/pci/pcie/tlp.c:pcie_print_tlp_log() {
>     ...
>     for (i = PCIE_STD_NUM_TLP_HEADERLOG; i < log->header_len; i++) {
>         len += scnprintf(buf + len, sizeof(buf) - len,
>                          " %#010x", log->dw[i]);
>     ...
> }
> 

[PATCH v4 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>
---
v4:
- New patch. The extlog lock inversion fix earlier in the series removes the
  last external caller, leaving the export and the <cxl/event.h> declaration
  dead.
---
 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