Re: [PATCH v2] cxl/hdm: Fix out of bounds read of the decoder target list

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

Pre-existing issues:
- [High] Endianness bug when decoding the HDM decoder Target List registers on big-endian architectures.
- [High] Permanent port state corruption on decoder initialization failure due to un-reverted `port->commit_end`.
--

--- Patch [1]: [PATCH v2] cxl/hdm: Fix out of bounds read of the decoder target list ---
commit c692efbe782398330e6841bfdd1146cdee44ef3b
Author: Guixin Liu <[email protected]>
Subject: cxl/hdm: Fix out of bounds read of the decoder target list

This commit fixes an out-of-bounds read in the HDM decoder initialization. It
adds a check to ensure the interleave ways do not exceed the size of the
target list array when programming the target map.

Link: https://lore.kernel.org/linux-cxl/[email protected]/

> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 0c80b76a5f9b4..9d49b48a4456e 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -1084,6 +1084,18 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  		cxld->interleave_ways, cxld->interleave_granularity);
>  
>  	if (!cxled) {
> +		/*
> +		 * The Target List register only holds
> +		 * ARRAY_SIZE(target_list.target_id) entries, so a switch
> +		 * decoder cannot interleave across more ports than that.
> +		 */
> +		if (cxld->interleave_ways > ARRAY_SIZE(target_list.target_id)) {
> +			dev_warn(&port->dev,
> +				 "decoder%d.%d: Interleave ways: %d exceeds target list size\n",
> +				 port->id, cxld->id, cxld->interleave_ways);
> +			return -ENXIO;

[Severity: High]
This is a pre-existing issue, but does returning here leave port->commit_end
permanently advanced?

Earlier in init_hdm_decoder(), port->commit_end is updated:

	if (committed) {
		...
		port->commit_end = cxld->id;
	}

If we fail here and return -ENXIO, port->commit_end is not rolled back.  If
initialization is retried, cxl_num_decoders_committed() will return an
inflated count, which could cause the out of order commit check to fail.
Could this permanently prevent the port from initializing?

> +		}
> +
>  		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 code read the target IDs 
incorrectly on big-endian architectures?

target_list.value is populated as a native 64-bit integer.  The subsequent loop
reads it byte-by-byte via the target_id union:

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

On big-endian hardware, accessing the native 64-bit integer byte-by-byte will
read the most significant bytes first, which could scramble the port target
routing.

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