[PATCH v3] cxl/edac: Fail ECS threshold read for reserved encodings

[PATCH v3] cxl/edac: Fail ECS threshold read for reserved encodings

[Yili Zhang]

cxl_get_ecs_threshold() extracts a 3-bit index (0-7) from the
device-supplied ECS config word and uses it to index
ecs_supp_threshold[], which only has 6 elements. A device reporting
index 6 or 7 causes an out-of-bounds read of adjacent .rodata, whose
value is then returned to userspace via the EDAC 'threshold' sysfs
attribute (small info leak and wrong reported threshold).

The threshold count field also has reserved encodings 0-2, which
currently read as 0 from the sparse lookup table. That behavior is
not intentional; it is just an artifact of the array's implicit
zero-fill. The ABI documentation only lists 256, 1024 and 4096 as
supported values, and the write side (cxl_set_ecs_threshold())
rejects everything else, so a reserved encoding can only come from
the device reporting it. Rather than documenting 0 as well, make the
read fail for all reserved encodings and only ever report the
documented values.

None of these encodings can be written through the sysfs write path,
and no device is known to report them, so this is hardening against
a device returning values not covered by the spec rather than a fix
for known hardware.

Convert the cxl_get_ecs_*() helpers to return an int and write the
attribute value through an output parameter (u32 * to match the
macro), so that errors propagate through the shared
CXL_ECS_GET_ATTR() macro to the sysfs read.

Suggested-by: Alison Schofield <alison.schofield@intel.com>
Signed-off-by: Yili Zhang <zhangyili01@baidu.com>
---
v3: Per review feedback, drop the Fixes tag and reword the
description to make clear this is hardening against a device
reporting values not covered by the spec rather than a fix
(Jonathan Cameron).

v2: Per review feedback, fail the read with -EINVAL for all reserved
encodings instead of returning 0, converting the cxl_get_ecs_*()
getters to an int return with an output parameter so the error
reaches the sysfs read (Alison Schofield).

v1: https://lore.kernel.org/all/20260928073553.85669-1-zhangyili01@baidu.com/
v2: https://lore.kernel.org/all/20260930095627.32616-1-zhangyili01@baidu.com/
---
 drivers/cxl/core/edac.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
index b321971fef58..fd6612dde7f1 100644
--- a/drivers/cxl/core/edac.c
+++ b/drivers/cxl/core/edac.c
@@ -624,21 +624,31 @@ static int cxl_mem_ecs_set_attrbs(struct device *dev,
 			       0, NULL);
 }
 
-static u8 cxl_get_ecs_log_entry_type(u8 log_cap, u16 config)
+static int cxl_get_ecs_log_entry_type(u8 log_cap, u16 config, u32 *val)
 {
-	return FIELD_GET(CXL_ECS_LOG_ENTRY_TYPE_MASK, log_cap);
+	*val = FIELD_GET(CXL_ECS_LOG_ENTRY_TYPE_MASK, log_cap);
+
+	return 0;
 }
 
-static u16 cxl_get_ecs_threshold(u8 log_cap, u16 config)
+static int cxl_get_ecs_threshold(u8 log_cap, u16 config, u32 *val)
 {
 	u8 index = FIELD_GET(CXL_ECS_THRESHOLD_COUNT_MASK, config);
 
-	return ecs_supp_threshold[index];
+	if (index >= ARRAY_SIZE(ecs_supp_threshold) ||
+	    !ecs_supp_threshold[index])
+		return -EINVAL;
+
+	*val = ecs_supp_threshold[index];
+
+	return 0;
 }
 
-static u8 cxl_get_ecs_count_mode(u8 log_cap, u16 config)
+static int cxl_get_ecs_count_mode(u8 log_cap, u16 config, u32 *val)
 {
-	return FIELD_GET(CXL_ECS_COUNT_MODE_MASK, config);
+	*val = FIELD_GET(CXL_ECS_COUNT_MODE_MASK, config);
+
+	return 0;
 }
 
 #define CXL_ECS_GET_ATTR(attrb)						    \
@@ -655,9 +665,7 @@ static u8 cxl_get_ecs_count_mode(u8 log_cap, u16 config)
 		if (ret)						    \
 			return ret;					    \
 									    \
-		*val = cxl_get_ecs_##attrb(log_cap, config);		    \
-									    \
-		return 0;						    \
+		return cxl_get_ecs_##attrb(log_cap, config, val);	    \
 	}
 
 CXL_ECS_GET_ATTR(log_entry_type)
-- 
2.27.0

Re: [PATCH v3] cxl/edac: Fail ECS threshold read for reserved encodings

[Cheatham, Benjamin]

On 10/21/2026 10:20 AM, Yili Zhang wrote:
> cxl_get_ecs_threshold() extracts a 3-bit index (0-7) from the
> device-supplied ECS config word and uses it to index

Nit: Should probably spell out ECS here for people (like myself) who don't
know what it means.
> ecs_supp_threshold[], which only has 6 elements. A device reporting
> index 6 or 7 causes an out-of-bounds read of adjacent .rodata, whose
> value is then returned to userspace via the EDAC 'threshold' sysfs
> attribute (small info leak and wrong reported threshold).
> 
> The threshold count field also has reserved encodings 0-2, which
> currently read as 0 from the sparse lookup table. That behavior is
> not intentional; it is just an artifact of the array's implicit
> zero-fill. The ABI documentation only lists 256, 1024 and 4096 as
> supported values, and the write side (cxl_set_ecs_threshold())
> rejects everything else, so a reserved encoding can only come from
> the device reporting it. Rather than documenting 0 as well, make the
> read fail for all reserved encodings and only ever report the
> documented values.
> 
> None of these encodings can be written through the sysfs write path,
> and no device is known to report them, so this is hardening against
> a device returning values not covered by the spec rather than a fix
> for known hardware.
> 
> Convert the cxl_get_ecs_*() helpers to return an int and write the
> attribute value through an output parameter (u32 * to match the
> macro), so that errors propagate through the shared
> CXL_ECS_GET_ATTR() macro to the sysfs read.
> 
> Suggested-by: Alison Schofield <alison.schofield@intel.com>
> Signed-off-by: Yili Zhang <zhangyili01@baidu.com>
> ---

I was going to make a suggestion about updating the documentation to mention
this new behavior, but noticed the other attributes in the ABI docs don't mention
reads of reserved values resulting in an error. The doc mentions that these attributes
are implemented by the device's driver (duh), so updating the general docs doesn't make
much sense.

So I'm going to take it a step further and ask if we should be failing the write at all?
It seems like other driver implementations probably don't, so it would be weird if just the
CXL driver did. I haven't looked for any other callers, so if CXL is the only user then
the docs should probably be updated so other implementers can follow the same convention.
If not, then it may be worth looking at the other call sites to update their behavior.

That may suck to do if multiple subsystems are involved, so if everyone else is fine with CXL being
different then I guess I am too :)

Thanks,
Ben