Re: [PATCH v4 02/23] cxl: Add checksum verification to CDAT from CXL

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

On 4/24/23 3:01 PM, Dan Williams wrote:
> Dave Jiang wrote:
>> A CDAT table is available from a CXL device. The table is read by the
>> driver and cached in software. With the CXL subsystem needing to parse the
>> CDAT table, the checksum should be verified. Add checksum verification
>> after the CDAT table is read from device.
>>
>> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>
>> ---
>> v3:
>> - Just return the final sum. (Alison)
>> v2:
>> - Drop ACPI checksum export and just use local verification. (Dan)
>> ---
>>   drivers/cxl/core/pci.c |   16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 25b7e8125d5d..9c7e2f69d9ca 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -528,6 +528,16 @@ static int cxl_cdat_read_table(struct device *dev,
>>   	return 0;
>>   }
>>   
>> +static unsigned char cdat_checksum(void *buf, size_t size)
>> +{
>> +	unsigned char sum, *data = buf;
>> +	size_t i;
>> +
>> +	for (sum = 0, i = 0; i < size; i++)
>> +		sum += data[i];
>> +	return sum;
>> +}
>> +
>>   /**
>>    * read_cdat_data - Read the CDAT data on this port
>>    * @port: Port to read data from
>> @@ -573,6 +583,12 @@ void read_cdat_data(struct cxl_port *port)
>>   	}
>>   
>>   	port->cdat.table = cdat_table + sizeof(__le32);
>> +	if (cdat_checksum(port->cdat.table, cdat_length)) {
>> +		/* Don't leave table data allocated on error */
>> +		devm_kfree(dev, cdat_table);
>> +		dev_err(dev, "CDAT data checksum error\n");
>> +	}
>> +
>>   	port->cdat.length = cdat_length;
> 
> I think read_cdat_data() is confused about error cases. I note that
> /sys/firmware/acpi/tables does not emit the entry if the table has bad
> length or bad checksum. If you want to have a debug mode then maybe make
> it a compile time option, but I otherwise do not see the benefit of
> publishing known bad tables to userspace.

I'll have it return on errors.