Re: [PATCH v2] cxl/port: Fix missing port lock in cxl_dport_remove()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incomplete fix for use-after-free vulnerabilities. The patch fixes use-after-free races in `cxl_mem_probe()` and `rcd_pcie_cap_emit()` by avoiding the dereference of locklessly-obtained `dport` pointers. However, it missed identical vulnerabilities in `cxl_pci_setup_regs()` and `cxl_handle_rdport_errors()`. Both functions use `cxl_pci_find_port()` to locklessly capture a `dport` pointer (via `xa_load`) and then immediately dereference it without holding a device lock (such as `uport_dev->mutex`) that would exclude concurrent devres teardown. A concurrent surprise removal could cause `cxl_dport_remove()` and `free_dport()` to free the `dport` structure while these functions are dereferencing it, leading to a Use-After-Free.
- [High] Use-After-Free in the error path of `devm_cxl_add_nvdimm()` due to a cleanup ordering violation. The function holds the device lock via `guard(device)(&cxl_nvb->dev)`. On the `err_alloc` failure path, it manually calls `put_device(&cxl_nvb->dev)`. If this is the last reference to the device, the object is immediately freed. Upon returning, the `guard` goes out of scope and calls `device_unlock(&cxl_nvb->dev)` on the already-freed memory, resulting in a use-after-free.
--

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

cxl/port: Fix missing port lock in cxl_dport_remove()

This commit addresses a use-after-free race condition in cxl_dport_remove()
by properly managing the port lock for root ports. It also updates callers
to re-fetch the dport pointer under the appropriate locks.

Link: https://lore.kernel.org/linux-cxl/20260505173029.2718246-1-terry.bowman@amd.com/

> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index ab88eaa31d1db..419ae58acce81 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
[ ... ]
> @@ -133,21 +132,57 @@ static int cxl_mem_probe(struct device *dev)
[ ... ]
> +	if (rch) {
> +		struct device *uport_dev = parent_port->uport_dev;
> +
> +		scoped_guard(device, uport_dev) {
> +			if (!uport_dev->driver) {
> +				dev_err(dev, "CXL port topology %s not enabled\n",
> +					dev_name(uport_dev));
> +				return -ENXIO;
> +			}
> +			/*
> +			 * Re-fetch dport under uport_dev lock.  uport_dev->mutex
> +			 * is held for the full devres teardown sequence including
> +			 * free_dport()/kfree(), so this excludes concurrent
> +			 * hotplug removal through the entire dereference.
> +			 */
> +			dport = cxl_find_dport_by_dev(parent_port, cxlmd->dev.parent->parent);
> +			if (!dport) {
> +				dev_err(dev, "CXL RCH dport not found\n");
> +				return -ENXIO;
> +			}
> +			rc = devm_cxl_add_endpoint(uport_dev, cxlmd, dport);
> +			if (rc)
> +				return rc;
> +		}
>  	}

[Severity: High]
This is a pre-existing issue, but while this patch fixes use-after-free
races here and in rcd_pcie_cap_emit() by avoiding the dereference of
locklessly-obtained dport pointers, could similar vulnerabilities still
exist in other functions?

For example, in drivers/cxl/core/pci.c:cxl_pci_setup_regs():

    rc = cxl_rcrb_get_comp_regs(pdev, map, dport);
    ...
    rc = cxl_dport_map_rcd_linkcap(pdev, dport);

And in drivers/cxl/core/ras_rch.c:cxl_handle_rdport_errors():

    if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs))

Both functions appear to use cxl_pci_find_port() to locklessly capture a
dport pointer (via xa_load) and dereference it without holding a device
lock (like uport_dev->mutex) that would exclude concurrent devres
teardown. Could a concurrent surprise removal cause cxl_dport_remove()
and free_dport() to free the dport structure while these functions are
dereferencing it?

[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free
in the error path of devm_cxl_add_nvdimm(), which is called during
cxl_mem_probe().

In drivers/cxl/core/pmem.c:devm_cxl_add_nvdimm():

    guard(device)(&cxl_nvb->dev);
    ...
    err_alloc:
        cxlmd->cxl_nvb = NULL;
        cxlmd->cxl_nvd = NULL;
        put_device(&cxl_nvb->dev);
    
        return rc;

Since guard() releases the device lock when returning, and the error path
manually calls put_device(&cxl_nvb->dev) before returning, wouldn't
device_unlock() be called on the already-freed memory if this was the
last reference, causing a use-after-free and lockdep crash?

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