Re: [PATCH v2] cxl/pci: Fix NULL pointer dereference in reset detection

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

Pre-existing issues:
- [High] Incomplete fix for DVSEC-emulated decoders allows spurious reset warnings and kernel tainting.
- [High] Race condition between PCIe reset and sysfs driver unbind leading to NULL pointer dereference or UAF.
- [High] Data race on `cxld->flags` due to missing synchronization.
--

--- Patch [1]: [PATCH v2] cxl/pci: Fix NULL pointer dereference in reset detection ---
commit 0a3a66bfcbdad33db13f3c33dd88b689403ca622
Author: Guixin Liu <[email protected]>

cxl/pci: Fix NULL pointer dereference in reset detection

This commit adds a check for a NULL hdm pointer before reading the control
register in __cxl_endpoint_decoder_reset_detected(). This prevents a kernel
oops during PCIe resets on memory devices that emulate their HDM decoders
using DVSEC ranges instead of physical registers.

Link: https://lore.kernel.org/linux-cxl/[email protected]/

> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 9d807c1a002ca..611175f29d245 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -685,6 +685,14 @@ static int __cxl_endpoint_decoder_reset_detected(struct device *dev, void *data)
>  
>  	cxlhdm = dev_get_drvdata(&port->dev);
>  	hdm = cxlhdm->regs.hdm_decoder;

[Severity: High]
This is a pre-existing issue, but is there a race condition here between
the PCIe reset handling and sysfs driver unbind?

Looking at cxl_reset_done(), it iterates over the endpoint's children using
device_for_each_child() without holding the port's device lock:

drivers/cxl/pci.c:cxl_reset_done() {
    ...
    device_for_each_child(&cxlmd->endpoint->dev, NULL,
                          cxl_endpoint_decoder_clear_reset_flags);
    ...
}

If a concurrent sysfs unbind of the cxl_port driver clears the driver data
and frees the cxlhdm structure, could dev_get_drvdata() return NULL or a
stale pointer, leading to a crash when cxlhdm is dereferenced?

> +
> +	/*
> +	 * Devices that describe their HDM ranges with the DVSEC range
> +	 * registers have no HDM decoder registers to consult.
> +	 */
> +	if (!hdm)
> +		return 0;

[Severity: High]
This isn't a bug introduced by this patch, but does this check fully cover
all DVSEC-emulated decoders?

In should_emulate_decoders(), a device can fall back to DVSEC emulation
even when the hdm pointer is not NULL (for example, if HDM decoders are
present but globally disabled by firmware).

In this fallback case, the decoder is emulated but the hdm pointer remains
valid. This function would bypass the !hdm check, read the HDM control
register where the COMMITTED bit is likely 0, and falsely trigger a reset
warning.

Since cxl_setup_hdm_decoder_from_dvsec() sets cxld->commit to NULL for all
emulated decoders, would checking for emulation directly (e.g.,
if (!cxld->commit)) be a more robust approach?

> +
>  	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
>  
>  	return !FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl);
>  }

[Severity: High]
This is a pre-existing issue, but does modifying cxld->flags during reset
handling lack necessary synchronization?

In cxl_endpoint_decoder_clear_reset_flags(), which is called during
cxl_reset_done(), the flags are modified using a non-atomic operation:

drivers/cxl/pci.c:cxl_endpoint_decoder_clear_reset_flags() {
    ...
    cxled->cxld.flags &= ~CXL_DECODER_F_RESET_MASK;
    ...
}

This happens while holding only the parent memory device lock.
Concurrently, sysfs operations can commit a decoder:

cxl_decoder_commit() {
    ...
    cxld->flags |= CXL_DECODER_F_ENABLE;
    ...
}

Because cxld->flags is modified concurrently without a common lock or atomic
bit operations, could flag updates be lost, potentially causing the loss of
the ENABLE flag?

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