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@xxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Yili Zhang <zhangyili01@xxxxxxxxx>

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