Re: [PATCH v3] cxl: Fix the incorrect assignment of SSLBIS entry pointer initial location

From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Dave Jiang <dave.jiang@intel.com>
Cc: <linux-cxl@vger.kernel.org>, <dan.j.williams@intel.com>,
	<ira.weiny@intel.com>, <vishal.l.verma@intel.com>,
	<alison.schofield@intel.com>, <dave@stgolabs.net>
Subject: Re: [PATCH v3] cxl: Fix the incorrect assignment of SSLBIS entry pointer initial location
Date: Tue, 5 Mar 2024 10:34:55 +0000	[thread overview]
Message-ID: <20240305103455.000064bc@Huawei.com> (raw)
In-Reply-To: <20240301210948.1298075-1-dave.jiang@intel.com>

On Fri, 1 Mar 2024 14:09:48 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> The 'entry' pointer in cdat_sslbis_handler() is set to header +
> sizeof(common header). However, the math missed the addition of the SSLBIS
> main header. It should be header + sizeof(common header) + sizeof(*sslbis).
> Use a defined struct for all the SSLBIS parts in order to avoid pointer
> math errors.
> 
> The bug causes incorrect parsing of the SSLBIS table and introduces incorrect
> performance values to the access_coordinates during the CXL access_coordinate
> calculation path if there are CXL switches present in the topology.
> 
> The issue was found during testing of new code being added to add additional
> checks for invalid CDAT values during CXL access_coordinate calculation. The
> testing was done on qemu with a CXL topology including a CXL switch.
> 
> Fixes: 80aa780dda20 ("cxl: Add callback to parse the SSLBIS subtable from CDAT")
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Hmm. This is far from a minimal fix.  The end result is nicer though so fair enough.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Pity there is no direct variable containing the count so we could do
a __counted_by() - don't think you can do maths in that unfortunately.

> ---
> v3:
> - use var for sizeof() instead of struct (Alison)
> ---
>  drivers/cxl/core/cdat.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index 08fd0baea7a0..0363ca434ef4 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -389,36 +389,38 @@ EXPORT_SYMBOL_NS_GPL(cxl_endpoint_parse_cdat, CXL);
>  static int cdat_sslbis_handler(union acpi_subtable_headers *header, void *arg,
>  			       const unsigned long end)
>  {
> +	struct acpi_cdat_sslbis_table {
> +		struct acpi_cdat_header header;
> +		struct acpi_cdat_sslbis sslbis_header;
> +		struct acpi_cdat_sslbe entries[];
> +	} *tbl = (struct acpi_cdat_sslbis_table *)header;
> +	int size = sizeof(header->cdat) + sizeof(tbl->sslbis_header);
>  	struct acpi_cdat_sslbis *sslbis;
> -	int size = sizeof(header->cdat) + sizeof(*sslbis);
>  	struct cxl_port *port = arg;
>  	struct device *dev = &port->dev;
> -	struct acpi_cdat_sslbe *entry;
>  	int remain, entries, i;
>  	u16 len;
>  
>  	len = le16_to_cpu((__force __le16)header->cdat.length);
>  	remain = len - size;
> -	if (!remain || remain % sizeof(*entry) ||
> +	if (!remain || remain % sizeof(tbl->entries[0]) ||
>  	    (unsigned long)header + len > end) {
>  		dev_warn(dev, "Malformed SSLBIS table length: (%u)\n", len);
>  		return -EINVAL;
>  	}
>  
> -	/* Skip common header */
> -	sslbis = (struct acpi_cdat_sslbis *)((unsigned long)header +
> -					     sizeof(header->cdat));
> -
> +	sslbis = &tbl->sslbis_header;
>  	/* Unrecognized data type, we can skip */
>  	if (sslbis->data_type > ACPI_HMAT_WRITE_BANDWIDTH)
>  		return 0;
>  
> -	entries = remain / sizeof(*entry);
> -	entry = (struct acpi_cdat_sslbe *)((unsigned long)header + sizeof(*sslbis));
> +	entries = remain / sizeof(tbl->entries[0]);
> +	if (struct_size(tbl, entries, entries) != len)
> +		return -EINVAL;
>  
>  	for (i = 0; i < entries; i++) {
> -		u16 x = le16_to_cpu((__force __le16)entry->portx_id);
> -		u16 y = le16_to_cpu((__force __le16)entry->porty_id);
> +		u16 x = le16_to_cpu((__force __le16)tbl->entries[i].portx_id);
> +		u16 y = le16_to_cpu((__force __le16)tbl->entries[i].porty_id);
>  		__le64 le_base;
>  		__le16 le_val;
>  		struct cxl_dport *dport;
> @@ -448,8 +450,8 @@ static int cdat_sslbis_handler(union acpi_subtable_headers *header, void *arg,
>  			break;
>  		}
>  
> -		le_base = (__force __le64)sslbis->entry_base_unit;
> -		le_val = (__force __le16)entry->latency_or_bandwidth;
> +		le_base = (__force __le64)tbl->sslbis_header.entry_base_unit;
> +		le_val = (__force __le16)tbl->entries[i].latency_or_bandwidth;
>  
>  		if (check_mul_overflow(le64_to_cpu(le_base),
>  				       le16_to_cpu(le_val), &val))
> @@ -462,8 +464,6 @@ static int cdat_sslbis_handler(union acpi_subtable_headers *header, void *arg,
>  							  sslbis->data_type,
>  							  val);
>  		}
> -
> -		entry++;
>  	}
>  
>  	return 0;