| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 17 Nov 2023 13:25:06 -0700
From: Dave Jiang <dave.jiang@...el.com>
To: Robert Richter <rrichter@....com>,
Dan Williams <dan.j.williams@...el.com>
CC: Ira Weiny <ira.weiny@...el.com>,
Davidlohr Bueso <dave@...olabs.net>,
Jonathan Cameron <jonathan.cameron@...wei.com>,
Alison Schofield <alison.schofield@...el.com>,
Vishal Verma <vishal.l.verma@...el.com>,
<linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
Fan Ni <nifan.cxl@...il.com>, Lukas Wunner <lukas@...ner.de>
Subject: Re: [PATCH] cxl/pci: Get rid of pointer arithmetic reading CDAT table
On 11/17/23 13:15, Robert Richter wrote:
> On 17.11.23 21:09:18, Robert Richter wrote:
>> I will send an on-top patch for 6.8 that reworks that code area to
>> remove the pointer arithmetic.
>
> Here it is:
>
> From 13787f72c20b8c54754ae86015d982307eae0397 Mon Sep 17 00:00:00 2001
> From: Robert Richter <rrichter@....com>
> Subject: [PATCH] cxl/pci: Get rid of pointer arithmetic reading CDAT table
>
> 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 <lukas@...ner.de>
> Cc: Dave Jiang <dave.jiang@...el.com>
> Cc: Fan Ni <nifan.cxl@...il.com>
> Signed-off-by: Robert Richter <rrichter@....com>
Reviewed-by: Dave Jiang <dave.jiang@...el.com>
> ---
> drivers/cxl/core/pci.c | 80 ++++++++++++++++++++----------------------
> drivers/cxl/cxlpci.h | 19 ++++++++++
> 2 files changed, 57 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 5aaa0b36c42a..f900740c6dea 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -517,14 +517,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));
> @@ -542,56 +542,54 @@ static int cxl_cdat_get_length(struct device *dev,
> }
>
> static int cxl_cdat_read_table(struct device *dev,
> - struct pci_doe_mb *cdat_doe,
> - void *cdat_table, size_t *cdat_length)
> + struct pci_doe_mb *doe_mb,
> + struct cdat_doe *doe, 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(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);
> + doe, sizeof(*doe) + 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(*doe))
> + return -EIO;
> +
> + received = rc - sizeof(*doe);
> +
> if ((entry_handle == 0 &&
> - rc != sizeof(__le32) + sizeof(struct cdat_header)) ||
> + received != sizeof(doe->header[0])) ||
> (entry_handle > 0 &&
> - (rc < sizeof(__le32) + sizeof(*entry) ||
> - rc != sizeof(__le32) + le16_to_cpu(entry->length))))
> + (received < sizeof(doe->entry[0]) ||
> + received != le16_to_cpu(doe->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(doe->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;
> + doe->doe_header = saved_dw;
> + remaining -= received;
> + doe = (void *)doe + received;
> + saved_dw = doe->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;
> }
> @@ -616,11 +614,11 @@ 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;
> - void *cdat_table, *cdat_buf;
> + struct cdat_doe *doe;
> + size_t length;
> int rc;
>
> if (is_cxl_memdev(uport)) {
> @@ -637,40 +635,38 @@ 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, &length)) {
> dev_dbg(dev, "No CDAT length\n");
> return;
> }
>
> - cdat_buf = devm_kzalloc(dev, cdat_length + sizeof(__le32),
> - GFP_KERNEL);
> - if (!cdat_buf)
> - return;
> + doe = devm_kzalloc(dev, sizeof(*doe) + length, GFP_KERNEL);
> + if (!doe)
> + goto err;
>
> - rc = cxl_cdat_read_table(dev, cdat_doe, cdat_buf, &cdat_length);
> + rc = cxl_cdat_read_table(dev, doe_mb, doe, &length);
> if (rc)
> goto err;
>
> - cdat_table = cdat_buf + sizeof(__le32);
> - if (cdat_checksum(cdat_table, cdat_length))
> + if (cdat_checksum(doe->table, length))
> goto err;
>
> - port->cdat.table = cdat_table;
> - port->cdat.length = cdat_length;
> - return;
> + port->cdat.table = doe->table;
> + port->cdat.length = length;
>
> + return;
> err:
> /* Don't leave table data allocated on error */
> - devm_kfree(dev, cdat_buf);
> + devm_kfree(dev, doe);
> 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 0fa4799ea316..d12ed9d8dec1 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 {
> + __le32 doe_header;
> + union {
> + u8 table[0];
> + struct cdat_header header[0];
> + struct cdat_entry_header entry[0];
> + };
> +} __packed;
> +
> int devm_cxl_port_enumerate_dports(struct cxl_port *port);
> struct cxl_dev_state;
> int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
Powered by blists - more mailing lists