Re: [PATCH v3 01/10] ACPI: APEI: GHES: Bound CXL event record copy to the firmware section length

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

Pre-existing issues:
- [High] The patch fixes an out-of-bounds read in cxl_cper_post_event(), but fails to apply a similar bounds check to the adjacent CXL Protocol Error handler for CPER_SEC_CXL_PROT_ERR.
- [High] Missing bounds checks for CPER_SEC_CXL_PROT_ERR and CPER_SEC_PCIE in the ACPI Extended Error Log (acpi_extlog.c), leading to out-of-bounds reads.
- [High] Missing bounds check for HISI PCIe vendor-specific error records, causing an out-of-bounds read.
- [High] Spinlock deadlock in CXL CPER work queue registration functions due to failure to disable interrupts.
--

--- Patch [1]: [PATCH v3 01/10] ACPI: APEI: GHES: Bound CXL event record copy to the firmware section length ---
commit 36a126e36333bbe9036612ec43459f24c942a9bb
Author: Dave Jiang <[email protected]>
Subject: ACPI: APEI: GHES: Bound CXL event record copy to the firmware section length

This commit addresses an out-of-bounds read of the firmware CPER section by
adding bounds checking for CXL event records. It updates cxl_cper_post_event()
to verify that the payload length matches expected structure sizes.

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..a752e152a5a0b 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -949,15 +954,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);
>  

[Severity: High]
This is a pre-existing issue, but does CPER_SEC_CXL_PROT_ERR also need a
similar bounds check in ghes_do_proc()?

The payload is passed to cxl_cper_post_prot_err() without validating the outer
section length:

drivers/acpi/apei/ghes.c:ghes_do_proc()
  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);
  }

Then in cxl_cper_setup_prot_err_work_data(), an out-of-bounds copy can occur
based on an unvalidated dvsec_len:

drivers/acpi/apei/ghes_helpers.c:cxl_cper_setup_prot_err_work_data()
  cap_start = dvsec_start + prot_err->dvsec_len;
  memcpy(&wd->ras_cap, cap_start, sizeof(wd->ras_cap));

Could an untrusted firmware payload lead to an out-of-bounds read here?

[Severity: High]
This is a pre-existing issue, but are there missing bounds checks in other
consumers of these error records, like drivers/acpi/acpi_extlog.c?

In extlog_print(), the payload for CXL protocol errors is passed without
checking if gdata->error_data_length is large enough:

drivers/acpi/acpi_extlog.c:extlog_print()
  extlog_cxl_cper_handle_prot_err(prot_err, gdata->error_severity);

Similarly, in extlog_print_pcie(), the payload is dereferenced directly:

drivers/acpi/acpi_extlog.c:extlog_print_pcie()
  if (!(pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID && ...))

Could this result in out-of-bounds reads if the firmware provides truncated
payload sections?

[Severity: High]
This is a pre-existing issue, but does hisi_pcie_notify_error() in
drivers/pci/controller/pcie-hisi-error.c also lack bounds checking for
vendor-specific error records?

The payload from gdata is directly cast and dereferenced without ensuring
gdata->error_data_length covers the structure:

drivers/pci/controller/pcie-hisi-error.c:hisi_pcie_notify_error()
  if (error_data->socket_id != socket)

Could a malformed firmware error record cause an out-of-bounds read here?

[Severity: High]
This is a pre-existing issue, but is there a potential spinlock deadlock in
the CXL CPER work queue registration functions?

In cxl_cper_register_work(), the lock is acquired without disabling interrupts:

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

If a hardware error interrupt fires on the same CPU while the lock is held,
the interrupt handler calls cxl_cper_post_event(), which attempts to acquire
the same lock:

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

Could this cause a deadlock if the interrupt occurs when the driver is
registering the work queue?

A similar issue appears in cxl_cper_register_prot_err_work() with
cxl_cper_prot_err_work_lock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1