Re: [PATCH 1/3] cxl/hdm: Reject switch decoder interleave ways that overflow targets

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

New issues:
- [High] Validation of `interleave_ways` returns a hard error for uncommitted decoders, causing a fatal probe failure if an inactive decoder contains leftover or uninitialized configuration.
- [High] Due to a preexisting off-by-one error in `nr_targets`, the new check `cxld->interleave_ways > cxlsd->nr_targets` will falsely reject valid max-width interleaved configurations.

Pre-existing issues:
- [Critical] Target list array accesses are endian-dependent, leading to corrupted target mapping on Big-Endian systems.
--

--- Patch [1]: [PATCH 1/3] cxl/hdm: Reject switch decoder interleave ways that overflow targets ---
commit 0f855df703f711546a94bba79b4742022fb3bae7
Author: Alison Schofield <[email protected]>

cxl/hdm: Reject switch decoder interleave ways that overflow targets

This commit adds validation during CXL switch decoder enumeration to ensure
that the number of interleave ways does not exceed either the hardware
target list capacity or the reported target count. If the ways exceed
these values, enumeration fails with an error to prevent array overflows.

Link: https://sashiko.dev/#/patchset/[email protected]?part=1

> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -1084,6 +1084,24 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  		cxld->interleave_ways, cxld->interleave_granularity);
>  
>  	if (!cxled) {
> +		struct cxl_switch_decoder *cxlsd =
> +			to_cxl_switch_decoder(&cxld->dev);
> +
> +		if (cxld->interleave_ways > CXL_HDM_DECODER0_TL_TARGETS) {

[Severity: High]
Will this return a hard error for uncommitted decoders?

If an inactive or uncommitted decoder contains uninitialized or leftover
state (such as from BIOS) in its control register, this bounds check will
fail and return -ENXIO. 

Because this validation happens before checking if the decoder is actually
committed, does this abort the initialization of subsequent decoders and
prevent the CXL switch port from probing?

devm_cxl_switch_port_decoders_setup()
  -> devm_cxl_enumerate_decoders()
    -> init_hdm_decoder()

> +			dev_err(&port->dev,
> +				"decoder%d.%d: interleave ways: %d exceeds target list capacity: %d\n",
> +				port->id, cxld->id, cxld->interleave_ways,
> +				CXL_HDM_DECODER0_TL_TARGETS);
> +			return -ENXIO;
> +		}
> +		if (cxld->interleave_ways > cxlsd->nr_targets) {

[Severity: High]
Is cxlsd->nr_targets a 0-based count?

In parse_hdm_decoder_caps(), the 0-based hardware capability "Target Count"
is assigned to cxlhdm->target_count without adjustment, making
cxlsd->nr_targets 0-based (where 0 means 1 target, 1 means 2 targets, etc.).

Since interleave_ways is a 1-based exact count, won't this check incorrectly
reject valid configurations? 

For example, a 2-way interleave across 2 targets evaluates as 2 > 1,
incorrectly returning -ENXIO and preventing CXL memory enumeration on valid
hardware topologies.

> +			dev_err(&port->dev,
> +				"decoder%d.%d: interleave ways: %d exceeds targets: %d\n",
> +				port->id, cxld->id, cxld->interleave_ways,
> +				cxlsd->nr_targets);
> +			return -ENXIO;
> +		}
> +
>  		lo = readl(hdm + CXL_HDM_DECODER0_TL_LOW(which));
>  		hi = readl(hdm + CXL_HDM_DECODER0_TL_HIGH(which));
>  		target_list.value = (hi << 32) + lo;

[Severity: Critical]
This isn't a bug introduced by this patch, but does assigning
target_list.value this way cause endian-dependent target mapping on
Big-Endian systems?

By reading the lo and hi registers and combining them into a host-endian
64-bit integer, accessing it as a byte array (e.g., via
target_list.target_id[i]) will read the most significant byte on Big-Endian
architectures instead of the expected byte.

Could this lead to memory traffic being routed to the wrong CXL target on
architectures like PowerPC or s390x?

[ ... ]

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