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

[Dave Jiang <dave.jiang@intel.com>]

On 3/5/24 3:34 AM, Jonathan Cameron wrote:
> 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.

Right. And even if there is, the endien conversion makes it complicated. I suppose it'll maybe take leXX_to_cpu() wrapper around the count?

> 
> 
>> ---
>> 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;
>