| 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
| ||
|
Message-ID: <20240214173158.000005c0@Huawei.com>
Date: Wed, 14 Feb 2024 17:31:58 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Robert Richter <rrichter@....com>
CC: Alison Schofield <alison.schofield@...el.com>, Vishal Verma
<vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>, Dan Williams
<dan.j.williams@...el.com>, Dave Jiang <dave.jiang@...el.com>, "Davidlohr
Bueso" <dave@...olabs.net>, <linux-cxl@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, "Rafael J. Wysocki" <rafael@...nel.org>, Len
Brown <lenb@...nel.org>, Lukas Wunner <lukas@...ner.de>, Fan Ni
<nifan.cxl@...il.com>
Subject: Re: [PATCH v3 2/3] cxl/pci: Get rid of pointer arithmetic reading
CDAT table
On Fri, 9 Feb 2024 20:26:46 +0100
Robert Richter <rrichter@....com> 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 <lukas@...ner.de>
> Cc: Fan Ni <nifan.cxl@...il.com>
> Reviewed-by: Dave Jiang <dave.jiang@...el.com>
> Signed-off-by: Robert Richter <rrichter@....com>
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?
> {
> - 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 []
> 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 *
> + 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 {
}
>
> /* 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.
> + 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.
> + 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;
> }
Powered by blists - more mailing lists