Re: [PATCH v18 07/13] PCI/CXL: Add RCH support to CXL handlers

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

New issues:
- [High] Removal of explicit PCI error handler forwarding for RCH devices leaves RCDs bound after a fatal PCI error.

Pre-existing issues:
- [High] Preexisting lockless dereference of `dport` returned by `cxl_pci_find_port()` and `cxl_mem_find_port()` leading to Use-After-Free.
--

--- Patch [7]: [PATCH v18 07/13] PCI/CXL: Add RCH support to CXL handlers ---
commit 6a99c70b18865d01d350aa367342e701ebe6dde2
Author: Terry Bowman <[email protected]>

PCI/CXL: Add RCH support to CXL handlers

Restricted CXL Host (RCH) error handling is folded into the new CXL Port
error handling flow. RCH protocol errors are forwarded through the AER-CXL
kfifo, and the manual CXL.mem error handler invocation is removed.

> diff --git a/drivers/cxl/core/ras_rch.c b/drivers/cxl/core/ras_rch.c
> --- a/drivers/cxl/core/ras_rch.c
> +++ b/drivers/cxl/core/ras_rch.c
> @@ -96,18 +95,21 @@ static bool cxl_rch_get_aer_severity(struct aer_capability_regs *aer_regs,
>  	return false;
>  }
>  
> -void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds)
> +void cxl_handle_rdport_errors(struct pci_dev *pdev)
>  {
> -	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
>  	struct aer_capability_regs aer_regs;
>  	struct cxl_dport *dport;
>  	int severity;
>  
> -	struct cxl_port *port __free(put_cxl_port) =
> -		cxl_pci_find_port(pdev, &dport);
> +	struct cxl_port *port __free(put_cxl_port) = cxl_pci_find_port(pdev, NULL);
>  	if (!port)
>  		return;
>  
> +	guard(device)(&port->dev);
> +	dport = cxl_find_dport_by_dev(port, pdev->dev.parent);

[Severity: High]
This is a pre-existing issue, but while this change correctly closes a
lockless Use-After-Free window for dport here, does the same vulnerability
still exist in cxl_pci_setup_regs() and rcd_pcie_cap_emit()?

In cxl_pci_setup_regs(), cxl_rcrb_get_comp_regs() dereferences the dport
returned by cxl_pci_find_port() before any lock is taken.

Similarly, in rcd_pcie_cap_emit(), dport->regs.rcd_pcie_cap is dereferenced
entirely locklessly, and then the function takes guard(device)(root_dev)
instead of the required &root->dev lock.

Since rcd_pcie_cap_emit() is reachable via sysfs attributes, could an
unprivileged local user trigger a Use-After-Free by reading sysfs during a
concurrent device hot-unplug?

[ ... ]

> diff --git a/drivers/pci/pcie/aer_cxl_rch.c b/drivers/pci/pcie/aer_cxl_rch.c
> --- a/drivers/pci/pcie/aer_cxl_rch.c
> +++ b/drivers/pci/pcie/aer_cxl_rch.c
> @@ -34,42 +34,37 @@ static bool cxl_error_is_native(struct pci_dev *dev)
>  	return (pcie_ports_native || host->native_aer);
>  }
>  
> +struct cxl_rch_error_ctx {
> +	struct aer_err_info *info;
> +	bool enqueued;
> +};
> +
>  static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data)
>  {
> -	struct aer_err_info *info = (struct aer_err_info *)data;
> -	const struct pci_error_handlers *err_handler;
> +	struct cxl_rch_error_ctx *ctx = data;
>  
>  	if (!is_cxl_mem_dev(dev) || !cxl_error_is_native(dev))
>  		return 0;
>  
> -	guard(device)(&dev->dev);
> -
> -	err_handler = dev->driver ? dev->driver->err_handler : NULL;
> -	if (!err_handler)
> -		return 0;
> -
> -	if (info->severity == AER_CORRECTABLE) {
> -		if (err_handler->cor_error_detected)
> -			err_handler->cor_error_detected(dev);
> -	} else if (err_handler->error_detected) {
> -		if (info->severity == AER_NONFATAL)
> -			err_handler->error_detected(dev, pci_channel_io_normal);
> -		else if (info->severity == AER_FATAL)
> -			err_handler->error_detected(dev, pci_channel_io_frozen);
> -	}
> +	if (cxl_forward_error(dev, ctx->info))
> +		ctx->enqueued = true;

[Severity: High]
Does removing the explicit invocation of the err_handler leave RCDs bound
after a fatal PCI error?

The standard pcie_do_recovery() on an RCEC does not iterate over its
associated RC_END (RCD) devices, so this manual invocation used to handle the
CXL.mem driver fallback. Now that errors are routed solely through the CXL
protocol error work queue (__cxl_proto_err_work_fn()), the work queue only
panics on uncorrectable cachemem errors.

Does the work queue replicate the PCI recovery state machine's fallback
behavior, such as calling device_release_driver() to unbind the CXL.mem
driver on frozen channel states?

>  	return 0;
>  }

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