[PATCH] cxl/edac: Bounds-check ECS threshold index from device

[PATCH] cxl/edac: Bounds-check ECS threshold index from device

[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).

Return 0 for indices outside the table.

Fixes: 85fb6a16ad14 ("cxl/edac: Add CXL memory device ECS control feature")
Signed-off-by: Yili Zhang <zhangyili01@baidu.com>
---
 drivers/cxl/core/edac.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
index b321971fef58..73335b980b11 100644
--- a/drivers/cxl/core/edac.c
+++ b/drivers/cxl/core/edac.c
@@ -633,6 +633,9 @@ static u16 cxl_get_ecs_threshold(u8 log_cap, u16 config)
 {
 	u8 index = FIELD_GET(CXL_ECS_THRESHOLD_COUNT_MASK, config);
 
+	if (index >= ARRAY_SIZE(ecs_supp_threshold))
+		return 0;
+
 	return ecs_supp_threshold[index];
 }
 
-- 
2.27.0

Re: [PATCH] cxl/edac: Bounds-check ECS threshold index from device

[Dave Jiang]

On 9/28/26 12:35 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 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).
> 
> Return 0 for indices outside the table.

Why return 0 instead of errno?

DJ

> 
> Fixes: 85fb6a16ad14 ("cxl/edac: Add CXL memory device ECS control feature")
> Signed-off-by: Yili Zhang <zhangyili01@baidu.com>
> ---
>  drivers/cxl/core/edac.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
> index b321971fef58..73335b980b11 100644
> --- a/drivers/cxl/core/edac.c
> +++ b/drivers/cxl/core/edac.c
> @@ -633,6 +633,9 @@ static u16 cxl_get_ecs_threshold(u8 log_cap, u16 config)
>  {
>  	u8 index = FIELD_GET(CXL_ECS_THRESHOLD_COUNT_MASK, config);
>  
> +	if (index >= ARRAY_SIZE(ecs_supp_threshold))
> +		return 0;
> +
>  	return ecs_supp_threshold[index];
>  }
>  

Re: [PATCH] cxl/edac: Bounds-check ECS threshold index from device

[Yili Zhang]

On 2026-09-28 11:21, Dave Jiang wrote:
>
> On 9/28/26 12:35 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 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).
> >
> > Return 0 for indices outside the table.
>
> Why return 0 instead of errno?

The threshold count field is 3 bits wide, with encodings 0-2 and
6-7 reserved. ecs_supp_threshold[] is a sparse array that only
initializes entries 3-5 (256/1024/4096), so when a device reports
one of the reserved encodings 0-2, the current code already
returns 0. The patch just extends the same treatment to the
remaining reserved indices instead of reading past the table.
0 is also not a valid threshold value, so userspace can tell it
apart from a real one.

Returning an errno would additionally require open-coding a
separate getter for this one attribute, as it goes through the
shared CXL_ECS_GET_ATTR() macro, which cannot propagate errors.

So I'd prefer to keep returning 0 here.

Thanks,
Yili

Re: [PATCH] cxl/edac: Bounds-check ECS threshold index from device

[Alison Schofield]

On Tue, Sep 29, 2026 at 11:17:42AM +0800, Yili Zhang wrote:
> On 2026-09-28 11:21, Dave Jiang wrote:
> >
> > On 9/28/26 12:35 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 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).
> > >
> > > Return 0 for indices outside the table.
> >
> > Why return 0 instead of errno?
> 
> The threshold count field is 3 bits wide, with encodings 0-2 and
> 6-7 reserved. ecs_supp_threshold[] is a sparse array that only
> initializes entries 3-5 (256/1024/4096), so when a device reports
> one of the reserved encodings 0-2, the current code already
> returns 0. The patch just extends the same treatment to the
> remaining reserved indices instead of reading past the table.
> 0 is also not a valid threshold value, so userspace can tell it
> apart from a real one.

Hi Yili,

I see the appeal of keeping this as the smallest possible fix, but is the
existing behavior for 0-2 something we intentionally want to preserve, or
just how the current implementation works?

Documentation/ABI/testing/sysfs-edac-ecs lists 256, 1024, and 4096 as the
threshold values. If 0 is meant to represent a reserved encoding, should
that be part of the documented ABI?

> 
> Returning an errno would additionally require open-coding a
> separate getter for this one attribute, as it goes through the
> shared CXL_ECS_GET_ATTR() macro, which cannot propagate errors.

Does CXL_ECS_GET_ATTR() itself prevent that? It already has an int return
path and can return errors from cxl_mem_ecs_get_attrbs().

Is the real limitation just the interface used by the cxl_get_ecs_*()
helpers? Would something like this work:


static int cxl_get_ecs_threshold(u8 log_cap, u16 config, u16 *threshold)
{
	u8 index = FIELD_GET(CXL_ECS_THRESHOLD_COUNT_MASK, config);

	if (index >= ARRAY_SIZE(ecs_supp_threshold) ||
	    !ecs_supp_threshold[index])
		return -EINVAL;

	*threshold = ecs_supp_threshold[index];
	return 0;
}

-- Alison

> 
> So I'd prefer to keep returning 0 here.
> 
> Thanks,
> Yili

Re: [PATCH] cxl/edac: Bounds-check ECS threshold index from device

[Yili Zhang]

On 2026-09-29 10:53, Alison Schofield wrote:
> 
> Hi Yili,
> 
> I see the appeal of keeping this as the smallest possible fix, but is the
> existing behavior for 0-2 something we intentionally want to preserve, or
> just how the current implementation works?
> 
> Documentation/ABI/testing/sysfs-edac-ecs lists 256, 1024, and 4096 as the
> threshold values. If 0 is meant to represent a reserved encoding, should
> that be part of the documented ABI?

Hi Alison,

You're right on both counts.

The 0-2 behavior is not something we want to preserve; it is just
an artifact of the sparse array's implicit zero-fill. The ABI documentation
only lists 256, 1024 and 4096 as supported values, so rather than documenting
0 as well, make the read fail for all reserved encodings and only ever report
the documented values.

> Does CXL_ECS_GET_ATTR() itself prevent that? It already has an int return
> path and can return errors from cxl_mem_ecs_get_attrbs().
> 
> Is the real limitation just the interface used by the cxl_get_ecs_*()
> helpers? Would something like this work:
> 
> 
> static int cxl_get_ecs_threshold(u8 log_cap, u16 config, u16 *threshold)
> {
> 	u8 index = FIELD_GET(CXL_ECS_THRESHOLD_COUNT_MASK, config);
> 
> 	if (index >= ARRAY_SIZE(ecs_supp_threshold) ||
> 	    !ecs_supp_threshold[index])
> 		return -EINVAL;
> 
> 	*threshold = ecs_supp_threshold[index];
> 	return 0;
> }

And you're correct that CXL_ECS_GET_ATTR() already propagates errors;
the only real limitation was the helper signature, as you sketched.
v2 restructures the cxl_get_ecs_*() helpers to return int with an
output parameter and updates the macro accordingly.

v2 follows shortly.

Thanks,
Yili

[PATCH v2] 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, so rather than documenting 0 as well, make the
read fail for all reserved encodings and only ever report the
documented values.

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.

Fixes: 85fb6a16ad14 ("cxl/edac: Add CXL memory device ECS control feature")
Suggested-by: Alison Schofield <alison.schofield@intel.com>
Signed-off-by: Yili Zhang <zhangyili01@baidu.com>
---
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.
---
 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