Re: [PATCH v12 6/8] cxl/mem: Configure dynamic capacity interrupts

[sashiko-bot@kernel.org]

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

New issues:
- [High] Infinite IRQ Thread Loop / CPU Spin when handling DCD events

Pre-existing issues:
- [High] Out-of-Bounds Read and Information Leak in Event logs
- [High] Out-of-Bounds Read and Information Leak in Poison logs
--

--- Patch [6]: [PATCH v12 6/8] cxl/mem: Configure dynamic capacity interrupts ---
commit 5b173c122c3602992c4ceeafd1c2d87a80accacb
Author: Ira Weiny <iweiny@kernel.org>

cxl/mem: Configure dynamic capacity interrupts

Dynamic Capacity Devices (DCD) support extent change notifications
through the event log mechanism. Configure DCD event log interrupts
on devices supporting dynamic capacity. Disable DCD if interrupts
are not supported.

> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index df13fb8802c38..6cb344ec4f3a7 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -509,11 +509,30 @@ static bool cxl_alloc_irq_vectors(struct pci_dev *pdev)
>  	return true;
>  }
>  
> +/*
> + * Event logs the driver drains: standard logs when native_cxl, DCD when
> + * supported.
> + */
> +static u32 cxl_event_drain_mask(struct pci_host_bridge *host_bridge,
> +				struct cxl_memdev_state *mds)
> +{
> +	u32 mask = 0;
> +
> +	if (host_bridge->native_cxl_error)
> +		mask |= CXLDEV_EVENT_STATUS_ALL & ~CXLDEV_EVENT_STATUS_DCD;
> +	if (cxl_dcd_supported(mds))
> +		mask |= CXLDEV_EVENT_STATUS_DCD;
> +	return mask;
> +}
> +
>  static irqreturn_t cxl_event_thread(int irq, void *id)
>  {
>  	struct cxl_dev_id *dev_id = id;
>  	struct cxl_dev_state *cxlds = dev_id->cxlds;
>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> +	struct pci_host_bridge *host_bridge =
> +		pci_find_host_bridge(to_pci_dev(cxlds->dev)->bus);
> +	u32 mask = cxl_event_drain_mask(host_bridge, mds);
>  	u32 status;
>  
>  	do {
> @@ -522,8 +541,8 @@ static irqreturn_t cxl_event_thread(int irq, void *id)
>  		 * ignore the reserved upper 32 bits
>  		 */
>  		status = readl(cxlds->regs.status + CXLDEV_DEV_EVENT_STATUS_OFFSET);
> -		/* Ignore logs unknown to the driver */
> -		status &= CXLDEV_EVENT_STATUS_ALL;
> +		/* Ignore logs unknown to the driver or owned by BIOS */
> +		status &= mask;
>  		if (!status)
>  			break;
>  		cxl_mem_get_event_records(mds, status);

[Severity: High]
Since mask now includes CXLDEV_EVENT_STATUS_DCD, status can have this bit set.
However, cxl_mem_get_event_records() does not appear to handle the DCD event:

drivers/cxl/core/mbox.c:cxl_mem_get_event_records() {
    ...
    if (status & CXLDEV_EVENT_STATUS_FATAL)
        cxl_mem_get_records_log(mds, CXL_EVENT_TYPE_FATAL);
    if (status & CXLDEV_EVENT_STATUS_FAIL)
        cxl_mem_get_records_log(mds, CXL_EVENT_TYPE_FAIL);
    if (status & CXLDEV_EVENT_STATUS_WARN)
        cxl_mem_get_records_log(mds, CXL_EVENT_TYPE_WARN);
    if (status & CXLDEV_EVENT_STATUS_INFO)
        cxl_mem_get_records_log(mds, CXL_EVENT_TYPE_INFO);
    ...
}

Will this fail to clear the DCD event from the device? If the event is never
cleared, will the hardware register retain the bit and cause the while (status)
loop to spin infinitely on the exact same status bit, potentially causing a CPU
soft lockup?

>  		cond_resched();
>  	} while (status);

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential out-of-bounds read when
fetching event logs:

drivers/cxl/core/mbox.c:cxl_mem_get_records_log() {
    ...
    nr_rec = le16_to_cpu(payload->record_count);
    if (!nr_rec)
        break;

    for (i = 0; i < nr_rec; i++)
        __cxl_event_trace_record(cxlmd, type,
                                 &payload->records[i]);
    ...
}

Does this code validate the hardware-provided nr_rec against the allocated
payload size? If an untrusted or compromised device returns an artificially
large count, could this read out of bounds?

Similarly, when clearing those records:

drivers/cxl/core/mbox.c:cxl_clear_event_record() {
    ...
    for (cnt = 0; cnt < total; cnt++) {
        struct cxl_event_record_raw *raw = &get_pl->records[cnt];
        struct cxl_event_generic *gen = &raw->event.generic;

        payload->handles[i++] = gen->hdr.handle;
    ...
}

Could this write the out-of-bounds memory values into the clear event payload
sent back to the device, leading to an information leak?

[Severity: High]
This is also a pre-existing issue, but a similar pattern exists in the poison
log handling:

drivers/cxl/core/mbox.c:cxl_mem_get_poison() {
    ...
    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);
    ...
}

Does this validate po->count against the buffer size, or could it result
in an out-of-bounds read if the device returns a large count?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731084901.1512819-1-anisa.su@samsung.com?part=6