Re: [PATCH v2 02/21] cxl: Add checksum verification to CDAT from CXL

From: Dave Jiang <dave.jiang@intel.com>
To: Alison Schofield <alison.schofield@intel.com>
Cc: linux-cxl@vger.kernel.org, linux-acpi@vger.kernel.org,
	dan.j.williams@intel.com, ira.weiny@intel.com,
	vishal.l.verma@intel.com, rafael@kernel.org, lukas@wunner.de
Subject: Re: [PATCH v2 02/21] cxl: Add checksum verification to CDAT from CXL
Date: Tue, 28 Mar 2023 17:21:09 -0700	[thread overview]
Message-ID: <b28baa29-ed88-dcb2-68c6-df6cc8954795@intel.com> (raw)
In-Reply-To: <ZCOANlmHyhJ0hDGn@aschofie-mobl2>

On 3/28/23 5:03 PM, Alison Schofield wrote:
> On Mon, Mar 27, 2023 at 02:44:13PM -0700, 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.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>
>> ---
>> 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..e0d5e6525c0d 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 0 - sum;
> 
> This return value isn't obvious to me. What's happening here?
> Thanks for explaining,

The expectation is that all the bytes add up to be equal 0 for the 
checksum verification. So if we get anything other than 0, the check 
would fail.

DJ

> 
> Alison
> 
>> +}
>> +
>>   /**
>>    * 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;
>>   }
>>   EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
>>
>>