Re: [PATCH 03/18] cxl: Add checksum verification to CDAT from CXL

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

On 2/9/23 4:34 AM, Jonathan Cameron wrote:
> On Mon, 06 Feb 2023 13:49:48 -0700
> Dave Jiang <dave.jiang@intel.com> 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>
> Hi Dave,
> 
> Some comments on this follow on from previous patch so may not
> be relevant once you've updated how that is done.

Dan advised to just drop the ACPICA changes and just do it locally since 
the verification code is tiny and simple.

> 
> Jonathan
> 
>> ---
>>   drivers/cxl/core/pci.c |   11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 57764e9cd19d..a24dac36bedd 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -3,6 +3,7 @@
>>   #include <linux/io-64-nonatomic-lo-hi.h>
>>   #include <linux/device.h>
>>   #include <linux/delay.h>
>> +#include <linux/acpi.h>
>>   #include <linux/pci.h>
>>   #include <linux/pci-doe.h>
>>   #include <cxlpci.h>
>> @@ -592,6 +593,7 @@ void read_cdat_data(struct cxl_port *port)
>>   	struct device *dev = &port->dev;
>>   	struct device *uport = port->uport;
>>   	size_t cdat_length;
>> +	acpi_status status;
>>   	int rc;
>>   
>>   	cdat_doe = find_cdat_doe(uport);
>> @@ -620,5 +622,14 @@ void read_cdat_data(struct cxl_port *port)
>>   		port->cdat.length = 0;
>>   		dev_err(dev, "CDAT data read error\n");
>>   	}
>> +
>> +	status = acpi_ut_verify_cdat_checksum(port->cdat.table, port->cdat.length);
>> +	if (status != AE_OK) {
> 
> if (ACPI_FAILURE(acpi_ut...))  or better still put that in the wrapper I suggeste
> in previous patch so that we have normal kernel return code handling out here.
> 
> 
>> +		/* Don't leave table data allocated on error */
>> +		devm_kfree(dev, port->cdat.table);
>> +		port->cdat.table = NULL;
>> +		port->cdat.length = 0;
> 
> I'd rather see us manipulate a local copy of cdat_length, and cdat_table
> then only assign them to the port->cdat fields the success path rather
> than setting them then unsetting the on error.
> 
> Diff will be bigger, but nicer resulting code (and hopefully diff
> won't be too big!)

Ok, I'll create a prep patch to change this as you suggested.

> 
> 
>> +		dev_err(dev, "CDAT data checksum error\n");
>> +	}
>>   }
>>   EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
>>
>>
>