Lukas Wunner 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+
> ---
> 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)))
Ah, I guess that's why the previous check can not be pushed down
further, its obviated by this more comprehensive check.
> +
> + /* 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) ||
> + t.task.rv != sizeof(u32) + le16_to_cpu(entry->length))))
> return -EIO;
Looks correct for catching that the response is large enough to
communicate the length and that the expected length was retrieved.
Reviewed-by: Dan Williams <dan.j.williams@xxxxxxxxx>
