Some CDAT related updates and fixes. Patch #3 does not depend on the
previous patches and could be applied separately.
Changelog:
v3 (Jonathan's review comments):
* added Reviewed-by tags
* made entry_handle unsigned
* updated patch decriptions for #2 and #3
* removed zero-sized arrays, use variable size arrays as data buffer,
introduced union cdat_data for parsing CDAT structs
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 | 91 +++++++++++++++++++++-------------------
drivers/cxl/cxlpci.h | 20 +++++++++
include/linux/fw_table.h | 4 +-
lib/fw_table.c | 15 ++++---
6 files changed, 84 insertions(+), 54 deletions(-)
base-commit: 6be99530c92c6b8ff7a01903edc42393575ad63b
--
2.39.2
There exists card implementations with a CDAT table using a fix
buffer, but with entries filled in that do not fill the whole table
length size. Then, 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 while 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, a check of the table buffer length is missing to prevent
an out-of-bound access then parsing the CDAT table.
Hardening code against device returning borked table. Fix that by
providing an optional 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 to check the buffer
length. This also prevents a possible out-of-bound access mentioned.
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 c3569d2ba503..16291814450e 100644
--- a/lib/fw_table.c
+++ b/lib/fw_table.c
@@ -127,6 +127,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
@@ -148,18 +149,21 @@ 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;
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. */
@@ -208,7 +212,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,
@@ -222,6 +227,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
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]>
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 480489f5644e..39366ce94985 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. E.g., see fix of devm_kfree()
in read_cdat_data():
c65efe3685f5 cxl/cdat: Free correct buffer on checksum error
Rework code to avoid calculations with sizeof(__le32). Introduce
struct cdat_doe_rsp 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 | 75 ++++++++++++++++++++++--------------------
drivers/cxl/cxlpci.h | 20 +++++++++++
2 files changed, 59 insertions(+), 36 deletions(-)
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 39366ce94985..569354a5536f 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -544,55 +544,55 @@ 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;
- int entry_handle = 0;
+ size_t received, remaining = *length;
+ unsigned int entry_handle = 0;
+ union cdat_data *data;
__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 ((entry_handle == 0 &&
- rc != sizeof(__le32) + sizeof(struct cdat_header)) ||
- (entry_handle > 0 &&
- (rc < sizeof(__le32) + sizeof(*entry) ||
- rc != sizeof(__le32) + le16_to_cpu(entry->length))))
+ if (rc < sizeof(*rsp))
+ return -EIO;
+
+ data = (void *)rsp->data;
+ received = rc - sizeof(*rsp);
+
+ if ((!entry_handle &&
+ received != sizeof(data->header)) ||
+ (entry_handle &&
+ (received < sizeof(data->entry) ||
+ received != le16_to_cpu(data->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 +620,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 +647,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->data, length))
goto err;
- port->cdat.table = cdat_table;
- port->cdat.length = cdat_length;
- return;
+ port->cdat.table = buf->data;
+ 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..152bd453c623 100644
--- a/drivers/cxl/cxlpci.h
+++ b/drivers/cxl/cxlpci.h
@@ -85,6 +85,26 @@ struct cdat_entry_header {
__le16 length;
} __packed;
+union cdat_data {
+ struct cdat_header header;
+ struct cdat_entry_header entry;
+} __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;
+ u8 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
--
2.39.2
On Wed, 14 Feb 2024 17:39:27 +0000
Jonathan Cameron <[email protected]> wrote:
> On Fri, 9 Feb 2024 20:26:47 +0100
> Robert Richter <[email protected]> wrote:
>
> > There exists card implementations with a CDAT table using a fix
> There exist ... fixed size buffer,
> > buffer, but with entries filled in that do not fill the whole table
> > length size. Then, 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 while 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, a check of the table buffer length is missing to prevent
> > an out-of-bound access then parsing the CDAT table.
> >
> > Hardening code against device returning borked table. Fix that by
> > providing an optional 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 to check the buffer
> > length. This also prevents a possible out-of-bound access mentioned.
> >
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > Cc: Len Brown <[email protected]>
> > Signed-off-by: Robert Richter <[email protected]>
> > Reviewed-by: Dave Jiang <[email protected]>
>
> I think we should scream a bit about this if we see it
> as I'm unconvinced the spec allows for an implementation like this.
>
> If the spec is unclear, lets seek a clarification.
>
> I'm fine with this as a defensive measure, I just don't want
> device vendors to keep doing it!
>
Scrub that - I got around to checking the CDAT spec. It can
change length whilst we are reading it due to DSEMTS entry
counts being allowed to change.
https://uefi.org/sites/default/files/resources/Coherent%20Device%20Attribute%20Table_1.03.pdf
(it's in the description of the Sequence field)
Sure we'll notice that the checksum fails and the sequence number
has updated but that doesn't help us if we went out of bounds
before we knew that.
Definitely good to check this as I think we can hit it even
if we don't have a potentially buggy device.
I'd still like to moan if we get inconsistent sizes and it
isn't a race though. Can we find a clean way to detect this
at a point where we know we have a valid complete table?
Jonathan
> Jonathan
>
>
> > ---
> > 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 c3569d2ba503..16291814450e 100644
> > --- a/lib/fw_table.c
> > +++ b/lib/fw_table.c
> > @@ -127,6 +127,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
> > @@ -148,18 +149,21 @@ 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;
> > 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;
> if (max_length)
> table_len = min(max_length, table_len);
>
> > + table_end = (unsigned long)table_header + table_len;
> >
> > /* Parse all entries looking for a match. */
> >
> > @@ -208,7 +212,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,
> > @@ -222,6 +227,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 20:26:47 +0100
Robert Richter <[email protected]> wrote:
> There exists card implementations with a CDAT table using a fix
There exist ... fixed size buffer,
> buffer, but with entries filled in that do not fill the whole table
> length size. Then, 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 while 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, a check of the table buffer length is missing to prevent
> an out-of-bound access then parsing the CDAT table.
>
> Hardening code against device returning borked table. Fix that by
> providing an optional 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 to check the buffer
> length. This also prevents a possible out-of-bound access mentioned.
>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Len Brown <[email protected]>
> Signed-off-by: Robert Richter <[email protected]>
> Reviewed-by: Dave Jiang <[email protected]>
I think we should scream a bit about this if we see it
as I'm unconvinced the spec allows for an implementation like this.
If the spec is unclear, lets seek a clarification.
I'm fine with this as a defensive measure, I just don't want
device vendors to keep doing it!
Jonathan
> ---
> 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 c3569d2ba503..16291814450e 100644
> --- a/lib/fw_table.c
> +++ b/lib/fw_table.c
> @@ -127,6 +127,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
> @@ -148,18 +149,21 @@ 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;
> 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;
if (max_length)
table_len = min(max_length, table_len);
> + table_end = (unsigned long)table_header + table_len;
>
> /* Parse all entries looking for a match. */
>
> @@ -208,7 +212,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,
> @@ -222,6 +227,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,
thanks for your review.
On 14.02.24 17:31:58, Jonathan Cameron wrote:
> On Fri, 9 Feb 2024 20:26:46 +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. E.g., see fix of devm_kfree()
> > in read_cdat_data():
> >
> > c65efe3685f5 cxl/cdat: Free correct buffer on checksum error
> >
> > Rework code to avoid calculations with sizeof(__le32). Introduce
> > struct cdat_doe_rsp 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]>
>
> Hi Robert,
>
> I like this in general. A few comments inline though.
>
> > ---
> > drivers/cxl/core/pci.c | 75 ++++++++++++++++++++++--------------------
> > drivers/cxl/cxlpci.h | 20 +++++++++++
> > 2 files changed, 59 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index 39366ce94985..569354a5536f 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> > @@ -544,55 +544,55 @@ 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)
>
> Nitpick, but rsp isn't a response, it's the whole table.
> Maybe it's worth a
> #define cdat_doe_table cdat_doe_rsp
> or a typedef so the two are different in name at least whilst sharing
> same structure definition?
There is a comment near the kzalloc of buf. I think introducing
another type here for single use will just add confusion.
I will also update the description of cdat_doe_rsp.
>
> > {
> > - size_t length = *cdat_length + sizeof(__le32);
> > - __le32 *data = cdat_table;
> > - int entry_handle = 0;
> > + size_t received, remaining = *length;
> > + unsigned int entry_handle = 0;
> > + union cdat_data *data;
> > __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);
>
> I guess it's not really worth using struct_size here.
> It's main advantage is making it clear we are dealing with a
> trailing []
Yes, will keep it as is. Since it's a u8 array, count is equal the
size for the remaining data and we do not need struct_size() here.
>
> > 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 ((entry_handle == 0 &&
> > - rc != sizeof(__le32) + sizeof(struct cdat_header)) ||
> > - (entry_handle > 0 &&
> > - (rc < sizeof(__le32) + sizeof(*entry) ||
> > - rc != sizeof(__le32) + le16_to_cpu(entry->length))))
> > + if (rc < sizeof(*rsp))
> > + return -EIO;
> > +
> > + data = (void *)rsp->data;
>
> Nicer to cast to (union cdat_data *) than rely on bounce via a void *
Will change.
>
> > + received = rc - sizeof(*rsp);
> > +
> > + if ((!entry_handle &&
>
> Prefer == 0 for this because 0 is a magic value here.
>
> > + received != sizeof(data->header)) ||
> > + (entry_handle &&
> > + (received < sizeof(data->entry) ||
> > + received != le16_to_cpu(data->entry.length))))
> > return -EIO;
>
> Given it's two rather involved conditions maybe better to do.
>
> if (entry_handle == 0) {
> if (received != sizeof(data->header)
> return -EIO;
> } else {
> if (received < sizeof(data->entry) ||
> received != le16_to_cpu(data->entry.length))
> return -EIO;
> }
>
> More code but easier to see the header vs entry checks.
> Could even define a little utility function / macro.
>
> cdat_is_head_handle(val) entry_handle == 0
> so you get somewhat more self documenting code.
>
> if (cdat_is_head_handle(entry_handle)) {
> } else {
> }
I will take this but without the macro.
>
> >
> > /* 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;
>
> I'm not keen on this looking like we are writing the doe header
> as we are writing the tail of the last response.
>
> Maybe the comment is enough. I don't have a better idea on how
> to make this more obvious.
I think the comment is good enough here.
>
> > + remaining -= received;
> > + rsp = (void *)rsp + received;
>
> Was a potential problem with previous code, but this could
> in theory become unaligned and we should be using unaligned accessors
> for it as a result, or maybe adding a check that it doesn't ever become so.
> The check is probably the easier path given CDAT entries are thankfully
> (I think) all dword multiples as are the two headers.
Yes, buffers are dwords. In any case, pci_doe_recv_resp() is safe to
be used unaligned anyway.
Thanks for your review, will prepare a v4.
-Robert
>
> > + 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;
> > }
>
>
On 14.02.24 17:44:44, Jonathan Cameron wrote:
> On Wed, 14 Feb 2024 17:39:27 +0000
> Jonathan Cameron <[email protected]> wrote:
>
> > On Fri, 9 Feb 2024 20:26:47 +0100
> > Robert Richter <[email protected]> wrote:
> >
> > > There exists card implementations with a CDAT table using a fix
> > There exist ... fixed size buffer,
> > > buffer, but with entries filled in that do not fill the whole table
> > > length size. Then, 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 while 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, a check of the table buffer length is missing to prevent
> > > an out-of-bound access then parsing the CDAT table.
> > >
> > > Hardening code against device returning borked table. Fix that by
> > > providing an optional 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 to check the buffer
> > > length. This also prevents a possible out-of-bound access mentioned.
> > >
> > > Cc: "Rafael J. Wysocki" <[email protected]>
> > > Cc: Len Brown <[email protected]>
> > > Signed-off-by: Robert Richter <[email protected]>
> > > Reviewed-by: Dave Jiang <[email protected]>
> >
> > I think we should scream a bit about this if we see it
> > as I'm unconvinced the spec allows for an implementation like this.
> >
> > If the spec is unclear, lets seek a clarification.
> >
> > I'm fine with this as a defensive measure, I just don't want
> > device vendors to keep doing it!
> >
> Scrub that - I got around to checking the CDAT spec. It can
> change length whilst we are reading it due to DSEMTS entry
> counts being allowed to change.
> https://uefi.org/sites/default/files/resources/Coherent%20Device%20Attribute%20Table_1.03.pdf
> (it's in the description of the Sequence field)
>
> Sure we'll notice that the checksum fails and the sequence number
> has updated but that doesn't help us if we went out of bounds
> before we knew that.
>
> Definitely good to check this as I think we can hit it even
> if we don't have a potentially buggy device.
> I'd still like to moan if we get inconsistent sizes and it
> isn't a race though. Can we find a clean way to detect this
> at a point where we know we have a valid complete table?
I will add a warning about a length mismatch.
Thanks,
-Robert