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

["Cheatham, Benjamin" <benjamin.cheatham@amd.com>]

On 10/27/2026 8:16 PM, Jonathan Cameron wrote:
> 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.
> 

You know, I thought about it to hard and forgot that this is a preemptive fixup. I think
you're right and that it probably doesn't need to be documented.
>>
>> 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.
> 

Cool, then this looks good to me. For Yili:

Reviewed-by: Ben Cheatham <benjamin.cheatham@amd.com>

And thanks for the response Jonathan!

Thanks,
Ben