diff mbox series

[2/3] cxl: Add checksum verification to CDAT from CXL

Message ID 168330452895.1986478.7758561874383258080.stgit@djiang5-mobl3
State Superseded
Headers show
Series cxl: Prep for QoS class support | expand

Commit Message

Dave Jiang May 5, 2023, 4:35 p.m. UTC 
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>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>

---
v5:
- Return on CDAT errors. (Dan)
v3:
- Just return the final sum. (Alison)
v2:
- Drop ACPI checksum export and just use local verification. (Dan)
---
 drivers/cxl/core/pci.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Davidlohr Bueso May 5, 2023, 7:15 p.m. UTC | #1 
On Fri, 05 May 2023, Dave Jiang wrote:

> /**
>  * read_cdat_data - Read the CDAT data on this port
>  * @port: Port to read data from
>@@ -571,9 +581,17 @@ void read_cdat_data(struct cxl_port *port)
>		/* Don't leave table data allocated on error */
>		devm_kfree(dev, cdat_table);
>		dev_err(dev, "CDAT data read error\n");
>+		return;

Should this be a separate fixlet?

>	}
>
>	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");
>+		return;
>+	}
>+
>	port->cdat.length = cdat_length;

Upon error, port->cdat should remain consistent (only be set after passing the
checksum. Ie, wouldn't the following be better?

	  cdat_table += sizeof(__le32);
	  if (cdat_checksum()) {
	     ...
	  }
	  port->cdat.table = cdat_table;
	  port->cdat.length = cdat_length;


Thanks,
Davidlohr
Dave Jiang May 5, 2023, 8:43 p.m. UTC | #2 
On 5/5/23 12:15 PM, Davidlohr Bueso wrote:
> On Fri, 05 May 2023, Dave Jiang wrote:
> 
>> /**
>>  * read_cdat_data - Read the CDAT data on this port
>>  * @port: Port to read data from
>> @@ -571,9 +581,17 @@ void read_cdat_data(struct cxl_port *port)
>>         /* Don't leave table data allocated on error */
>>         devm_kfree(dev, cdat_table);
>>         dev_err(dev, "CDAT data read error\n");
>> +        return;
> 
> Should this be a separate fixlet?

Yeah I can split that out.
> 
>>     }
>>
>>     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");
>> +        return;
>> +    }
>> +
>>     port->cdat.length = cdat_length;
> 
> Upon error, port->cdat should remain consistent (only be set after 
> passing the
> checksum. Ie, wouldn't the following be better?

yup. I'll update.

> 
>        cdat_table += sizeof(__le32);
>        if (cdat_checksum()) {
>           ...
>        }
>        port->cdat.table = cdat_table;
>        port->cdat.length = cdat_length;
> 
> 
> Thanks,
> Davidlohr
diff mbox series

Patch

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index bdbd907884ce..046e55a9b419 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
@@ -571,9 +581,17 @@  void read_cdat_data(struct cxl_port *port)
 		/* Don't leave table data allocated on error */
 		devm_kfree(dev, cdat_table);
 		dev_err(dev, "CDAT data read error\n");
+		return;
 	}
 
 	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");
+		return;
+	}
+
 	port->cdat.length = cdat_length;
 }
 EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);