[PATCH v6 00/13] ACPI: APEI: GHES: Collection of fixes for issues reported by sashiko
Dave Jiang <[email protected]>
| Newsgroups | org.kernel.vger.linux-acpi,org.kernel.vger.linux-cxl |
|---|---|
| Message-ID | <[email protected]> |
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(-)
base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
--
2.54.0