[PATCH 0/3] Harden HDM decoder enumeration

[PATCH 0/3] Harden HDM decoder enumeration

[Alison Schofield]

Here are 3 Sashiko reported cleanups.
Only the first includes a link to the complaint, since the others did
not originally appear on Lore for me to add public links. 

Srirangan: I believe the first 2 of these also address pre-existing
issues Sashiko posted in reply to your reset series, Patch 02/11.


Alison Schofield (3):
  cxl/hdm: Reject switch decoder interleave ways that overflow targets
  cxl/hdm: Make switch decoder target parsing endian-safe
  cxl/hdm: Restore commit_end when decoder enumeration fails

 drivers/cxl/core/hdm.c | 31 +++++++++++++++++++++++++------
 drivers/cxl/cxl.h      |  2 ++
 2 files changed, 27 insertions(+), 6 deletions(-)


base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
-- 
2.37.3

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

[Alison Schofield]

Switch decoder enumeration validates that the interleave ways encoding
is legal, but not that the resulting number of ways fits the available
targets.

A decoder reporting more interleave ways than available targets can
overflow the target arrays during enumeration and target population.

Reject decoders during enumeration if their interleave ways exceed
the hardware target list or the reported target count.

Link: https://sashiko.dev/#/patchset/20260711180755.1779002-1-alison.schofield@intel.com?part=1
Fixes: d17d0540a0db ("cxl/core/hdm: Add CXL standard decoder enumeration to the core")
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/hdm.c | 18 ++++++++++++++++++
 drivers/cxl/cxl.h      |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 0c80b76a5f9b..d86a607d2a2c 100644
--- 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) {
+			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) {
+			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;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index c0e5308e4d1b..6dda96c42849 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -67,6 +67,8 @@ extern const struct nvdimm_security_ops *cxl_security_ops;
 #define   CXL_HDM_DECODER0_CTRL_HOSTONLY BIT(12)
 #define CXL_HDM_DECODER0_TL_LOW(i) (0x20 * (i) + 0x24)
 #define CXL_HDM_DECODER0_TL_HIGH(i) (0x20 * (i) + 0x28)
+/* Target list is two 32-bit registers holding one target id per byte */
+#define CXL_HDM_DECODER0_TL_TARGETS 8
 #define CXL_HDM_DECODER0_SKIP_LOW(i) CXL_HDM_DECODER0_TL_LOW(i)
 #define CXL_HDM_DECODER0_SKIP_HIGH(i) CXL_HDM_DECODER0_TL_HIGH(i)
 
-- 
2.37.3

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

[sashiko-bot]

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 <alison.schofield@intel.com>

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/20260711180755.1779002-1-alison.schofield@intel.com?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/cover.1783974681.git.alison.schofield@intel.com?part=1

[PATCH 2/3] cxl/hdm: Make switch decoder target parsing endian-safe

[Alison Schofield]

Switch decoder target IDs are stored one per byte, starting with target
0 in the least significant byte. The current code accesses those bytes
in memory order, which only matches register order on little-endian
hosts.

On big-endian hosts, the target IDs are reversed and the decoder is
associated with the wrong downstream ports.

Extract each target ID from the register value with an explicit shift
and mask so the target mapping is independent of host endianness.

Fixes: d17d0540a0db ("cxl/core/hdm: Add CXL standard decoder enumeration to the core")
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/hdm.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index d86a607d2a2c..75d17120233d 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -974,14 +974,11 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 {
 	struct cxl_endpoint_decoder *cxled = NULL;
 	u64 size, base, skip, dpa_size, lo, hi;
+	u64 target_list;
 	bool committed;
 	u32 remainder;
 	int i, rc;
 	u32 ctrl;
-	union {
-		u64 value;
-		unsigned char target_id[8];
-	} target_list;
 
 	if (should_emulate_decoders(info))
 		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, dpa_base,
@@ -1104,9 +1101,9 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 
 		lo = readl(hdm + CXL_HDM_DECODER0_TL_LOW(which));
 		hi = readl(hdm + CXL_HDM_DECODER0_TL_HIGH(which));
-		target_list.value = (hi << 32) + lo;
+		target_list = (hi << 32) + lo;
 		for (i = 0; i < cxld->interleave_ways; i++)
-			cxld->target_map[i] = target_list.target_id[i];
+			cxld->target_map[i] = (target_list >> (i * 8)) & 0xff;
 
 		return 0;
 	}
-- 
2.37.3

[PATCH 3/3] cxl/hdm: Restore commit_end when decoder enumeration fails

[Alison Schofield]

commit_end tracks the highest committed decoder on a port. It gets
advanced before decoder enumeration is complete, so a later failure
leaves it pointing at a decoder that was never added. The next probe
then rejects decoder0 as out of order and enumeration fails.

Reset commit_end before enumerating so it is rebuilt from the decoders
actually committed on each probe.

Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
Fixes: b777e9bec960 ("cxl/hdm: Emulate HDM decoder from DVSEC range registers")
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/hdm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 75d17120233d..0145d3db3107 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -1178,6 +1178,10 @@ static int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
 
 	cxl_settle_decoders(cxlhdm);
 
+	/* Reset to the none committed baseline for a fresh enumeration */
+	scoped_guard(rwsem_write, &cxl_rwsem.region)
+		port->commit_end = -1;
+
 	for (i = 0; i < cxlhdm->decoder_count; i++) {
 		int rc, target_count = cxlhdm->target_count;
 		struct cxl_decoder *cxld;
-- 
2.37.3

Re: [PATCH 3/3] cxl/hdm: Restore commit_end when decoder enumeration fails

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Transient reset of `port->commit_end` allows concurrent bypass of safety checks in `cxl_mem_sanitize`, potentially destroying active memory.
--

--- Patch [3]: [PATCH 3/3] cxl/hdm: Restore commit_end when decoder enumeration fails ---
commit 0d7f87c9ca0ba7b9bbdd31e5ea90994445b1c9b5
Author: Alison Schofield <alison.schofield@intel.com>

cxl/hdm: Restore commit_end when decoder enumeration fails

This commit resets port->commit_end to -1 before enumerating decoders so
the commit state is rebuilt from the decoders actually committed on each
probe. This prevents previous enumeration failures from causing subsequent
probes to fail.

> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 75d17120233da..0145d3db31070 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -1178,6 +1178,10 @@ static int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
>  
>  	cxl_settle_decoders(cxlhdm);
>  
> +	/* Reset to the none committed baseline for a fresh enumeration */
> +	scoped_guard(rwsem_write, &cxl_rwsem.region)
> +		port->commit_end = -1;

[Severity: High]
Does this scoped_guard drop the lock too early?

Because scoped_guard only holds the lock for the following single statement,
the rwsem_write lock is released immediately after setting port->commit_end
to -1. This leaves the transient -1 state exposed to readers without protection
while the loop below performs the hardware enumeration.

Could a concurrent reader like cxl_mem_sanitize() acquire the read lock
during this window, observe a temporarily empty committed decoder count
(since cxl_num_decoders_committed() evaluates the exposed -1), and 
erroneously bypass its safety checks to securely erase actively mapped memory?

> +
>  	for (i = 0; i < cxlhdm->decoder_count; i++) {
>  		int rc, target_count = cxlhdm->target_count;
>  		struct cxl_decoder *cxld;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783974681.git.alison.schofield@intel.com?part=3