Some CDAT related updates and fixes. Patch #3 does not depend on the
previous patches and could be applied separately.
See [1] for v1. There is some rework going on [2] which might require
a v3 for patches #1 and #2.
References:
[1] https://patchwork.kernel.org/project/cxl/patch/[email protected]/
[2] https://patchwork.kernel.org/project/cxl/patch/170261791914.1714654.6447680285357545638.stgit@dwillia2-xfh.jf.intel.com/
Changelog:
v2:
* rebased onto cxl/next (e16bf7e015d7)
* renamed struct cdat_doe to struct cdat_doe_rsp and also local pointers
accordingly to buf/rsp.
* added comment that the CDAT table has space for a DOE header at the
beginning
* use DECLARE_FLEX_ARRAY() macro in struct cdat_doe_rsp
* moved the rename to doe_mb variable into separate patch
* added Reviewed-by tag
* added patch: lib/firmware_table: Provide buffer length argument to cdat_table_parse()
Robert Richter (3):
cxl/pci: Rename DOE mailbox handle to doe_mb
cxl/pci: Get rid of pointer arithmetic reading CDAT table
lib/firmware_table: Provide buffer length argument to
cdat_table_parse()
drivers/acpi/tables.c | 2 +-
drivers/cxl/core/cdat.c | 6 +--
drivers/cxl/core/pci.c | 83 ++++++++++++++++++++--------------------
drivers/cxl/cxlpci.h | 19 +++++++++
include/linux/fw_table.h | 4 +-
lib/fw_table.c | 15 +++++---
6 files changed, 78 insertions(+), 51 deletions(-)
base-commit: e16bf7e015d75fdd805528bedaf285fcb71dad2a
--
2.39.2
Trivial variable rename for the DOE mailbox handle from cdat_doe to
doe_mb. The variable name cdat_doe is too ambiguous, use doe_mb that
is commonly used for the mailbox.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/cxl/core/pci.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 6c9c8d92f8f7..89bab00bb291 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -518,14 +518,14 @@ EXPORT_SYMBOL_NS_GPL(cxl_hdm_decode_init, CXL);
FIELD_PREP(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE, (entry_handle)))
static int cxl_cdat_get_length(struct device *dev,
- struct pci_doe_mb *cdat_doe,
+ struct pci_doe_mb *doe_mb,
size_t *length)
{
__le32 request = CDAT_DOE_REQ(0);
__le32 response[2];
int rc;
- rc = pci_doe(cdat_doe, PCI_DVSEC_VENDOR_ID_CXL,
+ rc = pci_doe(doe_mb, PCI_DVSEC_VENDOR_ID_CXL,
CXL_DOE_PROTOCOL_TABLE_ACCESS,
&request, sizeof(request),
&response, sizeof(response));
@@ -543,7 +543,7 @@ static int cxl_cdat_get_length(struct device *dev,
}
static int cxl_cdat_read_table(struct device *dev,
- struct pci_doe_mb *cdat_doe,
+ struct pci_doe_mb *doe_mb,
void *cdat_table, size_t *cdat_length)
{
size_t length = *cdat_length + sizeof(__le32);
@@ -557,7 +557,7 @@ static int cxl_cdat_read_table(struct device *dev,
size_t entry_dw;
int rc;
- rc = pci_doe(cdat_doe, PCI_DVSEC_VENDOR_ID_CXL,
+ rc = pci_doe(doe_mb, PCI_DVSEC_VENDOR_ID_CXL,
CXL_DOE_PROTOCOL_TABLE_ACCESS,
&request, sizeof(request),
data, length);
@@ -617,7 +617,7 @@ void read_cdat_data(struct cxl_port *port)
{
struct device *uport = port->uport_dev;
struct device *dev = &port->dev;
- struct pci_doe_mb *cdat_doe;
+ struct pci_doe_mb *doe_mb;
struct pci_dev *pdev = NULL;
struct cxl_memdev *cxlmd;
size_t cdat_length;
@@ -638,16 +638,16 @@ void read_cdat_data(struct cxl_port *port)
if (!pdev)
return;
- cdat_doe = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL,
- CXL_DOE_PROTOCOL_TABLE_ACCESS);
- if (!cdat_doe) {
+ doe_mb = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL,
+ CXL_DOE_PROTOCOL_TABLE_ACCESS);
+ if (!doe_mb) {
dev_dbg(dev, "No CDAT mailbox\n");
return;
}
port->cdat_available = true;
- if (cxl_cdat_get_length(dev, cdat_doe, &cdat_length)) {
+ if (cxl_cdat_get_length(dev, doe_mb, &cdat_length)) {
dev_dbg(dev, "No CDAT length\n");
return;
}
@@ -656,7 +656,7 @@ void read_cdat_data(struct cxl_port *port)
if (!cdat_buf)
return;
- rc = cxl_cdat_read_table(dev, cdat_doe, cdat_buf, &cdat_length);
+ rc = cxl_cdat_read_table(dev, doe_mb, cdat_buf, &cdat_length);
if (rc)
goto err;
--
2.39.2
Reading the CDAT table using DOE requires a Table Access Response
Header in addition to the CDAT entry. In current implementation this
has caused offsets with sizeof(__le32) to the actual buffers. This led
to hardly readable code and even bugs (see fix of devm_kfree() in
read_cdat_data()).
Rework code to avoid calculations with sizeof(__le32). Introduce
struct cdat_doe for this which contains the Table Access Response
Header and a variable payload size for various data structures
afterwards to access the CDAT table and its CDAT Data Structures
without recalculating buffer offsets.
Cc: Lukas Wunner <[email protected]>
Cc: Fan Ni <[email protected]>
Reviewed-by: Dave Jiang <[email protected]>
Signed-off-by: Robert Richter <[email protected]>
---
drivers/cxl/core/pci.c | 67 +++++++++++++++++++++---------------------
drivers/cxl/cxlpci.h | 19 ++++++++++++
2 files changed, 53 insertions(+), 33 deletions(-)
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 89bab00bb291..75d8fa228879 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -544,55 +544,53 @@ static int cxl_cdat_get_length(struct device *dev,
static int cxl_cdat_read_table(struct device *dev,
struct pci_doe_mb *doe_mb,
- void *cdat_table, size_t *cdat_length)
+ struct cdat_doe_rsp *rsp, size_t *length)
{
- size_t length = *cdat_length + sizeof(__le32);
- __le32 *data = cdat_table;
+ size_t received, remaining = *length;
int entry_handle = 0;
__le32 saved_dw = 0;
do {
__le32 request = CDAT_DOE_REQ(entry_handle);
- struct cdat_entry_header *entry;
- size_t entry_dw;
int rc;
rc = pci_doe(doe_mb, PCI_DVSEC_VENDOR_ID_CXL,
CXL_DOE_PROTOCOL_TABLE_ACCESS,
&request, sizeof(request),
- data, length);
+ rsp, sizeof(*rsp) + remaining);
if (rc < 0) {
dev_err(dev, "DOE failed: %d", rc);
return rc;
}
- /* 1 DW Table Access Response Header + CDAT entry */
- entry = (struct cdat_entry_header *)(data + 1);
+ if (rc < sizeof(*rsp))
+ return -EIO;
+
+ received = rc - sizeof(*rsp);
+
if ((entry_handle == 0 &&
- rc != sizeof(__le32) + sizeof(struct cdat_header)) ||
+ received != sizeof(rsp->header[0])) ||
(entry_handle > 0 &&
- (rc < sizeof(__le32) + sizeof(*entry) ||
- rc != sizeof(__le32) + le16_to_cpu(entry->length))))
+ (received < sizeof(rsp->entry[0]) ||
+ received != le16_to_cpu(rsp->entry->length))))
return -EIO;
/* Get the CXL table access header entry handle */
entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE,
- le32_to_cpu(data[0]));
- entry_dw = rc / sizeof(__le32);
- /* Skip Header */
- entry_dw -= 1;
+ le32_to_cpu(rsp->doe_header));
+
/*
* Table Access Response Header overwrote the last DW of
* previous entry, so restore that DW
*/
- *data = saved_dw;
- length -= entry_dw * sizeof(__le32);
- data += entry_dw;
- saved_dw = *data;
+ rsp->doe_header = saved_dw;
+ remaining -= received;
+ rsp = (void *)rsp + received;
+ saved_dw = rsp->doe_header;
} while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY);
/* Length in CDAT header may exceed concatenation of CDAT entries */
- *cdat_length -= length - sizeof(__le32);
+ *length -= remaining;
return 0;
}
@@ -620,8 +618,8 @@ void read_cdat_data(struct cxl_port *port)
struct pci_doe_mb *doe_mb;
struct pci_dev *pdev = NULL;
struct cxl_memdev *cxlmd;
- size_t cdat_length;
- void *cdat_table, *cdat_buf;
+ struct cdat_doe_rsp *buf;
+ size_t length;
int rc;
if (is_cxl_memdev(uport)) {
@@ -647,30 +645,33 @@ void read_cdat_data(struct cxl_port *port)
port->cdat_available = true;
- if (cxl_cdat_get_length(dev, doe_mb, &cdat_length)) {
+ if (cxl_cdat_get_length(dev, doe_mb, &length)) {
dev_dbg(dev, "No CDAT length\n");
return;
}
- cdat_buf = devm_kzalloc(dev, cdat_length + sizeof(__le32), GFP_KERNEL);
- if (!cdat_buf)
- return;
+ /*
+ * The begin of the CDAT buffer needs space for additional 4
+ * bytes for the DOE header. Table data starts afterwards.
+ */
+ buf = devm_kzalloc(dev, sizeof(*buf) + length, GFP_KERNEL);
+ if (!buf)
+ goto err;
- rc = cxl_cdat_read_table(dev, doe_mb, cdat_buf, &cdat_length);
+ rc = cxl_cdat_read_table(dev, doe_mb, buf, &length);
if (rc)
goto err;
- cdat_table = cdat_buf + sizeof(__le32);
- if (cdat_checksum(cdat_table, cdat_length))
+ if (cdat_checksum(buf->table, length))
goto err;
- port->cdat.table = cdat_table;
- port->cdat.length = cdat_length;
- return;
+ port->cdat.table = buf->table;
+ port->cdat.length = length;
+ return;
err:
/* Don't leave table data allocated on error */
- devm_kfree(dev, cdat_buf);
+ devm_kfree(dev, buf);
dev_err(dev, "Failed to read/validate CDAT.\n");
}
EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
index 711b05d9a370..b40c571b2ab2 100644
--- a/drivers/cxl/cxlpci.h
+++ b/drivers/cxl/cxlpci.h
@@ -85,6 +85,25 @@ struct cdat_entry_header {
__le16 length;
} __packed;
+/*
+ * Response contains the CDAT only response header of the DOE. The
+ * response payload is a CDAT structure (either CDAT header or entry),
+ * it may also mark the beginning of the CDAT table.
+ *
+ * Spec refs:
+ *
+ * CXL 3.1 Table 8-14: Read Entry Response
+ * CDAT Specification 1.03: 2 CDAT Data Structures
+ */
+struct cdat_doe_rsp {
+ __le32 doe_header;
+ union {
+ DECLARE_FLEX_ARRAY(u8, table);
+ DECLARE_FLEX_ARRAY(struct cdat_header, header);
+ DECLARE_FLEX_ARRAY(struct cdat_entry_header, entry);
+ };
+} __packed;
+
/*
* CXL v3.0 6.2.3 Table 6-4
* The table indicates that if PCIe Flit Mode is set, then CXL is in 256B flits
--
2.39.2
The last entry in the CDAT table may not mark the end of the CDAT
table buffer specified by the length field in the CDAT header. It can
be shorter with trailing unused (zero'ed) data. The actual table
length is determined when reading all CDAT entries of the table with
DOE.
If the table is greater than expected (containing zero'ed trailing
data), the CDAT parser fails with:
[ 48.691717] Malformed DSMAS table length: (24:0)
[ 48.702084] [CDAT:0x00] Invalid zero length
[ 48.711460] cxl_port endpoint1: Failed to parse CDAT: -22
In addition, the table buffer size can be different from the size
specified in the length field. This may cause out-of-bound access then
parsing the CDAT table.
Fix that by providing an optonal buffer length argument to
acpi_parse_entries_array() that can be used by cdat_table_parse() to
propagate the buffer size down to its users.
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Len Brown <[email protected]>
Signed-off-by: Robert Richter <[email protected]>
---
drivers/acpi/tables.c | 2 +-
drivers/cxl/core/cdat.c | 6 +++---
include/linux/fw_table.h | 4 +++-
lib/fw_table.c | 15 ++++++++++-----
4 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index b07f7d091d13..b976e5fc3fbc 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -253,7 +253,7 @@ int __init_or_acpilib acpi_table_parse_entries_array(
count = acpi_parse_entries_array(id, table_size,
(union fw_table_header *)table_header,
- proc, proc_num, max_entries);
+ 0, proc, proc_num, max_entries);
acpi_put_table(table_header);
return count;
diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index 6fe11546889f..012d8f2a7945 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -149,13 +149,13 @@ static int cxl_cdat_endpoint_process(struct cxl_port *port,
int rc;
rc = cdat_table_parse(ACPI_CDAT_TYPE_DSMAS, cdat_dsmas_handler,
- dsmas_xa, port->cdat.table);
+ dsmas_xa, port->cdat.table, port->cdat.length);
rc = cdat_table_parse_output(rc);
if (rc)
return rc;
rc = cdat_table_parse(ACPI_CDAT_TYPE_DSLBIS, cdat_dslbis_handler,
- dsmas_xa, port->cdat.table);
+ dsmas_xa, port->cdat.table, port->cdat.length);
return cdat_table_parse_output(rc);
}
@@ -511,7 +511,7 @@ void cxl_switch_parse_cdat(struct cxl_port *port)
return;
rc = cdat_table_parse(ACPI_CDAT_TYPE_SSLBIS, cdat_sslbis_handler,
- port, port->cdat.table);
+ port, port->cdat.table, port->cdat.length);
rc = cdat_table_parse_output(rc);
if (rc)
dev_dbg(&port->dev, "Failed to parse SSLBIS: %d\n", rc);
diff --git a/include/linux/fw_table.h b/include/linux/fw_table.h
index 95421860397a..3ff4c277296f 100644
--- a/include/linux/fw_table.h
+++ b/include/linux/fw_table.h
@@ -40,12 +40,14 @@ union acpi_subtable_headers {
int acpi_parse_entries_array(char *id, unsigned long table_size,
union fw_table_header *table_header,
+ unsigned long max_length,
struct acpi_subtable_proc *proc,
int proc_num, unsigned int max_entries);
int cdat_table_parse(enum acpi_cdat_type type,
acpi_tbl_entry_handler_arg handler_arg, void *arg,
- struct acpi_table_cdat *table_header);
+ struct acpi_table_cdat *table_header,
+ unsigned long length);
/* CXL is the only non-ACPI consumer of the FIRMWARE_TABLE library */
#if IS_ENABLED(CONFIG_ACPI) && !IS_ENABLED(CONFIG_CXL_BUS)
diff --git a/lib/fw_table.c b/lib/fw_table.c
index 1e5e0b2f7012..ddb67853b7ac 100644
--- a/lib/fw_table.c
+++ b/lib/fw_table.c
@@ -132,6 +132,7 @@ static __init_or_fwtbl_lib int call_handler(struct acpi_subtable_proc *proc,
*
* @id: table id (for debugging purposes)
* @table_size: size of the root table
+ * @max_length: maximum size of the table (ignore if 0)
* @table_header: where does the table start?
* @proc: array of acpi_subtable_proc struct containing entry id
* and associated handler with it
@@ -153,10 +154,11 @@ static __init_or_fwtbl_lib int call_handler(struct acpi_subtable_proc *proc,
int __init_or_fwtbl_lib
acpi_parse_entries_array(char *id, unsigned long table_size,
union fw_table_header *table_header,
+ unsigned long max_length,
struct acpi_subtable_proc *proc,
int proc_num, unsigned int max_entries)
{
- unsigned long table_end, subtable_len, entry_len;
+ unsigned long table_len, table_end, subtable_len, entry_len;
struct acpi_subtable_entry entry;
enum acpi_subtable_type type;
int count = 0;
@@ -164,8 +166,10 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
int i;
type = acpi_get_subtable_type(id);
- table_end = (unsigned long)table_header +
- acpi_table_get_length(type, table_header);
+ table_len = acpi_table_get_length(type, table_header);
+ if (max_length && max_length < table_len)
+ table_len = max_length;
+ table_end = (unsigned long)table_header + table_len;
/* Parse all entries looking for a match. */
@@ -220,7 +224,8 @@ int __init_or_fwtbl_lib
cdat_table_parse(enum acpi_cdat_type type,
acpi_tbl_entry_handler_arg handler_arg,
void *arg,
- struct acpi_table_cdat *table_header)
+ struct acpi_table_cdat *table_header,
+ unsigned long length)
{
struct acpi_subtable_proc proc = {
.id = type,
@@ -234,6 +239,6 @@ cdat_table_parse(enum acpi_cdat_type type,
return acpi_parse_entries_array(ACPI_SIG_CDAT,
sizeof(struct acpi_table_cdat),
(union fw_table_header *)table_header,
- &proc, 1, 0);
+ length, &proc, 1, 0);
}
EXPORT_SYMBOL_FWTBL_LIB(cdat_table_parse);
--
2.39.2
Robert Richter wrote:
> The last entry in the CDAT table may not mark the end of the CDAT
> table buffer specified by the length field in the CDAT header. It can
> be shorter with trailing unused (zero'ed) data. The actual table
> length is determined when reading all CDAT entries of the table with
> DOE.
>
> If the table is greater than expected (containing zero'ed trailing
> data), the CDAT parser fails with:
>
> [ 48.691717] Malformed DSMAS table length: (24:0)
> [ 48.702084] [CDAT:0x00] Invalid zero length
> [ 48.711460] cxl_port endpoint1: Failed to parse CDAT: -22
>
> In addition, the table buffer size can be different from the size
> specified in the length field. This may cause out-of-bound access then
> parsing the CDAT table.
>
> Fix that by providing an optonal buffer length argument to
> acpi_parse_entries_array() that can be used by cdat_table_parse() to
> propagate the buffer size down to its users.
>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Len Brown <[email protected]>
> Signed-off-by: Robert Richter <[email protected]>
[..]
> diff --git a/lib/fw_table.c b/lib/fw_table.c
> index 1e5e0b2f7012..ddb67853b7ac 100644
> --- a/lib/fw_table.c
> +++ b/lib/fw_table.c
[..]
> @@ -164,8 +166,10 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
> int i;
>
> type = acpi_get_subtable_type(id);
> - table_end = (unsigned long)table_header +
> - acpi_table_get_length(type, table_header);
> + table_len = acpi_table_get_length(type, table_header);
> + if (max_length && max_length < table_len)
> + table_len = max_length;
The other patches in this series look good, my only quibble here is
that this is an open-coded min_not_zero(). If nothing else pops up in
testing that triggers a v3 I will just fix that up locally.
Thanks, Robert!
On 1/8/24 04:48, Robert Richter wrote:
> Trivial variable rename for the DOE mailbox handle from cdat_doe to
> doe_mb. The variable name cdat_doe is too ambiguous, use doe_mb that
> is commonly used for the mailbox.
>
> Signed-off-by: Robert Richter <[email protected]>
Reviewed-by: Dave Jiang <[email protected]>
> ---
> drivers/cxl/core/pci.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 6c9c8d92f8f7..89bab00bb291 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -518,14 +518,14 @@ EXPORT_SYMBOL_NS_GPL(cxl_hdm_decode_init, CXL);
> FIELD_PREP(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE, (entry_handle)))
>
> static int cxl_cdat_get_length(struct device *dev,
> - struct pci_doe_mb *cdat_doe,
> + struct pci_doe_mb *doe_mb,
> size_t *length)
> {
> __le32 request = CDAT_DOE_REQ(0);
> __le32 response[2];
> int rc;
>
> - rc = pci_doe(cdat_doe, PCI_DVSEC_VENDOR_ID_CXL,
> + rc = pci_doe(doe_mb, PCI_DVSEC_VENDOR_ID_CXL,
> CXL_DOE_PROTOCOL_TABLE_ACCESS,
> &request, sizeof(request),
> &response, sizeof(response));
> @@ -543,7 +543,7 @@ static int cxl_cdat_get_length(struct device *dev,
> }
>
> static int cxl_cdat_read_table(struct device *dev,
> - struct pci_doe_mb *cdat_doe,
> + struct pci_doe_mb *doe_mb,
> void *cdat_table, size_t *cdat_length)
> {
> size_t length = *cdat_length + sizeof(__le32);
> @@ -557,7 +557,7 @@ static int cxl_cdat_read_table(struct device *dev,
> size_t entry_dw;
> int rc;
>
> - rc = pci_doe(cdat_doe, PCI_DVSEC_VENDOR_ID_CXL,
> + rc = pci_doe(doe_mb, PCI_DVSEC_VENDOR_ID_CXL,
> CXL_DOE_PROTOCOL_TABLE_ACCESS,
> &request, sizeof(request),
> data, length);
> @@ -617,7 +617,7 @@ void read_cdat_data(struct cxl_port *port)
> {
> struct device *uport = port->uport_dev;
> struct device *dev = &port->dev;
> - struct pci_doe_mb *cdat_doe;
> + struct pci_doe_mb *doe_mb;
> struct pci_dev *pdev = NULL;
> struct cxl_memdev *cxlmd;
> size_t cdat_length;
> @@ -638,16 +638,16 @@ void read_cdat_data(struct cxl_port *port)
> if (!pdev)
> return;
>
> - cdat_doe = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL,
> - CXL_DOE_PROTOCOL_TABLE_ACCESS);
> - if (!cdat_doe) {
> + doe_mb = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL,
> + CXL_DOE_PROTOCOL_TABLE_ACCESS);
> + if (!doe_mb) {
> dev_dbg(dev, "No CDAT mailbox\n");
> return;
> }
>
> port->cdat_available = true;
>
> - if (cxl_cdat_get_length(dev, cdat_doe, &cdat_length)) {
> + if (cxl_cdat_get_length(dev, doe_mb, &cdat_length)) {
> dev_dbg(dev, "No CDAT length\n");
> return;
> }
> @@ -656,7 +656,7 @@ void read_cdat_data(struct cxl_port *port)
> if (!cdat_buf)
> return;
>
> - rc = cxl_cdat_read_table(dev, cdat_doe, cdat_buf, &cdat_length);
> + rc = cxl_cdat_read_table(dev, doe_mb, cdat_buf, &cdat_length);
> if (rc)
> goto err;
>
On 1/8/24 04:48, Robert Richter wrote:
> The last entry in the CDAT table may not mark the end of the CDAT
> table buffer specified by the length field in the CDAT header. It can
> be shorter with trailing unused (zero'ed) data. The actual table
> length is determined when reading all CDAT entries of the table with
> DOE.
>
> If the table is greater than expected (containing zero'ed trailing
> data), the CDAT parser fails with:
>
> [ 48.691717] Malformed DSMAS table length: (24:0)
> [ 48.702084] [CDAT:0x00] Invalid zero length
> [ 48.711460] cxl_port endpoint1: Failed to parse CDAT: -22
>
> In addition, the table buffer size can be different from the size
> specified in the length field. This may cause out-of-bound access then
> parsing the CDAT table.
>
> Fix that by providing an optonal buffer length argument to
> acpi_parse_entries_array() that can be used by cdat_table_parse() to
> propagate the buffer size down to its users.
>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Len Brown <[email protected]>
> Signed-off-by: Robert Richter <[email protected]>
Reviewed-by: Dave Jiang <[email protected]>
> ---
> drivers/acpi/tables.c | 2 +-
> drivers/cxl/core/cdat.c | 6 +++---
> include/linux/fw_table.h | 4 +++-
> lib/fw_table.c | 15 ++++++++++-----
> 4 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index b07f7d091d13..b976e5fc3fbc 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -253,7 +253,7 @@ int __init_or_acpilib acpi_table_parse_entries_array(
>
> count = acpi_parse_entries_array(id, table_size,
> (union fw_table_header *)table_header,
> - proc, proc_num, max_entries);
> + 0, proc, proc_num, max_entries);
>
> acpi_put_table(table_header);
> return count;
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index 6fe11546889f..012d8f2a7945 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -149,13 +149,13 @@ static int cxl_cdat_endpoint_process(struct cxl_port *port,
> int rc;
>
> rc = cdat_table_parse(ACPI_CDAT_TYPE_DSMAS, cdat_dsmas_handler,
> - dsmas_xa, port->cdat.table);
> + dsmas_xa, port->cdat.table, port->cdat.length);
> rc = cdat_table_parse_output(rc);
> if (rc)
> return rc;
>
> rc = cdat_table_parse(ACPI_CDAT_TYPE_DSLBIS, cdat_dslbis_handler,
> - dsmas_xa, port->cdat.table);
> + dsmas_xa, port->cdat.table, port->cdat.length);
> return cdat_table_parse_output(rc);
> }
>
> @@ -511,7 +511,7 @@ void cxl_switch_parse_cdat(struct cxl_port *port)
> return;
>
> rc = cdat_table_parse(ACPI_CDAT_TYPE_SSLBIS, cdat_sslbis_handler,
> - port, port->cdat.table);
> + port, port->cdat.table, port->cdat.length);
> rc = cdat_table_parse_output(rc);
> if (rc)
> dev_dbg(&port->dev, "Failed to parse SSLBIS: %d\n", rc);
> diff --git a/include/linux/fw_table.h b/include/linux/fw_table.h
> index 95421860397a..3ff4c277296f 100644
> --- a/include/linux/fw_table.h
> +++ b/include/linux/fw_table.h
> @@ -40,12 +40,14 @@ union acpi_subtable_headers {
>
> int acpi_parse_entries_array(char *id, unsigned long table_size,
> union fw_table_header *table_header,
> + unsigned long max_length,
> struct acpi_subtable_proc *proc,
> int proc_num, unsigned int max_entries);
>
> int cdat_table_parse(enum acpi_cdat_type type,
> acpi_tbl_entry_handler_arg handler_arg, void *arg,
> - struct acpi_table_cdat *table_header);
> + struct acpi_table_cdat *table_header,
> + unsigned long length);
>
> /* CXL is the only non-ACPI consumer of the FIRMWARE_TABLE library */
> #if IS_ENABLED(CONFIG_ACPI) && !IS_ENABLED(CONFIG_CXL_BUS)
> diff --git a/lib/fw_table.c b/lib/fw_table.c
> index 1e5e0b2f7012..ddb67853b7ac 100644
> --- a/lib/fw_table.c
> +++ b/lib/fw_table.c
> @@ -132,6 +132,7 @@ static __init_or_fwtbl_lib int call_handler(struct acpi_subtable_proc *proc,
> *
> * @id: table id (for debugging purposes)
> * @table_size: size of the root table
> + * @max_length: maximum size of the table (ignore if 0)
> * @table_header: where does the table start?
> * @proc: array of acpi_subtable_proc struct containing entry id
> * and associated handler with it
> @@ -153,10 +154,11 @@ static __init_or_fwtbl_lib int call_handler(struct acpi_subtable_proc *proc,
> int __init_or_fwtbl_lib
> acpi_parse_entries_array(char *id, unsigned long table_size,
> union fw_table_header *table_header,
> + unsigned long max_length,
> struct acpi_subtable_proc *proc,
> int proc_num, unsigned int max_entries)
> {
> - unsigned long table_end, subtable_len, entry_len;
> + unsigned long table_len, table_end, subtable_len, entry_len;
> struct acpi_subtable_entry entry;
> enum acpi_subtable_type type;
> int count = 0;
> @@ -164,8 +166,10 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
> int i;
>
> type = acpi_get_subtable_type(id);
> - table_end = (unsigned long)table_header +
> - acpi_table_get_length(type, table_header);
> + table_len = acpi_table_get_length(type, table_header);
> + if (max_length && max_length < table_len)
> + table_len = max_length;
> + table_end = (unsigned long)table_header + table_len;
>
> /* Parse all entries looking for a match. */
>
> @@ -220,7 +224,8 @@ int __init_or_fwtbl_lib
> cdat_table_parse(enum acpi_cdat_type type,
> acpi_tbl_entry_handler_arg handler_arg,
> void *arg,
> - struct acpi_table_cdat *table_header)
> + struct acpi_table_cdat *table_header,
> + unsigned long length)
> {
> struct acpi_subtable_proc proc = {
> .id = type,
> @@ -234,6 +239,6 @@ cdat_table_parse(enum acpi_cdat_type type,
> return acpi_parse_entries_array(ACPI_SIG_CDAT,
> sizeof(struct acpi_table_cdat),
> (union fw_table_header *)table_header,
> - &proc, 1, 0);
> + length, &proc, 1, 0);
> }
> EXPORT_SYMBOL_FWTBL_LIB(cdat_table_parse);
On Mon, 8 Jan 2024 12:48:31 +0100
Robert Richter <[email protected]> wrote:
> Trivial variable rename for the DOE mailbox handle from cdat_doe to
> doe_mb. The variable name cdat_doe is too ambiguous, use doe_mb that
> is commonly used for the mailbox.
>
> Signed-off-by: Robert Richter <[email protected]>
I don't feel strongly about this one either way, but I've probably spent
too long looking at this code in the past.
Reviewed-by: Jonathan Cameron <[email protected]>
> ---
> drivers/cxl/core/pci.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 6c9c8d92f8f7..89bab00bb291 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -518,14 +518,14 @@ EXPORT_SYMBOL_NS_GPL(cxl_hdm_decode_init, CXL);
> FIELD_PREP(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE, (entry_handle)))
>
> static int cxl_cdat_get_length(struct device *dev,
> - struct pci_doe_mb *cdat_doe,
> + struct pci_doe_mb *doe_mb,
> size_t *length)
> {
> __le32 request = CDAT_DOE_REQ(0);
> __le32 response[2];
> int rc;
>
> - rc = pci_doe(cdat_doe, PCI_DVSEC_VENDOR_ID_CXL,
> + rc = pci_doe(doe_mb, PCI_DVSEC_VENDOR_ID_CXL,
> CXL_DOE_PROTOCOL_TABLE_ACCESS,
> &request, sizeof(request),
> &response, sizeof(response));
> @@ -543,7 +543,7 @@ static int cxl_cdat_get_length(struct device *dev,
> }
>
> static int cxl_cdat_read_table(struct device *dev,
> - struct pci_doe_mb *cdat_doe,
> + struct pci_doe_mb *doe_mb,
> void *cdat_table, size_t *cdat_length)
> {
> size_t length = *cdat_length + sizeof(__le32);
> @@ -557,7 +557,7 @@ static int cxl_cdat_read_table(struct device *dev,
> size_t entry_dw;
> int rc;
>
> - rc = pci_doe(cdat_doe, PCI_DVSEC_VENDOR_ID_CXL,
> + rc = pci_doe(doe_mb, PCI_DVSEC_VENDOR_ID_CXL,
> CXL_DOE_PROTOCOL_TABLE_ACCESS,
> &request, sizeof(request),
> data, length);
> @@ -617,7 +617,7 @@ void read_cdat_data(struct cxl_port *port)
> {
> struct device *uport = port->uport_dev;
> struct device *dev = &port->dev;
> - struct pci_doe_mb *cdat_doe;
> + struct pci_doe_mb *doe_mb;
> struct pci_dev *pdev = NULL;
> struct cxl_memdev *cxlmd;
> size_t cdat_length;
> @@ -638,16 +638,16 @@ void read_cdat_data(struct cxl_port *port)
> if (!pdev)
> return;
>
> - cdat_doe = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL,
> - CXL_DOE_PROTOCOL_TABLE_ACCESS);
> - if (!cdat_doe) {
> + doe_mb = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL,
> + CXL_DOE_PROTOCOL_TABLE_ACCESS);
> + if (!doe_mb) {
> dev_dbg(dev, "No CDAT mailbox\n");
> return;
> }
>
> port->cdat_available = true;
>
> - if (cxl_cdat_get_length(dev, cdat_doe, &cdat_length)) {
> + if (cxl_cdat_get_length(dev, doe_mb, &cdat_length)) {
> dev_dbg(dev, "No CDAT length\n");
> return;
> }
> @@ -656,7 +656,7 @@ void read_cdat_data(struct cxl_port *port)
> if (!cdat_buf)
> return;
>
> - rc = cxl_cdat_read_table(dev, cdat_doe, cdat_buf, &cdat_length);
> + rc = cxl_cdat_read_table(dev, doe_mb, cdat_buf, &cdat_length);
> if (rc)
> goto err;
>
On Mon, 8 Jan 2024 12:48:33 +0100
Robert Richter <[email protected]> wrote:
> The last entry in the CDAT table may not mark the end of the CDAT
> table buffer specified by the length field in the CDAT header. It can
> be shorter with trailing unused (zero'ed) data. The actual table
> length is determined when reading all CDAT entries of the table with
> DOE.
Can you give some reasons why this would occur?
Need to be clear if this is:
1) Hardening against device returning borked table.
2) Hardening against in flight update of CDAT racing with the readout
(not sure table can change size, but maybe.. I haven't checked).
3) DW read back vs packed structures?
Patch seems reasonable to me, I'd just like a clear statement of why
it happens!
Jonathan
>
> If the table is greater than expected (containing zero'ed trailing
> data), the CDAT parser fails with:
>
> [ 48.691717] Malformed DSMAS table length: (24:0)
> [ 48.702084] [CDAT:0x00] Invalid zero length
> [ 48.711460] cxl_port endpoint1: Failed to parse CDAT: -22
>
> In addition, the table buffer size can be different from the size
> specified in the length field. This may cause out-of-bound access then
> parsing the CDAT table.
>
> Fix that by providing an optonal buffer length argument to
> acpi_parse_entries_array() that can be used by cdat_table_parse() to
> propagate the buffer size down to its users.
>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Len Brown <[email protected]>
> Signed-off-by: Robert Richter <[email protected]>
> ---
> drivers/acpi/tables.c | 2 +-
> drivers/cxl/core/cdat.c | 6 +++---
> include/linux/fw_table.h | 4 +++-
> lib/fw_table.c | 15 ++++++++++-----
> 4 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index b07f7d091d13..b976e5fc3fbc 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -253,7 +253,7 @@ int __init_or_acpilib acpi_table_parse_entries_array(
>
> count = acpi_parse_entries_array(id, table_size,
> (union fw_table_header *)table_header,
> - proc, proc_num, max_entries);
> + 0, proc, proc_num, max_entries);
>
> acpi_put_table(table_header);
> return count;
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index 6fe11546889f..012d8f2a7945 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -149,13 +149,13 @@ static int cxl_cdat_endpoint_process(struct cxl_port *port,
> int rc;
>
> rc = cdat_table_parse(ACPI_CDAT_TYPE_DSMAS, cdat_dsmas_handler,
> - dsmas_xa, port->cdat.table);
> + dsmas_xa, port->cdat.table, port->cdat.length);
> rc = cdat_table_parse_output(rc);
> if (rc)
> return rc;
>
> rc = cdat_table_parse(ACPI_CDAT_TYPE_DSLBIS, cdat_dslbis_handler,
> - dsmas_xa, port->cdat.table);
> + dsmas_xa, port->cdat.table, port->cdat.length);
> return cdat_table_parse_output(rc);
> }
>
> @@ -511,7 +511,7 @@ void cxl_switch_parse_cdat(struct cxl_port *port)
> return;
>
> rc = cdat_table_parse(ACPI_CDAT_TYPE_SSLBIS, cdat_sslbis_handler,
> - port, port->cdat.table);
> + port, port->cdat.table, port->cdat.length);
> rc = cdat_table_parse_output(rc);
> if (rc)
> dev_dbg(&port->dev, "Failed to parse SSLBIS: %d\n", rc);
> diff --git a/include/linux/fw_table.h b/include/linux/fw_table.h
> index 95421860397a..3ff4c277296f 100644
> --- a/include/linux/fw_table.h
> +++ b/include/linux/fw_table.h
> @@ -40,12 +40,14 @@ union acpi_subtable_headers {
>
> int acpi_parse_entries_array(char *id, unsigned long table_size,
> union fw_table_header *table_header,
> + unsigned long max_length,
> struct acpi_subtable_proc *proc,
> int proc_num, unsigned int max_entries);
>
> int cdat_table_parse(enum acpi_cdat_type type,
> acpi_tbl_entry_handler_arg handler_arg, void *arg,
> - struct acpi_table_cdat *table_header);
> + struct acpi_table_cdat *table_header,
> + unsigned long length);
>
> /* CXL is the only non-ACPI consumer of the FIRMWARE_TABLE library */
> #if IS_ENABLED(CONFIG_ACPI) && !IS_ENABLED(CONFIG_CXL_BUS)
> diff --git a/lib/fw_table.c b/lib/fw_table.c
> index 1e5e0b2f7012..ddb67853b7ac 100644
> --- a/lib/fw_table.c
> +++ b/lib/fw_table.c
> @@ -132,6 +132,7 @@ static __init_or_fwtbl_lib int call_handler(struct acpi_subtable_proc *proc,
> *
> * @id: table id (for debugging purposes)
> * @table_size: size of the root table
> + * @max_length: maximum size of the table (ignore if 0)
> * @table_header: where does the table start?
> * @proc: array of acpi_subtable_proc struct containing entry id
> * and associated handler with it
> @@ -153,10 +154,11 @@ static __init_or_fwtbl_lib int call_handler(struct acpi_subtable_proc *proc,
> int __init_or_fwtbl_lib
> acpi_parse_entries_array(char *id, unsigned long table_size,
> union fw_table_header *table_header,
> + unsigned long max_length,
> struct acpi_subtable_proc *proc,
> int proc_num, unsigned int max_entries)
> {
> - unsigned long table_end, subtable_len, entry_len;
> + unsigned long table_len, table_end, subtable_len, entry_len;
> struct acpi_subtable_entry entry;
> enum acpi_subtable_type type;
> int count = 0;
> @@ -164,8 +166,10 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
> int i;
>
> type = acpi_get_subtable_type(id);
> - table_end = (unsigned long)table_header +
> - acpi_table_get_length(type, table_header);
> + table_len = acpi_table_get_length(type, table_header);
> + if (max_length && max_length < table_len)
> + table_len = max_length;
> + table_end = (unsigned long)table_header + table_len;
>
> /* Parse all entries looking for a match. */
>
> @@ -220,7 +224,8 @@ int __init_or_fwtbl_lib
> cdat_table_parse(enum acpi_cdat_type type,
> acpi_tbl_entry_handler_arg handler_arg,
> void *arg,
> - struct acpi_table_cdat *table_header)
> + struct acpi_table_cdat *table_header,
> + unsigned long length)
> {
> struct acpi_subtable_proc proc = {
> .id = type,
> @@ -234,6 +239,6 @@ cdat_table_parse(enum acpi_cdat_type type,
> return acpi_parse_entries_array(ACPI_SIG_CDAT,
> sizeof(struct acpi_table_cdat),
> (union fw_table_header *)table_header,
> - &proc, 1, 0);
> + length, &proc, 1, 0);
> }
> EXPORT_SYMBOL_FWTBL_LIB(cdat_table_parse);
Hi Jonathan,
On 26.01.24 16:42:22, Jonathan Cameron wrote:
> On Mon, 8 Jan 2024 12:48:32 +0100
> Robert Richter <[email protected]> wrote:
>
> > Reading the CDAT table using DOE requires a Table Access Response
> > Header in addition to the CDAT entry. In current implementation this
> > has caused offsets with sizeof(__le32) to the actual buffers. This led
> > to hardly readable code and even bugs (see fix of devm_kfree() in
> > read_cdat_data()).
>
> Hi Robert,
>
> Referring to a fix in this patch set or one in another patch?
> If it's another patch a cross reference would be good. If it's in
> here this should have a fixes tag.
No, it is already fixed, see:
c65efe3685f5 cxl/cdat: Free correct buffer on checksum error
I will update the patch description.
>
> >
> > Rework code to avoid calculations with sizeof(__le32). Introduce
> > struct cdat_doe for this which contains the Table Access Response
> > Header and a variable payload size for various data structures
> > afterwards to access the CDAT table and its CDAT Data Structures
> > without recalculating buffer offsets.
>
> Comments inline, but I don't like using variable element arrays when
> we know that they are of size 1 but then rely on their minimum in
> the code being size 0. Better I think to make it explicit that there
> is one of each and use more complex offsetof() handling that doesn't
> give the impression the number of elements varies.
I have an update in place that addresses this, will send a v3.
>
>
> >
> > Cc: Lukas Wunner <[email protected]>
> > Cc: Fan Ni <[email protected]>
> > Reviewed-by: Dave Jiang <[email protected]>
> > Signed-off-by: Robert Richter <[email protected]>
> > ---
> > drivers/cxl/core/pci.c | 67 +++++++++++++++++++++---------------------
> > drivers/cxl/cxlpci.h | 19 ++++++++++++
> > 2 files changed, 53 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index 89bab00bb291..75d8fa228879 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> > @@ -544,55 +544,53 @@ static int cxl_cdat_get_length(struct device *dev,
> >
> > static int cxl_cdat_read_table(struct device *dev,
> > struct pci_doe_mb *doe_mb,
> > - void *cdat_table, size_t *cdat_length)
> > + struct cdat_doe_rsp *rsp, size_t *length)
> > {
> > - size_t length = *cdat_length + sizeof(__le32);
> > - __le32 *data = cdat_table;
> > + size_t received, remaining = *length;
> > int entry_handle = 0;
> > __le32 saved_dw = 0;
> >
> > do {
> > __le32 request = CDAT_DOE_REQ(entry_handle);
> > - struct cdat_entry_header *entry;
> > - size_t entry_dw;
> > int rc;
> >
> > rc = pci_doe(doe_mb, PCI_DVSEC_VENDOR_ID_CXL,
> > CXL_DOE_PROTOCOL_TABLE_ACCESS,
> > &request, sizeof(request),
> > - data, length);
> > + rsp, sizeof(*rsp) + remaining);
> > if (rc < 0) {
> > dev_err(dev, "DOE failed: %d", rc);
> > return rc;
> > }
> >
> > - /* 1 DW Table Access Response Header + CDAT entry */
> > - entry = (struct cdat_entry_header *)(data + 1);
> > + if (rc < sizeof(*rsp))
> > + return -EIO;
> > +
> > + received = rc - sizeof(*rsp);
>
> As mentioned below, I'd rather see this as offsetof() to get
> to a real element in the structure rather that relying on a flex array
> for a known single element array.
>
> It is still a little messy because the offsetof() variable should be different
> for the cdat header and the entries even though we know they are actually
> in the same place.
>
> if ((entry_handle == 0) {
> received = rc - offsetof(typeof(*rsp), header);
> if (received != sizeof(rsp->header))
> return -EIO;
> } else {
> received = rc - offsetof(typeof(*rsp), entry);
> if (received < sizeof(rsp->entry) ||
> received != le16_to_cpu(rsp->entry->length))
> return -EIO;
> }
>
>
>
> > +
> > if ((entry_handle == 0 &&
> > - rc != sizeof(__le32) + sizeof(struct cdat_header)) ||
> > + received != sizeof(rsp->header[0])) ||
> > (entry_handle > 0 &&
>
> Why are we letting entry_handle go negative anyway?
Will make that unsigned. The check means to be (entry_handle != 0) here.
>
>
> > - (rc < sizeof(__le32) + sizeof(*entry) ||
> > - rc != sizeof(__le32) + le16_to_cpu(entry->length))))
> > + (received < sizeof(rsp->entry[0]) ||
> > + received != le16_to_cpu(rsp->entry->length))))
> > return -EIO;
> >
> > /* Get the CXL table access header entry handle */
> > entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE,
> > - le32_to_cpu(data[0]));
> > - entry_dw = rc / sizeof(__le32);
> > - /* Skip Header */
> > - entry_dw -= 1;
> > + le32_to_cpu(rsp->doe_header));
> > +
> > /*
> > * Table Access Response Header overwrote the last DW of
> > * previous entry, so restore that DW
> > */
> > - *data = saved_dw;
> > - length -= entry_dw * sizeof(__le32);
> > - data += entry_dw;
> > - saved_dw = *data;
> > + rsp->doe_header = saved_dw;
> > + remaining -= received;
> > + rsp = (void *)rsp + received;
> > + saved_dw = rsp->doe_header;
> > } while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY);
> >
> > /* Length in CDAT header may exceed concatenation of CDAT entries */
> > - *cdat_length -= length - sizeof(__le32);
> > + *length -= remaining;
> >
> > return 0;
> > }
> > @@ -620,8 +618,8 @@ void read_cdat_data(struct cxl_port *port)
> > struct pci_doe_mb *doe_mb;
> > struct pci_dev *pdev = NULL;
> > struct cxl_memdev *cxlmd;
> > - size_t cdat_length;
> > - void *cdat_table, *cdat_buf;
> > + struct cdat_doe_rsp *buf;
> > + size_t length;
> > int rc;
> >
> > if (is_cxl_memdev(uport)) {
> > @@ -647,30 +645,33 @@ void read_cdat_data(struct cxl_port *port)
> >
> > port->cdat_available = true;
> >
> > - if (cxl_cdat_get_length(dev, doe_mb, &cdat_length)) {
> > + if (cxl_cdat_get_length(dev, doe_mb, &length)) {
> > dev_dbg(dev, "No CDAT length\n");
> > return;
> > }
> >
> > - cdat_buf = devm_kzalloc(dev, cdat_length + sizeof(__le32), GFP_KERNEL);
> > - if (!cdat_buf)
> > - return;
> > + /*
> > + * The begin of the CDAT buffer needs space for additional 4
> > + * bytes for the DOE header. Table data starts afterwards.
> > + */
> > + buf = devm_kzalloc(dev, sizeof(*buf) + length, GFP_KERNEL);
>
> with below suggestion this would need to use offsetof(... table)
> which isn't hugely obvious but there is definitely a trade off to be
> figured out somewhere and I think this is less painful than implying
> a variable number of entries etc.
>
> The implied structure of the data here is also disagreeing with below as
> I believe it's
>
> struct {
> u32 doe_header;
> struct cdat_header cdat_table_header;
> struct cdat_entry entries[];
> }
> Which is not implied by the type you have below.
>
> Hmm. I wonder if we should play games in the inner loop to avoid having
> that outer stray DWord here.
>
> Something like.
>
> 1. Assume that there is at least one CDAT entry of 4 bytes or more
> (seems reasonable).
> 2. Allocate the correct length and nothing more. Pass that into
> cxl_cdat_read_table()
> 3. Read the DOE header + the CDAT Header into that buffer.
> 4. Memmove result -4 bytes to drop the DOE header.
> 5. Rest as before complete with the stashing of the final DW
> each time so we can put it back when the next loop overwrites
> it by accident.
>
> Your structure is still useful for within the function, but not
> out here where it represents completely the wrong data layout.
> Here we just want the raw CDAT table anyway so represent it
> as u8 data[]; is fine as no odd offsets to deal with.
See my update I will send, it will be very simple and avoids zero
sized arrays.
Thanks for review, that will improve v3.
-Robert
>
>
> Jonathan
>
>
>
>
>
>
> > + if (!buf)
> > + goto err;
> >
> > - rc = cxl_cdat_read_table(dev, doe_mb, cdat_buf, &cdat_length);
> > + rc = cxl_cdat_read_table(dev, doe_mb, buf, &length);
> > if (rc)
> > goto err;
> >
> > - cdat_table = cdat_buf + sizeof(__le32);
> > - if (cdat_checksum(cdat_table, cdat_length))
> > + if (cdat_checksum(buf->table, length))
> > goto err;
> >
> > - port->cdat.table = cdat_table;
> > - port->cdat.length = cdat_length;
> > - return;
> > + port->cdat.table = buf->table;
> > + port->cdat.length = length;
> >
> > + return;
> > err:
> > /* Don't leave table data allocated on error */
> > - devm_kfree(dev, cdat_buf);
> > + devm_kfree(dev, buf);
> > dev_err(dev, "Failed to read/validate CDAT.\n");
> > }
> > EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
> > diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> > index 711b05d9a370..b40c571b2ab2 100644
> > --- a/drivers/cxl/cxlpci.h
> > +++ b/drivers/cxl/cxlpci.h
> > @@ -85,6 +85,25 @@ struct cdat_entry_header {
> > __le16 length;
> > } __packed;
> >
> > +/*
> > + * Response contains the CDAT only response header of the DOE. The
> > + * response payload is a CDAT structure (either CDAT header or entry),
> > + * it may also mark the beginning of the CDAT table.
> > + *
> > + * Spec refs:
> > + *
> > + * CXL 3.1 Table 8-14: Read Entry Response
> > + * CDAT Specification 1.03: 2 CDAT Data Structures
> > + */
> > +struct cdat_doe_rsp {
> > + __le32 doe_header;
> > + union {
> > + DECLARE_FLEX_ARRAY(u8, table);
> > + DECLARE_FLEX_ARRAY(struct cdat_header, header);
> > + DECLARE_FLEX_ARRAY(struct cdat_entry_header, entry);
>
> I'm not understanding why these last two are flex arrays...
> Each DOE resp only includes either 1 cdat_header or 1 cdat_entry_header
> For table fair enough.
>
> If you want a general response I think it needs to be something like
>
> struct cdat_doe_rsp {
> union {
> DECLARE_FLEX_ARRAY(u8, table);
> struct cdat_header header;
> struct {
> struct cdat_entry_header entry;
> u8 entry_data[];
> };
> };
> } __packed;
>
> That doesn't make it useful for above unless you use
> offsetof() to establish you are really interested in how far in to
> get to the real data.
>
>
> > + };
> > +} __packed;
> > +
> > /*
> > * CXL v3.0 6.2.3 Table 6-4
> > * The table indicates that if PCIe Flit Mode is set, then CXL is in 256B flits
>
On 26.01.24 16:46:03, Jonathan Cameron wrote:
> On Mon, 8 Jan 2024 12:48:33 +0100
> Robert Richter <[email protected]> wrote:
>
> > The last entry in the CDAT table may not mark the end of the CDAT
> > table buffer specified by the length field in the CDAT header. It can
> > be shorter with trailing unused (zero'ed) data. The actual table
> > length is determined when reading all CDAT entries of the table with
> > DOE.
>
> Can you give some reasons why this would occur?
I have seen card implementations where the CDAT table is some sort of
fix buffer, but with entries filled in that do not fill the whole
table length size. Which means that the last DOE ends earlier than the
table end that then contains padding bytes. Spec is not entierly clear
here. It could be interpreted as a spec violation, but DOE is the card
vendor's firmware there is not much that can be done to fix that
there...
>
> Need to be clear if this is:
> 1) Hardening against device returning borked table.
So this was the main motivation. It will be likely there are more
cards with that issue.
> 2) Hardening against in flight update of CDAT racing with the readout
> (not sure table can change size, but maybe.. I haven't checked).
That is a side effect I realized while implementing this patch. To
prevent an out of bound buffer access, either the buffer needs a
length argument or the length field in the header needs to be checked.
The earlier is more reasonable and natural to me as no buffer insight
is needed for that.
> 3) DW read back vs packed structures?
Good point. But that was not the reason. IIR the PCI DOE code
correctly, there is a DW access but only the missing bytes are written
to the buffer. The CDAT structures are all DW aligned, but the DOE
header isn't.
>
> Patch seems reasonable to me, I'd just like a clear statement of why
> it happens!
So regardless of padding bytes or not, a length check is required in
any case, one or the other way.
Will update the patch description.
Thanks,
-Robert
>
> Jonathan
>
> >
> > If the table is greater than expected (containing zero'ed trailing
> > data), the CDAT parser fails with:
> >
> > [ 48.691717] Malformed DSMAS table length: (24:0)
> > [ 48.702084] [CDAT:0x00] Invalid zero length
> > [ 48.711460] cxl_port endpoint1: Failed to parse CDAT: -22
> >
> > In addition, the table buffer size can be different from the size
> > specified in the length field. This may cause out-of-bound access then
> > parsing the CDAT table.
> >
> > Fix that by providing an optonal buffer length argument to
> > acpi_parse_entries_array() that can be used by cdat_table_parse() to
> > propagate the buffer size down to its users.
> >
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > Cc: Len Brown <[email protected]>
> > Signed-off-by: Robert Richter <[email protected]>
> > ---
> > drivers/acpi/tables.c | 2 +-
> > drivers/cxl/core/cdat.c | 6 +++---
> > include/linux/fw_table.h | 4 +++-
> > lib/fw_table.c | 15 ++++++++++-----
> > 4 files changed, 17 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> > index b07f7d091d13..b976e5fc3fbc 100644
> > --- a/drivers/acpi/tables.c
> > +++ b/drivers/acpi/tables.c
> > @@ -253,7 +253,7 @@ int __init_or_acpilib acpi_table_parse_entries_array(
> >
> > count = acpi_parse_entries_array(id, table_size,
> > (union fw_table_header *)table_header,
> > - proc, proc_num, max_entries);
> > + 0, proc, proc_num, max_entries);
> >
> > acpi_put_table(table_header);
> > return count;
> > diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> > index 6fe11546889f..012d8f2a7945 100644
> > --- a/drivers/cxl/core/cdat.c
> > +++ b/drivers/cxl/core/cdat.c
> > @@ -149,13 +149,13 @@ static int cxl_cdat_endpoint_process(struct cxl_port *port,
> > int rc;
> >
> > rc = cdat_table_parse(ACPI_CDAT_TYPE_DSMAS, cdat_dsmas_handler,
> > - dsmas_xa, port->cdat.table);
> > + dsmas_xa, port->cdat.table, port->cdat.length);
> > rc = cdat_table_parse_output(rc);
> > if (rc)
> > return rc;
> >
> > rc = cdat_table_parse(ACPI_CDAT_TYPE_DSLBIS, cdat_dslbis_handler,
> > - dsmas_xa, port->cdat.table);
> > + dsmas_xa, port->cdat.table, port->cdat.length);
> > return cdat_table_parse_output(rc);
> > }
> >
> > @@ -511,7 +511,7 @@ void cxl_switch_parse_cdat(struct cxl_port *port)
> > return;
> >
> > rc = cdat_table_parse(ACPI_CDAT_TYPE_SSLBIS, cdat_sslbis_handler,
> > - port, port->cdat.table);
> > + port, port->cdat.table, port->cdat.length);
> > rc = cdat_table_parse_output(rc);
> > if (rc)
> > dev_dbg(&port->dev, "Failed to parse SSLBIS: %d\n", rc);
> > diff --git a/include/linux/fw_table.h b/include/linux/fw_table.h
> > index 95421860397a..3ff4c277296f 100644
> > --- a/include/linux/fw_table.h
> > +++ b/include/linux/fw_table.h
> > @@ -40,12 +40,14 @@ union acpi_subtable_headers {
> >
> > int acpi_parse_entries_array(char *id, unsigned long table_size,
> > union fw_table_header *table_header,
> > + unsigned long max_length,
> > struct acpi_subtable_proc *proc,
> > int proc_num, unsigned int max_entries);
> >
> > int cdat_table_parse(enum acpi_cdat_type type,
> > acpi_tbl_entry_handler_arg handler_arg, void *arg,
> > - struct acpi_table_cdat *table_header);
> > + struct acpi_table_cdat *table_header,
> > + unsigned long length);
> >
> > /* CXL is the only non-ACPI consumer of the FIRMWARE_TABLE library */
> > #if IS_ENABLED(CONFIG_ACPI) && !IS_ENABLED(CONFIG_CXL_BUS)
> > diff --git a/lib/fw_table.c b/lib/fw_table.c
> > index 1e5e0b2f7012..ddb67853b7ac 100644
> > --- a/lib/fw_table.c
> > +++ b/lib/fw_table.c
> > @@ -132,6 +132,7 @@ static __init_or_fwtbl_lib int call_handler(struct acpi_subtable_proc *proc,
> > *
> > * @id: table id (for debugging purposes)
> > * @table_size: size of the root table
> > + * @max_length: maximum size of the table (ignore if 0)
> > * @table_header: where does the table start?
> > * @proc: array of acpi_subtable_proc struct containing entry id
> > * and associated handler with it
> > @@ -153,10 +154,11 @@ static __init_or_fwtbl_lib int call_handler(struct acpi_subtable_proc *proc,
> > int __init_or_fwtbl_lib
> > acpi_parse_entries_array(char *id, unsigned long table_size,
> > union fw_table_header *table_header,
> > + unsigned long max_length,
> > struct acpi_subtable_proc *proc,
> > int proc_num, unsigned int max_entries)
> > {
> > - unsigned long table_end, subtable_len, entry_len;
> > + unsigned long table_len, table_end, subtable_len, entry_len;
> > struct acpi_subtable_entry entry;
> > enum acpi_subtable_type type;
> > int count = 0;
> > @@ -164,8 +166,10 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
> > int i;
> >
> > type = acpi_get_subtable_type(id);
> > - table_end = (unsigned long)table_header +
> > - acpi_table_get_length(type, table_header);
> > + table_len = acpi_table_get_length(type, table_header);
> > + if (max_length && max_length < table_len)
> > + table_len = max_length;
> > + table_end = (unsigned long)table_header + table_len;
> >
> > /* Parse all entries looking for a match. */
> >
> > @@ -220,7 +224,8 @@ int __init_or_fwtbl_lib
> > cdat_table_parse(enum acpi_cdat_type type,
> > acpi_tbl_entry_handler_arg handler_arg,
> > void *arg,
> > - struct acpi_table_cdat *table_header)
> > + struct acpi_table_cdat *table_header,
> > + unsigned long length)
> > {
> > struct acpi_subtable_proc proc = {
> > .id = type,
> > @@ -234,6 +239,6 @@ cdat_table_parse(enum acpi_cdat_type type,
> > return acpi_parse_entries_array(ACPI_SIG_CDAT,
> > sizeof(struct acpi_table_cdat),
> > (union fw_table_header *)table_header,
> > - &proc, 1, 0);
> > + length, &proc, 1, 0);
> > }
> > EXPORT_SYMBOL_FWTBL_LIB(cdat_table_parse);
>
On Fri, 9 Feb 2024 12:23:39 +0100
Robert Richter <[email protected]> wrote:
> On 26.01.24 16:46:03, Jonathan Cameron wrote:
> > On Mon, 8 Jan 2024 12:48:33 +0100
> > Robert Richter <[email protected]> wrote:
> >
> > > The last entry in the CDAT table may not mark the end of the CDAT
> > > table buffer specified by the length field in the CDAT header. It can
> > > be shorter with trailing unused (zero'ed) data. The actual table
> > > length is determined when reading all CDAT entries of the table with
> > > DOE.
> >
> > Can you give some reasons why this would occur?
>
> I have seen card implementations where the CDAT table is some sort of
> fix buffer, but with entries filled in that do not fill the whole
> table length size. Which means that the last DOE ends earlier than the
> table end that then contains padding bytes. Spec is not entierly clear
> here. It could be interpreted as a spec violation, but DOE is the card
> vendor's firmware there is not much that can be done to fix that
> there...
We can fall back on the usual kernel developer nudge solution.
Print a rude message and hope their customers tell them to fix it ;)
>
> >
> > Need to be clear if this is:
> > 1) Hardening against device returning borked table.
>
> So this was the main motivation. It will be likely there are more
> cards with that issue.
>
> > 2) Hardening against in flight update of CDAT racing with the readout
> > (not sure table can change size, but maybe.. I haven't checked).
>
> That is a side effect I realized while implementing this patch. To
> prevent an out of bound buffer access, either the buffer needs a
> length argument or the length field in the header needs to be checked.
> The earlier is more reasonable and natural to me as no buffer insight
> is needed for that.
>
> > 3) DW read back vs packed structures?
>
> Good point. But that was not the reason. IIR the PCI DOE code
> correctly, there is a DW access but only the missing bytes are written
> to the buffer. The CDAT structures are all DW aligned, but the DOE
> header isn't.
Yes. I hoped that was still working :)
>
> >
> > Patch seems reasonable to me, I'd just like a clear statement of why
> > it happens!
>
> So regardless of padding bytes or not, a length check is required in
> any case, one or the other way.
>
> Will update the patch description.
Great.
>
> Thanks,
>
> -Robert
>
> >
> > Jonathan
> >
> > >
> > > If the table is greater than expected (containing zero'ed trailing
> > > data), the CDAT parser fails with:
> > >
> > > [ 48.691717] Malformed DSMAS table length: (24:0)
> > > [ 48.702084] [CDAT:0x00] Invalid zero length
> > > [ 48.711460] cxl_port endpoint1: Failed to parse CDAT: -22
> > >
> > > In addition, the table buffer size can be different from the size
> > > specified in the length field. This may cause out-of-bound access then
> > > parsing the CDAT table.
> > >
> > > Fix that by providing an optonal buffer length argument to
> > > acpi_parse_entries_array() that can be used by cdat_table_parse() to
> > > propagate the buffer size down to its users.
> > >
> > > Cc: "Rafael J. Wysocki" <[email protected]>
> > > Cc: Len Brown <[email protected]>
> > > Signed-off-by: Robert Richter <[email protected]>
> > > ---
> > > drivers/acpi/tables.c | 2 +-
> > > drivers/cxl/core/cdat.c | 6 +++---
> > > include/linux/fw_table.h | 4 +++-
> > > lib/fw_table.c | 15 ++++++++++-----
> > > 4 files changed, 17 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> > > index b07f7d091d13..b976e5fc3fbc 100644
> > > --- a/drivers/acpi/tables.c
> > > +++ b/drivers/acpi/tables.c
> > > @@ -253,7 +253,7 @@ int __init_or_acpilib acpi_table_parse_entries_array(
> > >
> > > count = acpi_parse_entries_array(id, table_size,
> > > (union fw_table_header *)table_header,
> > > - proc, proc_num, max_entries);
> > > + 0, proc, proc_num, max_entries);
> > >
> > > acpi_put_table(table_header);
> > > return count;
> > > diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> > > index 6fe11546889f..012d8f2a7945 100644
> > > --- a/drivers/cxl/core/cdat.c
> > > +++ b/drivers/cxl/core/cdat.c
> > > @@ -149,13 +149,13 @@ static int cxl_cdat_endpoint_process(struct cxl_port *port,
> > > int rc;
> > >
> > > rc = cdat_table_parse(ACPI_CDAT_TYPE_DSMAS, cdat_dsmas_handler,
> > > - dsmas_xa, port->cdat.table);
> > > + dsmas_xa, port->cdat.table, port->cdat.length);
> > > rc = cdat_table_parse_output(rc);
> > > if (rc)
> > > return rc;
> > >
> > > rc = cdat_table_parse(ACPI_CDAT_TYPE_DSLBIS, cdat_dslbis_handler,
> > > - dsmas_xa, port->cdat.table);
> > > + dsmas_xa, port->cdat.table, port->cdat.length);
> > > return cdat_table_parse_output(rc);
> > > }
> > >
> > > @@ -511,7 +511,7 @@ void cxl_switch_parse_cdat(struct cxl_port *port)
> > > return;
> > >
> > > rc = cdat_table_parse(ACPI_CDAT_TYPE_SSLBIS, cdat_sslbis_handler,
> > > - port, port->cdat.table);
> > > + port, port->cdat.table, port->cdat.length);
> > > rc = cdat_table_parse_output(rc);
> > > if (rc)
> > > dev_dbg(&port->dev, "Failed to parse SSLBIS: %d\n", rc);
> > > diff --git a/include/linux/fw_table.h b/include/linux/fw_table.h
> > > index 95421860397a..3ff4c277296f 100644
> > > --- a/include/linux/fw_table.h
> > > +++ b/include/linux/fw_table.h
> > > @@ -40,12 +40,14 @@ union acpi_subtable_headers {
> > >
> > > int acpi_parse_entries_array(char *id, unsigned long table_size,
> > > union fw_table_header *table_header,
> > > + unsigned long max_length,
> > > struct acpi_subtable_proc *proc,
> > > int proc_num, unsigned int max_entries);
> > >
> > > int cdat_table_parse(enum acpi_cdat_type type,
> > > acpi_tbl_entry_handler_arg handler_arg, void *arg,
> > > - struct acpi_table_cdat *table_header);
> > > + struct acpi_table_cdat *table_header,
> > > + unsigned long length);
> > >
> > > /* CXL is the only non-ACPI consumer of the FIRMWARE_TABLE library */
> > > #if IS_ENABLED(CONFIG_ACPI) && !IS_ENABLED(CONFIG_CXL_BUS)
> > > diff --git a/lib/fw_table.c b/lib/fw_table.c
> > > index 1e5e0b2f7012..ddb67853b7ac 100644
> > > --- a/lib/fw_table.c
> > > +++ b/lib/fw_table.c
> > > @@ -132,6 +132,7 @@ static __init_or_fwtbl_lib int call_handler(struct acpi_subtable_proc *proc,
> > > *
> > > * @id: table id (for debugging purposes)
> > > * @table_size: size of the root table
> > > + * @max_length: maximum size of the table (ignore if 0)
> > > * @table_header: where does the table start?
> > > * @proc: array of acpi_subtable_proc struct containing entry id
> > > * and associated handler with it
> > > @@ -153,10 +154,11 @@ static __init_or_fwtbl_lib int call_handler(struct acpi_subtable_proc *proc,
> > > int __init_or_fwtbl_lib
> > > acpi_parse_entries_array(char *id, unsigned long table_size,
> > > union fw_table_header *table_header,
> > > + unsigned long max_length,
> > > struct acpi_subtable_proc *proc,
> > > int proc_num, unsigned int max_entries)
> > > {
> > > - unsigned long table_end, subtable_len, entry_len;
> > > + unsigned long table_len, table_end, subtable_len, entry_len;
> > > struct acpi_subtable_entry entry;
> > > enum acpi_subtable_type type;
> > > int count = 0;
> > > @@ -164,8 +166,10 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
> > > int i;
> > >
> > > type = acpi_get_subtable_type(id);
> > > - table_end = (unsigned long)table_header +
> > > - acpi_table_get_length(type, table_header);
> > > + table_len = acpi_table_get_length(type, table_header);
> > > + if (max_length && max_length < table_len)
> > > + table_len = max_length;
> > > + table_end = (unsigned long)table_header + table_len;
> > >
> > > /* Parse all entries looking for a match. */
> > >
> > > @@ -220,7 +224,8 @@ int __init_or_fwtbl_lib
> > > cdat_table_parse(enum acpi_cdat_type type,
> > > acpi_tbl_entry_handler_arg handler_arg,
> > > void *arg,
> > > - struct acpi_table_cdat *table_header)
> > > + struct acpi_table_cdat *table_header,
> > > + unsigned long length)
> > > {
> > > struct acpi_subtable_proc proc = {
> > > .id = type,
> > > @@ -234,6 +239,6 @@ cdat_table_parse(enum acpi_cdat_type type,
> > > return acpi_parse_entries_array(ACPI_SIG_CDAT,
> > > sizeof(struct acpi_table_cdat),
> > > (union fw_table_header *)table_header,
> > > - &proc, 1, 0);
> > > + length, &proc, 1, 0);
> > > }
> > > EXPORT_SYMBOL_FWTBL_LIB(cdat_table_parse);
> >