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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 23 Oct 2026 16:05:27 -0500
"Cheatham, Benjamin" <benjamin.cheatham@amd.com> wrote:

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

It would be unusual to document what happens in a hardware bug situation
in an ABI doc.  Tends to be noisy and hopefully never applies.
I don't mind it being added if people really think it adds value.

> 
> So I'm going to take it a step further and ask if we should be failing the write at all?

This is only the read path I think? I guess documenting that
writes can only be of allowed values and otherwise you get an
error return would be fine but to me that would normally be
assumed for a sysfs parameter.  It is nicer for userspace than
alternative of just ignoring such a write.


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

Far as I know, today this is the only ECS control driver.
Maybe someone surfaced another whilst I wasn't paying attention though
or someone has one in the works.

Jonathan



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