Re: [PATCH v3 03/16] cxl/pci: Handle truncated CDAT entries

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Fri, 10 Feb 2023 21:25:03 +0100
Lukas Wunner <lukas@wunner.de> 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@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org # v6.0+
One trivial suggestion.

Glad to see this tightened up.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  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);