[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

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

[Jonathan Cameron]

On Wed, 30 Sep 2026 17:56:27 +0800
Yili Zhang <zhangyili01@baidu.com> wrote:

This just called out in the CXL upstream sync call as lacking
review!  Sorry about that - I missed it entirely :(


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

So this bit suggests this is hardening against bad device.
Definitely a good thing to do given it is fairly simple but
not a fix as such (so no rush to land it!)

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

For this one, can we trigger these values by writing a threshold of 0?
I think that is blocked by cxl_set_ecs_threshold() so I think this
one is also hardening against a bad device?

> 
> 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")

My overall suggestion is drop the fixes tag and state that this
is hardening against devices returning values not currently in the spec.

It might actually evolve to become a real fix with future hardware
changes using those reserved values but we have no reason to think
that will happen yet. 

We had a small discussion on the call about fixes for bad hardware
and I expressed my view that if they are cheap (like here) in
maintenance burden by all means take them, but I'll push back on
anything that needs very complex solutions.

With this minor tweak to the commit description this patch looks
good to me.


> Suggested-by: Alison Schofield <alison.schofield@intel.com>
> Signed-off-by: Yili Zhang <zhangyili01@baidu.com>

Updated Shiju's email address in the +CC.


> ---
> 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;
>  }

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

[Yili Zhang]

On 2026-10-20 22:27, Jonathan Cameron wrote:
> This just called out in the CXL upstream sync call as lacking
> review!  Sorry about that - I missed it entirely :(

Hi Jonathan,

Thanks for the review, and no worries at all about the delay.

> 
> So this bit suggests this is hardening against bad device.
> Definitely a good thing to do given it is fairly simple but
> not a fix as such (so no rush to land it!)

Agreed.

> 
> For this one, can we trigger these values by writing a threshold of 0?
> I think that is blocked by cxl_set_ecs_threshold() so I think this
> one is also hardening against a bad device?

Confirmed, the write path cannot produce these encodings.
cxl_set_ecs_threshold() validates the value in a switch that only
accepts 256, 1024 and 4096 and returns -EINVAL for anything else, so
writing 0 (or any other unsupported value) is rejected. Both the
out-of-bounds indices (6-7) and the reserved encodings (0-2) can
therefore only be observed when a device itself reports them, so
this is hardening against a bad device in both cases.

> > Fixes: 85fb6a16ad14 ("cxl/edac: Add CXL memory device ECS control feature")
> 
> My overall suggestion is drop the fixes tag and state that this
> is hardening against devices returning values not currently in the spec.

Will do. I had added the Fixes tag mainly because of the out-of-bounds
read, but as that can only happen with a device reporting values
outside the spec, hardening is indeed the right framing. I'll drop
the Fixes tag and reword the commit message accordingly; the code is
unchanged, so I'll send a v3 with just the commit message updated.

> 
> It might actually evolve to become a real fix with future hardware
> changes using those reserved values but we have no reason to think
> that will happen yet. 
> 
> We had a small discussion on the call about fixes for bad hardware
> and I expressed my view that if they are cheap (like here) in
> maintenance burden by all means take them, but I'll push back on
> anything that needs very complex solutions.

That also fits your maintenance-burden point nicely - this one is a
single validity check on top of the existing lookup, and if a future
spec revision ever assigns meaning to those reserved encodings,
ecs_supp_threshold[] can simply be extended at that point.

> 
> > Suggested-by: Alison Schofield <alison.schofield@intel.com>
> > Signed-off-by: Yili Zhang <zhangyili01@baidu.com>
> 
> Updated Shiju's email address in the +CC.
> 

Thanks also for updating Shiju's address on the CC.

Regards,
Yili

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

[Alison Schofield]

On Wed, Sep 30, 2026 at 05:56:27PM +0800, 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).
> 
> 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>

Hi Yili,

Thanks for the updates from v1.

Just a couple of process things for v3. Drop my Suggested-by tag.
That is usually reserved for folks who suggest the actual patch,
not folks who suggest changes on v1, like I did. And, send v3 as
a new email thread, not in reply to this thread, as you did here.

-- Alison


> ---
> 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
>