[PATCH v2 0/7] ACPI: APEI: GHES: Collection of fixes for issues reported by sashiko

[PATCH v2 0/7] ACPI: APEI: GHES: Collection of fixes for issues reported by sashiko

[Dave Jiang]

A collection of fixes for pre-existing issues reported by sashiko-bot while
reviewing patches.

1/7: Bound the CXL event record copy to the firmware section length.
2/7: Validate the CXL protocol error section length before the RAS cap copy.
3/7: Avoid populating software AER metadata from the raw hardware buffer.
4/7: Validate the PCIe error section length before payload access.
5/7: Fix the CONFIG_ACPI_APEI_PCIEAER guard typo in extlog.c.
6/7: Defer CXL protocol error handling to avoid a lock inversion.
7/7: Validate the memory error section length before payload access.

v1: https://lore.kernel.org/linux-cxl/20260709162807.1957783-1-dave.jiang@intel.com/

Changes since v1:
- Dropped v1 1/4 "ACPI: APEI: GHES: Use spin_lock_irqsave() for CXL CPER
  work locks". Terry Bowman has a more comprehensive version of this that
  he will post.
- Reordered the series so the two patches that harden extlog_print_pcie()
  land before the CONFIG_ACPI_APEI_PCIEAER typo fix that activates that
  code path, avoiding a bisection window where the path is live but
  unhardened.
- Grew the series from 4 to 7 patches, adding four fixes for the
  additional pre-existing issues Sashiko flagged on the v1 posting.

Dave Jiang (7):
  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
  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: extlog: Defer CXL protocol error handling to avoid lock
    inversion
  ACPI: APEI: GHES: Validate memory error section length before payload
    access

 drivers/acpi/acpi_extlog.c       | 40 ++++++++++++++------------------
 drivers/acpi/apei/ghes.c         | 35 +++++++++++++++++++++-------
 drivers/acpi/apei/ghes_helpers.c | 22 +++++++++++++++++-
 include/acpi/ghes.h              |  4 ++++
 include/cxl/event.h              |  4 ++--
 5 files changed, 71 insertions(+), 34 deletions(-)


base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
-- 
2.55.0

[PATCH v2 1/7] ACPI: APEI: GHES: Bound CXL event record copy to the firmware section length

[Dave Jiang]

cxl_cper_post_event() blindly copies a fixed
sizeof(struct cxl_cper_event_rec) out of the firmware-provided CPER
section without checking the length. Pass gdata->error_data_length into
the function and reject a section too small to hold the record before
the copy.

Link: https://sashiko.dev/#/patchset/20260617-topics-ahmtib01-ras_ffh_arm_internal_review-v6-0-91f725174aa0@arm.com?part=6
Fixes: 5e4a264bf8b5 ("acpi/ghes: Process CXL Component Events")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v2:
- Corrected err_data_length to error_data_length in commit log
---
 drivers/acpi/apei/ghes.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 3236a3ce79d6..9916271a8151 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -815,10 +815,21 @@ 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;
 
+	/*
+	 * The record is copied whole below, so the firmware-provided section
+	 * must be at least as large as the record. cper_estatus_check() only
+	 * validates the section fits the error block, not that it satisfies a
+	 * given section type's size, so guard the fixed-size copy here.
+	 */
+	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",
@@ -949,15 +960,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.55.0

Re: [PATCH v2 1/7] ACPI: APEI: GHES: Bound CXL event record copy to the firmware section length

[sashiko-bot]

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

New issues:
- [Medium] The new length validation ties the minimum firmware section size to the kernel's internal maximum union size, which will break backwards compatibility with existing firmwares as soon as the CXL event union grows.

Pre-existing issues:
- [High] Deadlock due to acquiring a spinlock in process context without disabling interrupts, while the same lock is acquired in hardware interrupt context.
- [High] The newly added length check `len < sizeof(*rec)` can be bypassed via an integer overflow, defeating the patch's protection and allowing an out-of-bounds kernel memory read.
--

--- Patch [1]: [PATCH v2 1/7] ACPI: APEI: GHES: Bound CXL event record copy to the firmware section length ---
commit e26fd5aa15a7b2ef67720b7ee480d05f9c35224c
Author: Dave Jiang <dave.jiang@intel.com>

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

This commit adds a bounds check to cxl_cper_post_event() to prevent copying
more data than what the firmware provided in the CPER section. It passes
the error data length into the function to reject undersized sections.

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

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 3236a3ce79d6b..9916271a81513 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -815,10 +815,21 @@ static DEFINE_SPINLOCK(cxl_cper_work_lock);
>  struct work_struct *cxl_cper_work;
>  
>  static void cxl_cper_post_event(enum cxl_event_type event_type,
> -				struct cxl_cper_event_rec *rec)
> +				struct cxl_cper_event_rec *rec, u32 len)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but can this lock cause a
deadlock?

Looking at how cxl_cper_work_lock is acquired in process context:

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

And how it is acquired in hardirq context here in cxl_cper_post_event():

drivers/acpi/apei/ghes.c:cxl_cper_post_event() {
    ...
    guard(spinlock_irqsave)(&cxl_cper_work_lock);
    ...
}

If a process calls cxl_cper_register_work() and acquires the lock without
disabling local interrupts, and a hardware error triggers an interrupt on
the same CPU, cxl_cper_post_event() will attempt to acquire the exact same
lock. Does this violate the locking rule that if a lock is used in hardirq
context, all process-context acquisitions must disable interrupts?

>  	struct cxl_cper_work_data wd;
>  
> +	/*
> +	 * The record is copied whole below, so the firmware-provided section
> +	 * must be at least as large as the record. cper_estatus_check() only
> +	 * validates the section fits the error block, not that it satisfies a
> +	 * given section type's size, so guard the fixed-size copy here.
> +	 */
> +	if (len < sizeof(*rec)) {

[Severity: High]
This is a pre-existing issue, but does this length check remain vulnerable
to an integer overflow bypass?

If the firmware provides a length like 0xFFFFFFB9, upstream validation
implicitly casts this u32 to a signed int:

include/acpi/ghes.h:acpi_hest_get_error_length() {
    return ((struct acpi_hest_generic_data *)(gdata))->error_data_length;
}

This returns a negative value, which underflows the size calculation and
bypasses the estatus bounds check.

When execution reaches this point, len is evaluated as the unsigned value
0xFFFFFFB9. The check 0xFFFFFFB9 < sizeof(*rec) evaluates to false,
bypassing this new protection and allowing the memcpy to read out of bounds.

[Severity: Medium]
Will this strict size requirement break backwards compatibility with older
firmwares if the kernel's internal event union structure increases in size?

By enforcing that len is at least sizeof(*rec) (which currently contains
union cxl_event), the minimum firmware section size is tied to the kernel's
internal maximum union size.

If a future CXL specification introduces a larger event type and the kernel
expands the union, sizeof(*rec) will increase. When that happens, would the
updated kernel erroneously reject perfectly valid legacy events emitted by
existing older firmwares?

Could we instead verify len >= sizeof(rec->hdr), zero-initialize the work
data buffer, and safely copy min_t(size_t, len, sizeof(*rec)) bytes to
decouple firmware section sizes from internal kernel union sizes?

> +		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",

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

[PATCH v2 2/7] 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 by
skipping a firmware-controlled DVSEC:

	dvsec_start = (u8 *)(prot_err + 1);
	cap_start = dvsec_start + prot_err->dvsec_len;
	memcpy(&wd->ras_cap, cap_start, sizeof(wd->ras_cap));

prot_err->dvsec_len is taken from the CPER section and is never validated.

Add a check to cxl_cper_sec_prot_err_valid() to first verify the section
is large enough to contain the header struct itself before accessing any
fields, and then to verify the header, DVSEC, and RAS Capability block all
fit within the reported section length.

Link: https://sashiko.dev/#/patchset/20260617-topics-ahmtib01-ras_ffh_arm_internal_review-v6-0-91f725174aa0@arm.com?part=6
Link: https://lore.kernel.org/linux-cxl/20260709165457.8BA181F000E9@smtp.kernel.org/
Fixes: 315c2f0b53ba ("acpi/ghes, cper: Recognize and cache CXL Protocol errors")
Assisted-by: Claude:claude-sonnet-4-6
Reviewed-by: Ben Cheatham <benjamin.cheatham@amd.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v2:
- Missing bounds check on CPER record length and dvsec_len (sashiko)
- Add Ben's reviewed-by
---
 drivers/acpi/acpi_extlog.c       |  7 ++++---
 drivers/acpi/apei/ghes.c         |  7 ++++---
 drivers/acpi/apei/ghes_helpers.c | 22 +++++++++++++++++++++-
 include/cxl/event.h              |  4 ++--
 4 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index 7ad3b36013cc..06a944dadbc1 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -165,12 +165,12 @@ static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
 
 static void
 extlog_cxl_cper_handle_prot_err(struct cxl_cper_sec_prot_err *prot_err,
-				int severity)
+				int severity, u32 len)
 {
 #ifdef 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;
 
 	if (cxl_cper_setup_prot_err_work_data(&wd, prot_err, severity))
@@ -236,7 +236,8 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
 				acpi_hest_get_payload(gdata);
 
 			extlog_cxl_cper_handle_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 9916271a8151..6fa5d3bab1ce 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;
 
 static 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);
@@ -956,7 +956,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..7db6a6722fcf 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,19 @@ int cxl_cper_sec_prot_err_valid(struct cxl_cper_sec_prot_err *prot_err)
 		return -EINVAL;
 	}
 
+	/*
+	 * The RAS Capability block follows a firmware-controlled DVSEC of
+	 * prot_err->dvsec_len bytes. Verify the header, the DVSEC and the RAS
+	 * Capability block all fit within the CPER section.
+	 */
+	if (sizeof(*prot_err) + prot_err->dvsec_len +
+	    sizeof(struct cxl_ras_capability_regs) > len) {
+		pr_err_ratelimited(FW_WARN
+				   "CXL CPER prot err section too small (%u)\n",
+				   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/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.55.0

Re: [PATCH v2 2/7] ACPI: APEI: GHES: Validate CXL protocol error section length before RAS cap copy

[sashiko-bot]

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

Pre-existing issues:
- [High] CXL Protocol Error handling in Extlog is permanently disabled due to a typo in the Kconfig macro check.
- [High] Unconditional use of `dvsec_len` without checking the `PROT_ERR_VALID_DVSEC` valid bit, leading to incorrect validation and wrong RAS capability offset.
- [High] Out-of-bounds read in `cxl_cper_print_prot_err()` because `dvsec_len` is not validated against the section length.
- [High] Deadlock (IRQ inversion) due to acquiring `cxl_cper_prot_err_work_lock` and `cxl_cper_work_lock` without disabling IRQs in process context.
--

--- Patch [2]: [PATCH v2 2/7] ACPI: APEI: GHES: Validate CXL protocol error section length before RAS cap copy ---
commit 66f8d9e4b5a4dd7c78d837b5436bcc5297ca8261
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 by
skipping a firmware-controlled DVSEC. This commit adds checks to verify
the section is large enough to contain the header and capabilities before
accessing any fields.

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/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> index 7ad3b36013cc6..06a944dadbc15 100644
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c
> @@ -165,12 +165,12 @@ static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
>  
>  static void
>  extlog_cxl_cper_handle_prot_err(struct cxl_cper_sec_prot_err *prot_err,
> -				int severity)
> +				int severity, u32 len)
>  {
>  #ifdef ACPI_APEI_PCIEAER

[Severity: High]
This is a pre-existing issue, but does this Kconfig macro check have a typo?

It uses ACPI_APEI_PCIEAER instead of CONFIG_ACPI_APEI_PCIEAER, which means the
entire body of extlog_cxl_cper_handle_prot_err() is compiled out.

On platforms that use Extlog rather than GHES, CXL protocol errors would be
silently ignored, potentially leaving hardware in a broken state and leading
to system crashes.

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

[ ... ]

> diff --git a/drivers/acpi/apei/ghes_helpers.c b/drivers/acpi/apei/ghes_helpers.c
> index bc7111b740afb..7db6a6722fcff 100644
> --- a/drivers/acpi/apei/ghes_helpers.c
> +++ b/drivers/acpi/apei/ghes_helpers.c
> @@ -23,6 +30,19 @@ int cxl_cper_sec_prot_err_valid(struct cxl_cper_sec_prot_err *prot_err)
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * The RAS Capability block follows a firmware-controlled DVSEC of
> +	 * prot_err->dvsec_len bytes. Verify the header, the DVSEC and the RAS
> +	 * Capability block all fit within the CPER section.
> +	 */
> +	if (sizeof(*prot_err) + prot_err->dvsec_len +
> +	    sizeof(struct cxl_ras_capability_regs) > len) {

[Severity: High]
This is a pre-existing issue, but should this bounds check and the offset
calculation in cxl_cper_setup_prot_err_work_data() verify that the
PROT_ERR_VALID_DVSEC bit is set in valid_bits before using dvsec_len?

If the PROT_ERR_VALID_DVSEC bit is clear, dvsec_len is undefined and could
contain garbage.

Using a garbage length here could cause this new bounds check to spuriously
fail, or cause the RAS capability offset to point to arbitrary data that gets
passed to the PCI AER driver.

> +		pr_err_ratelimited(FW_WARN
> +				   "CXL CPER prot err section too small (%u)\n",
> +				   len);
> +		return -EINVAL;
> +	}

While reviewing this area, I also noticed a couple of other pre-existing
issues related to CXL protocol error handling that might be worth addressing.

[Severity: High]
This is a pre-existing issue, but is there an out-of-bounds read in
cxl_cper_print_prot_err() in drivers/firmware/efi/cper_cxl.c?

When calling print_hex_dump() using prot_err->dvsec_len:

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

The dvsec_len field is not validated against the actual section length
provided in gdata->error_data_length. If firmware provides a corrupt or
malicious dvsec_len, this could read past the memory buffer, potentially
causing a page fault and kernel panic.


[Severity: High]
This is a pre-existing issue, but is there a potential deadlock (IRQ inversion)
between cxl_cper_register_prot_err_work() and cxl_cper_post_prot_err() in
drivers/acpi/apei/ghes.c?

In process context, cxl_cper_register_prot_err_work() acquires the lock
without disabling interrupts:

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

If an error interrupt arrives on the same CPU while the lock is held, the
interrupt handler calls cxl_cper_post_prot_err(), which tries to acquire the
same lock:

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

Would this cause the interrupt handler to spin forever waiting for the lock?
The same pattern appears to exist for cxl_cper_work_lock in
cxl_cper_register_work().

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

[PATCH v2 3/7] ACPI: extlog: Avoid populating software AER metadata from raw hardware buffer

[Dave Jiang]

extlog_print_pcie() casts pcie_err->aer_info directly to struct
aer_capability_regs *, treating raw hardware register bytes as the
struct's software-only metadata fields header_log.header_len and
header_log.flit.

struct aer_capability_regs embeds struct pcie_tlp_log, which places
header_len and flit after the 14-element dw[] array at offset 84.
The 96-byte aer_info hardware buffer covers that offset, so the cast
populates header_len and flit with unvalidated hardware data.

pcie_print_tlp_log() uses flit and header_len to bound a loop over
dw[]. If flit is set and header_len reads as a large value, the loop
iterates past the end of the dw[] array.

Copy aer_info into a zeroed local struct aer_capability_regs and
explicitly clear header_len and flit after the copy so only known-safe
values reach pcie_print_tlp_log().

Fixes: e778ffefa34d ("ACPI: extlog: Trace CPER PCI Express Error Section")
Link: https://lore.kernel.org/linux-cxl/20260709165457.8BA181F000E9@smtp.kernel.org/
Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v2:
- new patch, issue raised by sashiko
---
 drivers/acpi/acpi_extlog.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index 06a944dadbc1..fbc88c584c06 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -137,6 +137,7 @@ static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
 			      int severity)
 {
 #ifdef ACPI_APEI_PCIEAER
+	struct aer_capability_regs aer_regs = {};
 	struct aer_capability_regs *aer;
 	struct pci_dev *pdev;
 	unsigned int devfn;
@@ -149,7 +150,12 @@ 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;
+	aer = &aer_regs;
+
 	domain = pcie_err->device_id.segment;
 	bus = pcie_err->device_id.bus;
 	devfn = PCI_DEVFN(pcie_err->device_id.device,
-- 
2.55.0

Re: [PATCH v2 3/7] ACPI: extlog: Avoid populating software AER metadata from raw hardware buffer

[sashiko-bot]

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

Pre-existing issues:
- [High] GHES AER handling suffers from the exact same unvalidated hardware data bug being fixed, plus a 4-byte out-of-bounds buffer read.
- [High] cxl_rch_get_aer_info() reads raw MMIO past the hardware capability boundary and populates software struct fields directly, triggering an out-of-bounds stack read.
- [High] Missing bounds check on gdata->error_data_length before processing CPER_SEC_CXL_PROT_ERR and CPER_SEC_PCIE payloads.
- [Medium] The patch maps the hardware AER capability buffer to struct aer_capability_regs via memcpy, but structural misalignment causes hardware registers to be logged erroneously as TLP Prefixes.
--

--- Patch [3]: [PATCH v2 3/7] ACPI: extlog: Avoid populating software AER metadata from raw hardware buffer ---
commit abdd5781d57340e81525247f53a52f27296139bd
Author: Dave Jiang <dave.jiang@intel.com>
ACPI: extlog: Avoid populating software AER metadata from raw hardware buffer

This commit copies the AER info hardware buffer into a zeroed local struct
to avoid treating raw hardware bytes as software-only metadata. It then
explicitly clears the header_len and flit fields so only known-safe values
reach the TLP log printing routine.

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

> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> index 06a944dadbc15..fbc88c584c067 100644
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c
> @@ -137,6 +137,7 @@ static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
>  			      int severity)
>  {
>  #ifdef ACPI_APEI_PCIEAER
> +	struct aer_capability_regs aer_regs = {};
>  	struct aer_capability_regs *aer;

[ ... ]

> @@ -149,7 +150,12 @@ 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));

[Severity: Medium]
This is a pre-existing issue, but the structural misalignment here appears to
cause hardware registers to be logged erroneously as TLP Prefixes.

The software struct embeds a struct pcie_tlp_log that is 60 bytes long,
whereas the actual hardware Header Log is only 16 bytes. Does mapping the
hardware AER capability buffer directly to struct aer_capability_regs cause
the Root Error Command and Root Error Status registers to map into the
aer_regs.header_log.prefix array?

Since flit is set to false below, pcie_print_tlp_log() falls back to
printing the prefix array if prefix[0] is non-zero. Could this erroneously
print raw hardware control registers as End-to-End TLP Prefixes, corrupting
logs?

> +	aer_regs.header_log.header_len = 0;
> +	aer_regs.header_log.flit = false;
> +	aer = &aer_regs;

I noticed a few other related pre-existing issues in the codebase that might
warrant a look:

[Severity: High]
This is a pre-existing issue, but is there a missing bounds check on the
error data length before processing payloads?

In extlog_print(), the code iterates over ACPI HEST generic data sections
and processes them based on section type:

drivers/acpi/acpi_extlog.c:extlog_print() {
    ...
    extlog_print_pcie(pcie_err, gdata->error_severity);
    ...
}

If a malformed or truncated payload is received from the hardware, could this
access out-of-bounds memory when downstream functions dereference the payload
fields, since gdata->error_data_length isn't validated to be at least
sizeof(struct cper_sec_pcie)?


[Severity: High]
This is a pre-existing issue, but does ghes_handle_aer() suffer from the
same unvalidated hardware data bug being fixed here, along with an
out-of-bounds read?

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

This memcpy copies 100 bytes (size of the struct) from a 96-byte array,
reading past the payload buffer. Does this also populate the software-only
flit and header_len fields with unvalidated hardware data, potentially
causing pcie_print_tlp_log() to iterate past the end of the dynamically
allocated buffer?


[Severity: High]
This is a pre-existing issue, but does cxl_rch_get_aer_info() read raw MMIO
past the hardware capability boundary?

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

This loop reads 100 bytes from the MMIO address, which exceeds standard AER
capability sizes (56 to 72 bytes). 

Later, in cxl_handle_rdport_errors(), the polluted struct is allocated on
the stack:

drivers/cxl/core/ras_rch.c:cxl_handle_rdport_errors() {
    ...
    struct aer_capability_regs aer_regs;
    ...
}

Since the raw MMIO read pollutes the flit and header_len fields, could
pcie_print_tlp_log() use these unvalidated fields to iterate past the bounds
of the dw array and leak kernel stack memory?

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

[PATCH v2 4/7] 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 fixed 96-byte pcie_err->aer_info buffer without first checking
that gdata->error_data_length is large enough to hold a struct
cper_sec_pcie.

cper_estatus_check() guarantees the reported error_data_length lies
within the estatus block, so a short section does not read unmapped
memory, but it does let stale bytes from adjacent padding or the next
section be interpreted as PCIe error data. Reject a section too small to
hold the record before touching any field, matching the convention used
elsewhere.

Fixes: e778ffefa34d ("ACPI: extlog: Trace CPER PCI Express Error Section")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v2:
- new patch, issue raised by sashiko
---
 drivers/acpi/acpi_extlog.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index fbc88c584c06..0c440d75d9a7 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 = {};
@@ -145,6 +145,9 @@ static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
 	int aer_severity;
 	int domain;
 
+	if (len < sizeof(*pcie_err))
+		return;
+
 	if (!(pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID &&
 	      pcie_err->validation_bits & CPER_PCIE_VALID_AER_INFO))
 		return;
@@ -247,7 +250,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.55.0

Re: [PATCH v2 4/7] ACPI: extlog: Validate PCIe error section length before payload access

[sashiko-bot]

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

Pre-existing issues:
- [High] Other APEI/GHES CPER payload handlers lack error_data_length validation before accessing payload structs.
- [High] Hardware PCIe AER capability layout is blindly copied into a software structure, causing severe field misalignment and OOB reads.
--

--- Patch [4]: [PATCH v2 4/7] ACPI: extlog: Validate PCIe error section length before payload access ---
commit e17a8b52722d296c5c160626ff1eb6bdf1232531
Author: Dave Jiang <dave.jiang@intel.com>

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

extlog_print_pcie() reads pcie_err->validation_bits and device_id and
copies the fixed 96-byte pcie_err->aer_info buffer without first checking
that gdata->error_data_length is large enough to hold a struct
cper_sec_pcie.

> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> index fbc88c584c067..0c440d75d9a78 100644
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c
[ ... ]
> @@ -145,6 +145,9 @@ static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
>  	int aer_severity;
>  	int domain;
>  
> +	if (len < sizeof(*pcie_err))
> +		return;
> +
>  	if (!(pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID &&
>  	      pcie_err->validation_bits & CPER_PCIE_VALID_AER_INFO))
>  		return;

[Severity: High]
This is a pre-existing issue, but does this code correctly handle the
hardware PCIe AER capability layout?

When copying the hardware aer_info (a 96-byte raw CPER array) into the
software struct aer_capability_regs in extlog_print_pcie(), it appears the
software structure expands header_log to 60 bytes to support 14 DWORDs and
prefix arrays, while the hardware TLP log is only 16 bytes.

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

Does this cause hardware fields starting from root_command to overlap with
header_log.dw[4], losing the root status information and misinterpreting
them as TLP prefixes?

This same misalignment appears to occur in other places too:

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

In ghes_handle_aer(), does copying sizeof(struct aer_capability_regs)
(100 bytes) from a 96-byte array cause a 4-byte out-of-bounds read?

And also blindly casted in CPER:

drivers/firmware/efi/cper.c:cper_print_pcie() {
    ...
    aer = (struct aer_capability_regs *)pcie->aer_info;
    ...
}

[ ... ]
> @@ -247,7 +250,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);

[Severity: High]
This isn't a bug introduced by this patch, but do other APEI/GHES CPER
payload handlers also lack error_data_length validation before accessing
payload structs?

If firmware provides a CPER payload with an artificially small
error_data_length, could we experience out-of-bounds reads in
ghes_handle_memory_failure()?

drivers/acpi/apei/ghes.c:ghes_handle_memory_failure() {
    ...
    struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);

    if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
    ...
}

Does this risk operating on garbage physical addresses or adjacent
uninitialized padding?

Similarly, in ghes_handle_aer() without a length check:

drivers/acpi/apei/ghes.c:ghes_handle_aer() {
    ...
    struct cper_sec_pcie *pcie_err = acpi_hest_get_payload(gdata);

    if (pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID)
    ...
}

Would adding length validation similar to this patch be needed for those
other handlers as well?

>  		} else {
>  			void *err = acpi_hest_get_payload(gdata);
>

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

[PATCH v2 5/7] ACPI: extlog: Fix CONFIG_ACPI_APEI_PCIEAER guard typo

[Dave Jiang]

"#ifdef ACPI_APEI_PCIEAER" is incorrect. It should be
"#ifdef CONFIG_ACPI_APEI_PCIEAER". Currently the section wrapped is
always compiled out because of this. Use the correct CONFIG_ prefixed
symbol so the code is actually built.

Fixes: 95350effc3ad ("ACPI: extlog: Trace CPER CXL Protocol Error Section")
Assisted-by: Claude:claude-opus-4-8
Reviewed-by: Ben Cheatham <benjamin.cheatham@amd.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v2:
- Fixed commit log incorrect kconfig symbol.
- Added Ben's review tag.
---
 drivers/acpi/acpi_extlog.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index 0c440d75d9a7..3e3e388af47b 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 aer_capability_regs *aer;
 	struct pci_dev *pdev;
@@ -176,7 +176,7 @@ static void
 extlog_cxl_cper_handle_prot_err(struct cxl_cper_sec_prot_err *prot_err,
 				int severity, u32 len)
 {
-#ifdef ACPI_APEI_PCIEAER
+#ifdef CONFIG_ACPI_APEI_PCIEAER
 	struct cxl_cper_prot_err_work_data wd;
 
 	if (cxl_cper_sec_prot_err_valid(prot_err, len))
-- 
2.55.0

[PATCH v2 6/7] ACPI: extlog: Defer CXL protocol error handling to avoid lock inversion

[Dave Jiang]

extlog_cxl_cper_handle_prot_err() calls cxl_cper_handle_prot_err()
synchronously while the MCE notifier chain rwsem is held read:

  blocking_notifier_call_chain()       # holds rwsem read
    extlog_print()
      extlog_cxl_cper_handle_prot_err()
        cxl_cper_handle_prot_err()
          guard(device)(&pdev->dev)    # acquires device_lock

This creates an AB-BA lock ordering with the probe path, where the PCI
core holds device_lock during .probe and cxl_memdev_state_create() calls
devm_cxl_register_mce_notifier() -> mce_register_decode_chain() which
takes the rwsem write:

  device_lock -> rwsem (probe path)
  rwsem -> device_lock (MCE path)

ghes.c avoids this by posting CXL protocol errors to a kfifo and
deferring processing to a workqueue via cxl_cper_post_prot_err(). Export
that function and use it from acpi_extlog.c instead of calling
cxl_cper_handle_prot_err() directly.

Link: https://lore.kernel.org/linux-cxl/20260709165457.8BA181F000E9@smtp.kernel.org/
Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v2:
- Export to acpi_extlog modules instead of CXL namespace. (0-day bot)
---
 drivers/acpi/acpi_extlog.c | 23 +++--------------------
 drivers/acpi/apei/ghes.c   |  5 +++--
 include/acpi/ghes.h        |  4 ++++
 3 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index 3e3e388af47b..1fbaf134c04e 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -172,23 +172,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, u32 len)
-{
-#ifdef CONFIG_ACPI_APEI_PCIEAER
-	struct cxl_cper_prot_err_work_data wd;
-
-	if (cxl_cper_sec_prot_err_valid(prot_err, len))
-		return;
-
-	if (cxl_cper_setup_prot_err_work_data(&wd, prot_err, severity))
-		return;
-
-	cxl_cper_handle_prot_err(&wd);
-#endif
-}
-
 static int extlog_print(struct notifier_block *nb, unsigned long val,
 			void *data)
 {
@@ -244,9 +227,9 @@ 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,
-							gdata->error_data_length);
+			cxl_cper_post_prot_err(prot_err,
+					       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 6fa5d3bab1ce..b36f6d268c9f 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, u32 len)
+void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
+			    int severity, u32 len)
 {
 #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..4dcbb2c30ea2 100644
--- a/include/acpi/ghes.h
+++ b/include/acpi/ghes.h
@@ -143,4 +143,8 @@ static inline int ghes_notify_sea(void) { return -ENOENT; }
 struct notifier_block;
 extern void ghes_register_report_chain(struct notifier_block *nb);
 extern void ghes_unregister_report_chain(struct notifier_block *nb);
+
+struct cxl_cper_sec_prot_err;
+void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
+			    int severity, u32 len);
 #endif /* GHES_H */
-- 
2.55.0

Re: [PATCH v2 6/7] ACPI: extlog: Defer CXL protocol error handling to avoid lock inversion

[sashiko-bot]

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

Pre-existing issues:
- [High] Deadlock: `cxl_cper_register_prot_err_work` and `cxl_cper_unregister_prot_err_work` take `cxl_cper_prot_err_work_lock` without disabling interrupts, but the lock is also acquired in hardirq context.
- [High] `cxl_cper_prot_err_work_lock` is a `spinlock_t` acquired in hardirq context, which violates PREEMPT_RT locking rules.
- [High] Missing bounds check on `estatus->data_length` in `extlog_print` leads to out-of-bounds read of `elog_buf`.
--

--- Patch [6]: [PATCH v2 6/7] ACPI: extlog: Defer CXL protocol error handling to avoid lock inversion ---
commit 7b79585a4c6060a80ae775e5e2ed8715f3df690b
Author: Dave Jiang <dave.jiang@intel.com>

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

The commit avoids an AB-BA lock inversion between the device_lock and the
MCE notifier chain rwsem. It does this by deferring CXL protocol error
handling to a workqueue using cxl_cper_post_prot_err(), which posts errors
to a kfifo rather than handling them synchronously.

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

> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> index 3e3e388af47b3..1fbaf134c04e3 100644
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c

[ ... ]

> @@ -244,9 +227,9 @@ 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,
> -							gdata->error_data_length);
> +			cxl_cper_post_prot_err(prot_err,
> +					       gdata->error_severity,
> +					       gdata->error_data_length);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for
estatus->data_length to cause an out-of-bounds read of the elog_buf?

Earlier in extlog_print(), estatus is copied into a 4096-byte (ELOG_ENTRY_LEN)
buffer:

    memcpy(elog_buf, (void *)estatus, ELOG_ENTRY_LEN);
    tmp = (struct acpi_hest_generic_status *)elog_buf;

Then apei_estatus_for_each_section(tmp, gdata) iterates over this memory
based on tmp->data_length. Since there appears to be no bounds check
verifying that data_length fits within ELOG_ENTRY_LEN, could a
firmware-controlled data_length cause the macro to read past the end of
the allocated buffer?

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 6fa5d3bab1ceb..b36f6d268c9fd 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, u32 len)
> +void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
> +			    int severity, u32 len)

[Severity: High]
This is a pre-existing issue, but does cxl_cper_prot_err_work_lock cause
a sleeping in atomic violation on PREEMPT_RT kernels?

cxl_cper_prot_err_work_lock is declared as a standard spinlock_t using
DEFINE_SPINLOCK(), which becomes a sleepable rt_mutex on PREEMPT_RT. Since
cxl_cper_post_prot_err() is called from hardirq context (via
ghes_proc_in_irq), does acquiring this lock violate the rule against
acquiring sleepable locks in hardirq context?

[Severity: High]
This is a pre-existing issue, but is there a risk of a deadlock involving
cxl_cper_prot_err_work_lock?

When cxl_cper_register_prot_err_work() and cxl_cper_unregister_prot_err_work()
acquire this lock from process context, they use:

    guard(spinlock)(&cxl_cper_prot_err_work_lock);

This leaves interrupts enabled.

If a hardware error triggers an MCE or ghes_proc_in_irq on the same CPU while
the lock is held, cxl_cper_post_prot_err() will be called from hardirq
context. It will attempt to acquire the already-held lock using
guard(spinlock_irqsave)(&cxl_cper_prot_err_work_lock), which could freeze
the CPU.

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

[PATCH v2 7/7] ACPI: APEI: GHES: Validate memory error section length before payload access

[Dave Jiang]

ghes_handle_memory_failure() dereferences the CPER section payload
(mem_err->validation_bits, and mem_err->physical_addr) without first
checking that gdata->error_data_length is large enough to hold a
struct cper_sec_mem_err.

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>
---
v2:
- new from issue raised by sashiko
---
 drivers/acpi/apei/ghes.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index b36f6d268c9f..e8b2980bd5fe 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -535,6 +535,9 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
 	int sec_sev = ghes_severity(gdata->error_severity);
 	struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
 
+	if (gdata->error_data_length < sizeof(*mem_err))
+		return false;
+
 	if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
 		return false;
 
-- 
2.55.0

Re: [PATCH v2 7/7] ACPI: APEI: GHES: Validate memory error section length before payload access

[sashiko-bot]

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

New issues:
- [High] The newly added length check is overly strict and incorrectly rejects legitimate memory error payloads generated by older UEFI firmware, silently dropping valid hardware errors.

Pre-existing issues:
- [High] The payload length check added in `ghes_handle_memory_failure()` is an incomplete fix and placed too late, leaving other consumers of the same payload vulnerable to out-of-bounds reads.
--

--- Patch [7]: [PATCH v2 7/7] ACPI: APEI: GHES: Validate memory error section length before payload access ---
commit e9b5dfb37c8ab1d8db8c3e63d015c33fc4b4b12f
Author: Dave Jiang <dave.jiang@intel.com>

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

This commit attempts to prevent out-of-bounds accesses when handling memory
errors by validating that the CPER section payload length is large enough
to contain a struct cper_sec_mem_err before dereferencing it.

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index b36f6d268c9fd..e8b2980bd5fea 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c

[ ... ]

> @@ -535,6 +535,9 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
>  	int sec_sev = ghes_severity(gdata->error_severity);
>  	struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
>  
> +	if (gdata->error_data_length < sizeof(*mem_err))
> +		return false;
> +

[Severity: High]
Does this length check accidentally reject legitimate memory error payloads
from older UEFI firmware?

If the firmware uses the older v2.1/v2.2 layout (which is 73 bytes long),
comparing gdata->error_data_length against sizeof(*mem_err) (which evaluates
to 80 bytes) will cause this condition to return false.

Could this regression lead to silently dropping valid hardware errors and
preventing the kernel from gracefully offlining broken memory pages?

[Severity: High]
This is a pre-existing issue, but does validating the payload length here in
ghes_handle_memory_failure() leave other consumers vulnerable to
out-of-bounds reads?

Looking at the call chain, the payload is distributed to other consumers
before this function is ever reached:

drivers/acpi/apei/ghes.c:ghes_do_proc() {
        ...
        struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
        atomic_notifier_call_chain(&ghes_report_chain, sev, mem_err);
        arch_apei_report_mem_error(sev, mem_err);
        queued = ghes_handle_memory_failure(gdata, sev, sync);
        ...
}

If the payload is truncated, won't callbacks like
ghes_edac_report_mem_error() and apei_mce_report_mem_error() access fields
out of bounds when they check mem_err->validation_bits, potentially
interpreting adjacent garbage memory as a valid physical address?

>  	if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
>  		return false;
>

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