Re: [PATCH v17 03/11] cxl: Use common CPER handling for all CXL devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Stack out-of-bounds read of 448 bytes when copying the CXL header log to the trace event ring buffer, leaking kernel stack memory to userspace.
- [High] Silent dropping of valid CPER hardware errors for unbound devices.
- [Medium] Unnecessary device locking based on a flawed premise of object lifetime.
--

commit fb7e6f8e6c582af55947d02985f007b61cfc1671
Author: Terry Bowman <terry.bowman@amd.com>

cxl: Use common CPER handling for all CXL devices

This commit unifies the CPER error handling paths for CXL Port and Endpoint
devices by removing the PCI-type specific dispatch. It extends the driver
check and device lock to Port devices, and replaces the memdev lookup with
direct use of the PCI device for trace events.

> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index d7081caaf5d3f..56611da8357a2 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c

[ ... ]

> @@ -77,38 +40,20 @@ void cxl_cper_handle_prot_err(struct cxl_cper_prot_err_work_data *data)
>  		pci_get_domain_bus_and_slot(data->prot_err.agent_addr.segment,
>  					    data->prot_err.agent_addr.bus,
>  					    devfn);
> -	struct cxl_memdev *cxlmd;
> -	int port_type;
>  
>  	if (!pdev)
>  		return;
>  
> -	port_type = pci_pcie_type(pdev);
> -	if (port_type == PCI_EXP_TYPE_ROOT_PORT ||
> -	    port_type == PCI_EXP_TYPE_DOWNSTREAM ||
> -	    port_type == PCI_EXP_TYPE_UPSTREAM) {
> -		if (data->severity == AER_CORRECTABLE)
> -			cxl_cper_trace_corr_port_prot_err(pdev, data->ras_cap);
> -		else
> -			cxl_cper_trace_uncorr_port_prot_err(pdev, data->ras_cap);
> -
> -		return;
> -	}
> -
>  	guard(device)(&pdev->dev);
>  	if (!pdev->dev.driver)
>  		return;

Will this silently drop valid CPER hardware errors for unbound Port devices?

Previously, the port device dispatch block correctly bypassed the driver check
for CXL ports. Since the code only logs the CPER data via tracepoints and
reads the PCI config space for the DSN, is a bound driver actually required?

Also, is the device lock necessary here?

The commit message notes "the existing device lock will ensure the CXL device
remains accessible while in scope". However, object lifetime and accessibility
of pdev are already guaranteed by the reference count acquired via
pci_get_domain_bus_and_slot(), which is released safely by the
__free(pci_dev_put) cleanup.

> -	struct device *mem_dev __free(put_device) = bus_find_device(
> -		&cxl_bus_type, NULL, pdev, match_memdev_by_parent);
> -	if (!mem_dev)
> -		return;
> -
> -	cxlmd = to_cxl_memdev(mem_dev);
>  	if (data->severity == AER_CORRECTABLE)
> -		cxl_cper_trace_corr_prot_err(pdev, cxlmd, data->ras_cap);
> +		cxl_cper_trace_corr_prot_err(pdev, pci_get_dsn(pdev),
> +					     &data->ras_cap);
>  	else
> -		cxl_cper_trace_uncorr_prot_err(pdev, cxlmd, data->ras_cap);
> +		cxl_cper_trace_uncorr_prot_err(pdev, pci_get_dsn(pdev),
> +					       &data->ras_cap);

Can this cause an out-of-bounds read of kernel stack memory?

The trace event cxl_aer_uncorrectable_error performs a memory copy of the
header log:

memcpy(__entry->header_log, hl, CXL_HEADERLOG_SIZE);

CXL_HEADERLOG_SIZE is 512 bytes, but the hl pointer passed in via
ras_cap->header_log is only a 16-element u32 array (64 bytes) inside
struct cxl_ras_capability_regs.

Could this copy 448 bytes of uninitialized stack memory past the array into
the userspace trace buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260505173029.2718246-1-terry.bowman@amd.com?part=3