Re: [PATCH v8 1/3] cxl/region: Simplify poison_by_decoder() error handling

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Array out-of-bounds read when `cxled->part` is -1.
- [High] Silent error masking in `cxl_get_poison_by_memdev` overwrites earlier failures.
--

--- Patch [1]: [PATCH v8 1/3] cxl/region: Simplify poison_by_decoder() error handling ---
commit 091367d84a5db5147f3a65c244b70983d67217ac
Author: Richard Cheng <[email protected]>

cxl/region: Simplify poison_by_decoder() error handling

"rc" carries both an error code and the loop control signal for
device_for_each_child(), so returning it bare is misleading, the early
guards mean "keep walking", not "no error". Zeroing "rc" to forgive an
-EFAULT on a RAM partition adds to that by discarding what the device
actually returned.

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 1e211542b6b64..fabaad3469b1b 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2950,14 +2950,14 @@ static int poison_by_decoder(struct device *dev, void *arg)
>  	struct cxl_dev_state *cxlds;
>  	struct cxl_memdev *cxlmd;
>  	u64 offset, length;
> -	int rc = 0;
> +	int rc;
>  
>  	if (!is_endpoint_decoder(dev))
> -		return rc;
> +		return 0;
>  
>  	cxled = to_cxl_endpoint_decoder(dev);
>  	if (!cxled->dpa_res)
> -		return rc;
> +		return 0;
>  
>  	cxlmd = cxled_to_memdev(cxled);
>  	cxlds = cxlmd->cxlds;

[Severity: High]
This isn't a bug introduced by this patch, but does the array access in
poison_by_decoder() risk an out-of-bounds read if cxled->part is -1?

If firmware provides misconfigured ACPI tables or CXL DPA resources,
cxl_dpa_alloc() might leave cxled->part as -1. A subsequent userspace read
of the trigger_poison_list sysfs attribute calls
cxl_get_poison_by_endpoint() which iterates over endpoint decoders and
evaluates:

drivers/cxl/core/region.c:poison_by_decoder() {
    ...
    cxlmd = cxled_to_memdev(cxled);
    cxlds = cxlmd->cxlds;
    mode = cxlds->part[cxled->part].mode;
    ...
}

Would this result in an invalid memory access of cxlds->part[-1]?


[Severity: High]
This is also a pre-existing issue, but does cxl_get_poison_by_memdev()
silently overwrite errors from earlier partitions?

Looking at cxl_get_poison_by_memdev() in drivers/cxl/core/memdev.c:

drivers/cxl/core/memdev.c:cxl_get_poison_by_memdev() {
    ...
    for (int i = 0; i < cxlds->nr_partitions; i++) {
        ...
        rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
        if (rc == -EFAULT && cxlds->part[i].mode == CXL_PARTMODE_RAM)
            rc = 0;
    }
    return rc;
}

If a device fails to retrieve poison for the first partition (e.g. PMEM,
returning -ENXIO) but succeeds on a subsequent partition (e.g. RAM, returning
0), rc is overwritten to 0.

Does this permanently mask failures of earlier partitions, potentially leaving
poisoned memory undetected?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1