Re: [PATCH v5 5/7] cxl/memdev: Don't overwrite the error from an earlier partition poison query

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Lockless access to `cxlmd->endpoint` in `cxl_trigger_poison_list()` leads to a TOCTOU Use-After-Free if the device is concurrently unbound.
- [High] Missing bounds check on `po->count` in `cxl_mem_get_poison()` allows an out-of-bounds read of kernel memory.
- [Medium] `cxl_mem_get_poison()` fails to update the payload offset when retrieving subsequent poison records, resulting in duplicated traces and dropped records.
--

--- Patch [5]: [PATCH v5 5/7] cxl/memdev: Don't overwrite the error from an earlier partition poison query ---
commit 68fad9eb0dea3e233e2489fa05fad1099307f601
Author: Richard Cheng <icheng@nvidia.com>

cxl/memdev: Don't overwrite the error from an earlier partition poison query

cxl_get_poison_by_memdev() currently doesn't check the loop result, allowing
successful partition scans to overwrite earlier failures. This patch breaks
the loop on any unhandled error to properly report failures.

> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index b3419df586b9c..e39b3d13fd566 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -231,6 +231,8 @@ static int cxl_get_poison_by_memdev(struct cxl_memdev *cxlmd)
>  		 */
>  		if (rc == -EFAULT && cxlds->part[i].mode == CXL_PARTMODE_RAM)
>  			rc = 0;
> +		if (rc)
> +			break;
>  	}
>  	return rc;
>  }

While reviewing this, I noticed a few pre-existing issues in related code:

drivers/cxl/core/memdev.c:cxl_trigger_poison_list() {
    ...
    int cxl_trigger_poison_list(struct cxl_memdev *cxlmd)
    {
        struct cxl_port *port;
        int rc;

        port = cxlmd->endpoint;
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but when a root user writes to the
trigger_poison_list sysfs attribute of a cxl_memdev device, it calls
cxl_trigger_poison_list(), which reads cxlmd->endpoint locklessly.

Concurrently, the parent CXL port can be unbound.

The unbind triggers the devm callback delete_endpoint(), which clears
cxlmd->endpoint to NULL and releases the port's reference, freeing it.

Since the sysfs thread has already loaded the pointer, it proceeds to use the
freed port pointer in calls to cxl_num_decoders_committed(port) and
cxl_get_poison_by_endpoint(port).

Does lockless access to cxlmd->endpoint in cxl_trigger_poison_list() lead to a
TOCTOU Use-After-Free if the device is concurrently unbound?


drivers/cxl/core/mbox.c:cxl_mem_get_poison() {
    ...
        if (!le16_to_cpu(po->count)) {
            dev_dbg(&cxlmd->dev, "Poison empty payload!\n");
            break;
        }

        for (int i = 0; i < le16_to_cpu(po->count); i++)
            trace_cxl_poison(cxlmd, cxlr, &po->record[i],
                     po->flags, po->overflow_ts,
                     CXL_POISON_TRACE_LIST);
    ...
}

[Severity: High]
This is a pre-existing issue, but in cxl_mem_get_poison(), the driver sends a
GET_POISON_LIST command.

While cxl_internal_send_cmd() validates that the returned payload is at least
the size of the response header, it does not validate that it is large enough
to contain le16_to_cpu(po->count) records.

A malicious or buggy CXL device could return a small payload (e.g., 24 bytes)
but a large po->count (e.g., 65535).

The driver iterates po->count times, reading po->record[i] from the
mds->poison.list_out buffer (which is allocated as cxl_mbox->payload_size).
This will read past the end of the allocation.

Is there a missing bounds check on po->count in cxl_mem_get_poison() that
allows an out-of-bounds read of kernel memory?


drivers/cxl/core/mbox.c:cxl_mem_get_poison() {
    ...
    po = mds->poison.list_out;
    pi.offset = cpu_to_le64(offset);
    pi.length = cpu_to_le64(len / CXL_POISON_LEN_MULT);

    do {
        struct cxl_mbox_cmd mbox_cmd = (struct cxl_mbox_cmd){
            .opcode = CXL_MBOX_OP_GET_POISON,
            .size_in = sizeof(pi),
            .payload_in = &pi,
            .size_out = cxl_mbox->payload_size,
            .payload_out = po,
            .min_out = struct_size(po, record, 0),
        };

        rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
    ...
}

[Severity: Medium]
This isn't a bug introduced by this patch, but according to the CXL 3.0
specification, if the device sets the 'More Poison Records' flag, the host
must issue subsequent Get Poison List commands with an updated Offset and
Length.

In cxl_mem_get_poison(), pi.offset and pi.length are populated before the
do-while loop.

Because they are never updated inside the loop, the driver sends the identical
payload in every iteration.

A stateless CXL device will respond with the identical first chunk of poison
records, continuously setting the MORE flag.

The driver will log these duplicate records until it artificially hits
mds->poison.max_errors, dropping the remaining records.

Does cxl_mem_get_poison() fail to update the payload offset when retrieving
subsequent poison records, resulting in duplicated traces and dropped records?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813034538.13189-1-icheng@nvidia.com?part=5