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

On Fri, Sep 4, 2026 at 7:23 PM Dave Jiang <[email protected]> wrote:
>
> Fixes for pre-existing issues sashiko-bot found while reviewing patches in the
> CPER, extlog and GHES paths. v1 through v3 fixed successive batches as the
> review widened; see the links below.
>
> The series is grouped in three parts, plus a cleanup.
>
> Bound the record before anything walks it:
>
> 1/13:  Reject CPER records with an out-of-range error_data_length.
> 2/13:  Reject an error status block length that wraps a u32.
> 3/13:  Validate the extlog record length before walking sections.
>
> Fix the extlog error paths, then enable them:
>
> 4/13:  Defer CXL protocol error handling to avoid a lock inversion.
> 5/13:  Avoid populating software AER metadata from the raw hardware buffer.
> 6/13:  Validate the PCIe error section length before payload access.
> 7/13:  Fix the CONFIG_ACPI_APEI_PCIEAER guard typo in extlog.c.
>
> Bound each section payload before its consumers read it:
>
> 8/13:  Bound the CXL event record copy to the firmware section length.
> 9/13:  Validate the CXL protocol error section length before the RAS cap copy.
> 10/13: Read only validated fields in cper_mem_err_pack().
> 11/13: Validate the memory error section length before payload access.
> 12/13: Bound the AER info copy and sanitize software metadata in ghes.c.
>
> Then drop an export patch 4 made redundant:
>
> 13/13: Make cxl_cper_handle_prot_err() static.
>
> Patches 1, 2 and 10 touch drivers/firmware/efi/cper.c, closing the holes at
> the shared choke point the rest of the series relies on. Patch 2 also fixes an
> infinite loop in bert_print_all(), unrelated to this series but the same root
> cause.
>
> Known gaps, left for separate patches:
>
>   - cxl_cper_print_prot_err() in drivers/firmware/efi/cper_cxl.c uses
>     dvsec_len without bounding it against the section length.
>   - cxl_rch_get_aer_info() in drivers/cxl/core/ras_rch.c reads the RCH AER
>     capability from MMIO without clearing header_len/flit, the same class as
>     patches 5 and 12. In the same file, cxl_rch_get_aer_severity() tests
>     PCI_ERR_ROOT_FATAL_RCV against uncor_status, where that bit is
>     PCI_ERR_UNC_FCP.
>   - struct pcie_tlp_log grew to 60 bytes for Flit mode, so the 96-byte CPER
>     AER info no longer maps 1:1 onto struct aer_capability_regs past the
>     Header Log. Patches 5 and 12 now copy the part that maps and place the TLP
>     Prefix Log from its own offset, which covers every field the print path
>     reads - but nothing here decodes the Flit-mode header DWORDs, which reuse
>     those same prefix registers at payload offset 56 while the struct expects
>     dw[4..13] at 44. Doing that properly wants a field-by-field mapping
>     shared with cxl_rch_get_aer_info(), plus a clamp: pcie_print_tlp_log()
>     trusts header_len against a 14-entry dw[], and PCI_ERR_CAP_TLP_LOG_SIZE
>     is five bits wide.
>
> v1: https://lore.kernel.org/linux-cxl/[email protected]/
> v2: https://lore.kernel.org/linux-cxl/[email protected]/
> v3: https://lore.kernel.org/linux-cxl/[email protected]/
> v4: https://lore.kernel.org/linux-cxl/[email protected]/
> v5: https://lore.kernel.org/linux-cxl/[email protected]/
>
> Changes since v5
> ----------------
> - Patches 5 and 12: also place the TLP Prefix Log from its own hardware
>   offset rather than leaving it zero. Shortening the copy in v5 dropped it,
>   and it is the one field the print path still reads (sashiko). Alison's and
>   Shuai's Reviewed-by are kept on both, since the intent and location have
>   not changed.
>
> Changes since v4
> ----------------
> - Patch 1: use check_add_overflow() instead of a u64 sum plus an INT_MAX
>   test, and drop the now-redundant acpi_hest_get_size() bound, since
>   record_size is never smaller than the header (Jonathan Cameron).
> - Patches 5 and 12: copy only the 44 bytes of aer_info that map onto struct
>   aer_capability_regs - the leading registers and the four Header Log DWORDs
>   - and leave the rest zero, instead of copying all 96 bytes and then
>   clearing header_len and flit (Jonathan Cameron). That also keeps the Root
>   Error Command, Root Error Status and Error Source ID out of
>   header_log.prefix[], where pcie_print_tlp_log() was printing them as
>   end-to-end prefixes.
> - Patch 3: kept the length bound ahead of cper_estatus_check() and expanded
>   the comment to say why. Swapping them would let cper_estatus_check() walk
>   sections over an unbounded data_length, past the end of elog_buf
>   (Jonathan Cameron).
> - Condensed the commit logs and comments again; prose only.
>
> 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: bound the sum rather than the u32 error_data_length, so the check
>   does not depend on the <acpi/ghes.h> helpers behaving. The v3 "< 0" check
>   was dead code (Tony Luck, Shuai Xue). 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).
> - Patch 4: moved the cxl_cper_post_prot_err() declaration inside the
>   CONFIG_ACPI_APEI_GHES block in <acpi/ghes.h> (Shuai Xue).
> - 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 (sashiko). Reviewed-by dropped; the
>   check gained a helper and is no longer the one Alison and Shuai reviewed.
> - New patch 13: make cxl_cper_handle_prot_err() static. Patch 4 removed its
>   last external caller.
>
> 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       | 64 +++++++++++++---------
>  drivers/acpi/apei/ghes.c         | 92 +++++++++++++++++++++++++++-----
>  drivers/acpi/apei/ghes_helpers.c | 18 ++++++-
>  drivers/cxl/core/ras.c           |  3 +-
>  drivers/firmware/efi/cper.c      | 51 +++++++++++++++---
>  include/acpi/ghes.h              |  4 ++
>  include/cxl/event.h              |  6 +--
>  7 files changed, 186 insertions(+), 52 deletions(-)

Sashiko has still reported a finding (medium-level) in patch [5/13]:

https://sashiko.dev/#/patchset/20260904172337.1409775-1-dave.jiang%40intel.com

but since the entire series has been reviewed by humans, I'm going to
apply it as is for 7.4 and if you think that the potential issue
pointed out in the above is worth taking care of, please send a
follow-up fix patch (which also applies to the pre-existing issues
reported there).

Thanks!