Re: [PATCH v9 02/11] cxl: Cache decoder settings on PCI devices

[sashiko-bot@kernel.org]

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

New issues:
- [High] Resource Leak / Devres Lifecycle Bug

Pre-existing issues:
- [Critical] Out-of-bounds Read / Information Leak
- [High] Endianness / Byte Order Logic Error
--

--- Patch [2]: [PATCH v9 02/11] cxl: Cache decoder settings on PCI devices ---
commit b30e35d89911fd38d088f7e8ed1637175c1c6ae9
Author: Srirangan Madhavan <smadhavan@nvidia.com>
cxl: Cache decoder settings on PCI devices

This commit caches CXL core's HDM decoder settings in pci_dev->hdm as decoders
are enumerated, committed, or reset. PCI reset paths can use this snapshot to
restore HDM programming without walking CXL topology during reset recovery.

> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index fa978c297546f..83cda63f76a5a 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c

[ ... ]

> +static int devm_cxl_pci_setup_hdm_info(struct cxl_hdm *cxlhdm)
> +{
> +	struct cxl_port *port = cxlhdm->port;
> +	struct cxl_hdm_info *info;
> +	struct pci_dev *pdev;
> +	struct device *uport;

[ ... ]

> +	if (!dev_is_pci(uport))
> +		return 0;
> +
> +	pdev = to_pci_dev(uport);
> +	info = devm_kzalloc(&pdev->dev,
> +			    struct_size(info, settings, cxlhdm->decoder_count),
> +			    GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	info->decoder_count = cxlhdm->decoder_count;
> +	WRITE_ONCE(pdev->hdm, info);
> +
> +	return devm_add_action_or_reset(&pdev->dev, clear_hdm_info, pdev);
> +}

[Severity: High]
Could this cause a memory leak if the CXL port driver is unbound and rebound
by a user via sysfs?

Because pdev is the long-lived parent PCI device rather than the child
cxl_port device being probed, these devres resources are tied to the
parent's lifecycle. Repeated bind/unbind cycles on the port driver could
cause the devres allocations and cleanup actions to accumulate infinitely
on the parent device.

[ ... ]

> @@ -989,6 +1061,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  		lo = readl(hdm + CXL_HDM_DECODER0_TL_LOW(which));
>  		hi = readl(hdm + CXL_HDM_DECODER0_TL_HIGH(which));
>  		target_list.value = (hi << 32) + lo;

[Severity: High]
This is a pre-existing issue, but does this byte-by-byte access of a
native 64-bit integer break on big-endian architectures?

Since hi and lo are correctly read as native 32-bit CPU-endian integers
and combined into a native 64-bit integer, accessing individual bytes via
target_list.target_id[i] on a big-endian system will place the most
significant byte at index 0. This would reverse the target IDs and result
in incorrect downstream port routing.

> +		cxld->targets = target_list.value;
>  		for (i = 0; i < cxld->interleave_ways; i++)
>  			cxld->target_map[i] = target_list.target_id[i];

[Severity: Critical]
This isn't a bug introduced by this patch, but can this loop read past the
end of the 8-byte union and leak kernel stack memory?

Looking at parse_hdm_decoder_caps(), cxld->interleave_ways can be set
to 16. The target_list union is only 8 bytes long (containing an 8-byte
u64 value and an 8-byte unsigned char target_id[8]). If a device
reports a 16-way interleave, this loop will read uninitialized stack memory
from indices 8 through 15, which could then be exposed to userspace via
the sysfs target_list attribute.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709010304.680422-1-smadhavan@nvidia.com?part=2