| Message ID | 5b4e23f256b3705360d84eccb9652e4b558a77b5.1676043318.git.lukas@wunner.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Collection of DOE material | expand |
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@intel.com> > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Cc: stable@vger.kernel.org # 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@intel.com>
On Fri, Feb 10, 2023 at 04:50:38PM -0800, Dan Williams wrote: > 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. [...] > > @@ -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. The check in the previous patch ("cxl/pci: Handle truncated CDAT header") fixes response size verification for cxl_cdat_get_length(). The check here however is in a different function, cxl_cdat_read_table(). Previously the function only checked whether the DOE response contained two dwords. That's one dword for the Table Access Response header (CXL r3.0 sec 8.1.11.1) plus one dword for the common header shared by all CDAT structures. What can happen is we receive a truncated entry with, say, just two dwords, and we copy that to the cached CDAT exposed in sysfs. Obviously that's a corrupt CDAT. The consumer of the CDAT doesn't know that the data was truncated and will try to parse the entry even though the data may actually be a portion of the next entry. The improved verification here ensures that the received amount of data corresponds to the length in the entry header. It is up to the parsing function of each entry to verify whether that length is correct for that specific entry. E.g. a DSMAS entry should contain 24 bytes, so the parsing function needs to verify that the length in the entry header is actually 24. (See the e-mail I sent to Dave a few minutes ago.) Thanks, Lukas > > + /* 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;
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);
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) || + 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);
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+ --- 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(-)