On Fri, 10 Feb 2023 21:25:03 +0100
Lukas Wunner <lukas@xxxxxxxxx> wrote:
> If truncated CDAT entries are received from a device, the concatenation
> of those entries constitutes a corrupt CDAT, yet is happily exposed to
> user space.
>
> Avoid by verifying response lengths and erroring out if truncation is
> detected.
>
> The last CDAT entry may still be truncated despite the checks introduced
> herein if the length in the CDAT header is too small. However, that is
> easily detectable by user space because it reaches EOF prematurely.
> A subsequent commit which rightsizes the CDAT response allocation closes
> that remaining loophole.
>
> The two lines introduced here which exceed 80 chars are shortened to
> less than 80 chars by a subsequent commit which migrates to a
> synchronous DOE API and replaces "t.task.rv" by "rc".
>
> The existing acpi_cdat_header and acpi_table_cdat struct definitions
> provided by ACPICA cannot be used because they do not employ __le16 or
> __le32 types. I believe that cannot be changed because those types are
> Linux-specific and ACPI is specified for little endian platforms only,
> hence doesn't care about endianness. So duplicate the structs.
>
> Fixes: c97006046c79 ("cxl/port: Read CDAT table")
> Tested-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx # v6.0+
One trivial suggestion.
Glad to see this tightened up.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> ---
> Changes v2 -> v3:
> * Newly added patch in v3
>
> drivers/cxl/core/pci.c | 13 +++++++++----
> drivers/cxl/cxlpci.h | 14 ++++++++++++++
> 2 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 11a85b3a9a0b..a3fb6bd68d17 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -547,8 +547,8 @@ static int cxl_cdat_read_table(struct device *dev,
>
> do {
> DECLARE_CDAT_DOE_TASK(CDAT_DOE_REQ(entry_handle), t);
> + struct cdat_entry_header *entry;
> size_t entry_dw;
> - __le32 *entry;
> int rc;
>
> rc = pci_doe_submit_task(cdat_doe, &t.task);
> @@ -557,14 +557,19 @@ static int cxl_cdat_read_table(struct device *dev,
> return rc;
> }
> wait_for_completion(&t.c);
> - /* 1 DW header + 1 DW data min */
> - if (t.task.rv < (2 * sizeof(u32)))
> +
> + /* 1 DW Table Access Response Header + CDAT entry */
> + entry = (struct cdat_entry_header *)(t.response_pl + 1);
> + if ((entry_handle == 0 &&
> + t.task.rv != sizeof(u32) + sizeof(struct cdat_header)) ||
> + (entry_handle > 0 &&
> + (t.task.rv < sizeof(u32) + sizeof(struct cdat_entry_header) ||
Slight preference for sizeof(*entry)
so that it is clear that we are checking we can do the next check without
getting garbage.
> + t.task.rv != sizeof(u32) + le16_to_cpu(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(t.response_pl[0]));
> - entry = t.response_pl + 1;
> entry_dw = t.task.rv / sizeof(u32);
> /* Skip Header */
> entry_dw -= 1;
> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> index 920909791bb9..104ad2b72516 100644
> --- a/drivers/cxl/cxlpci.h
> +++ b/drivers/cxl/cxlpci.h
> @@ -62,6 +62,20 @@ enum cxl_regloc_type {
> CXL_REGLOC_RBI_TYPES
> };
>
> +struct cdat_header {
> + __le32 length;
> + u8 revision;
> + u8 checksum;
> + u8 reserved[6];
> + __le32 sequence;
> +} __packed;
> +
> +struct cdat_entry_header {
> + u8 type;
> + u8 reserved;
> + __le16 length;
> +} __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);
