Re: [PATCH v18 01/13] cxl/ras: Fix cxl_rch_get_aer_severity() wrong severity register

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

Pre-existing issues:
- [High] cxl_rch_get_aer_info() uses sizeof(struct aer_capability_regs) to perform a linear MMIO read block copy, but the struct contains software-defined fields that break physical hardware layout parity, causing out-of-bounds MMIO reads and a stack data leak.
- [Medium] Correctable errors are silently dropped and permanently lost if uncorrectable errors are simultaneously present.
--

--- Patch [1]: [PATCH v18 01/13] cxl/ras: Fix cxl_rch_get_aer_severity() wrong severity register ---
commit 900d8b615c2219573e44e775aca2f70550afbe9a
Author: Terry Bowman <[email protected]>
cxl/ras: Fix cxl_rch_get_aer_severity() wrong severity register

This commit fixes the AER severity classification for RCH Downstream
Port uncorrectable errors by correctly checking the unmasked uncor_status
against uncor_severity.

Cc: [email protected]

> diff --git a/drivers/cxl/core/ras_rch.c b/drivers/cxl/core/ras_rch.c
> index 0a8b3b9b63884..44b335d560708 100644
> --- a/drivers/cxl/core/ras_rch.c
> +++ b/drivers/cxl/core/ras_rch.c
> @@ -80,7 +80,8 @@ static bool cxl_rch_get_aer_severity(struct aer_capability_regs *aer_regs,
>  				     int *severity)
>  {
>  	if (aer_regs->uncor_status & ~aer_regs->uncor_mask) {
> -		if (aer_regs->uncor_status & PCI_ERR_ROOT_FATAL_RCV)
> +		if ((aer_regs->uncor_status & ~aer_regs->uncor_mask) &
> +		    aer_regs->uncor_severity)
>  			*severity = AER_FATAL;
>  		else
>  			*severity = AER_NONFATAL;

[Severity: Medium]
This isn't a bug introduced by this patch, but does this logic permanently
lose correctable errors if both types are logged simultaneously?

Looking at cxl_rch_get_aer_info(), it reads the entire AER capability and
clears both error types in hardware:

drivers/cxl/core/ras_rch.c:cxl_rch_get_aer_info() {
    ...
    writel(aer_regs->uncor_status, aer_base + PCI_ERR_UNCOR_STATUS);
    writel(aer_regs->cor_status, aer_base + PCI_ERR_COR_STATUS);
    ...
}

Then here in cxl_rch_get_aer_severity(), it evaluates the uncorrectable
error, assigns the severity, and returns early without checking for
correctable errors:

drivers/cxl/core/ras_rch.c:cxl_rch_get_aer_severity() {
    if (aer_regs->uncor_status & ~aer_regs->uncor_mask) {
        ...
        return true;
    }
    ...
}

Returning true immediately bypasses the correctable error check, preventing
cxl_handle_cor_ras() from being called on the same pass.

[Severity: High]
This is a pre-existing issue, but do the MMIO reads in the related
cxl_rch_get_aer_info() perform out-of-bounds hardware access?

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

Since struct aer_capability_regs includes struct pcie_tlp_log, recent
additions to struct pcie_tlp_log (such as header_len and flit) increase
the size of the software struct well beyond the standard physical AER
capability size.

Would this loop copy unmapped PCIe config space (typically returning
0xFFFFFFFF) into software fields like header_len, leading
pcie_print_tlp_log() to later loop over an inflated log->header_len value
and trigger an out-of-bounds read on the kernel stack?

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