Re: [PULL 53/60] hw/cxl: Standardize all references on CXL r3.1 and minor updates

[Peter Maydell]

On Wed, 14 Feb 2024 at 11:16, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Previously not all references mentioned any spec version at all.
> Given r3.1 is the current specification available for evaluation at
> www.computeexpresslink.org update references to refer to that.
> Hopefully this won't become a never ending job.
>
> A few structure definitions have been updated to add new fields.
> Defaults of 0 and read only are valid choices for these new DVSEC
> registers so go with that for now.
>
> There are additional error codes and some of the 'questions' in
> the comments are resolved now.
>
> Update documentation reference to point to the CXL r3.1 specification
> with naming closer to what is on the cover.
>
> For cases where there are structure version numbers, add defines
> so they can be found next to the register definitions.

Hi; Coverity points out that this change has introduced a
buffer overrun (CID 1534905). In hw/mem/cxl_type3.c:build_dvsecs()
we create a local struct of type CXLDVSecDevice, and then we
pass it to cxl_component_create_dvsec() as the body parameter,
passing it a length argument PCIE_CXL_DEVICE_DVSEC_LENGTH.

Before this change, both sizeof(CXLDVSecDevice) and
PCIE_CXL_DEVICE_DVSEC_LENGTH were 0x38, so this was fine.
But now...

> diff --git a/include/hw/cxl/cxl_pci.h b/include/hw/cxl/cxl_pci.h
> index ddf01a543b..265db6c407 100644
> --- a/include/hw/cxl/cxl_pci.h
> +++ b/include/hw/cxl/cxl_pci.h
> @@ -16,9 +16,8 @@
>  #define PCIE_DVSEC_HEADER1_OFFSET 0x4 /* Offset from start of extend cap */
>  #define PCIE_DVSEC_ID_OFFSET 0x8
>
> -#define PCIE_CXL_DEVICE_DVSEC_LENGTH 0x38
> -#define PCIE_CXL1_DEVICE_DVSEC_REVID 0
> -#define PCIE_CXL2_DEVICE_DVSEC_REVID 1
> +#define PCIE_CXL_DEVICE_DVSEC_LENGTH 0x3C
> +#define PCIE_CXL31_DEVICE_DVSEC_REVID 3
>
>  #define EXTENSIONS_PORT_DVSEC_LENGTH 0x28
>  #define EXTENSIONS_PORT_DVSEC_REVID 0

...PCIE_CXL_DEVICE_DVSEC_LENGTH is 0x3C...

> -/* CXL 2.0 - 8.1.3 (ID 0001) */
> +/*
> + * CXL r3.1 Section 8.1.3: PCIe DVSEC for Devices
> + * DVSEC ID: 0, Revision: 3
> + */
>  typedef struct CXLDVSECDevice {
>      DVSECHeader hdr;
>      uint16_t cap;
> @@ -82,10 +91,14 @@ typedef struct CXLDVSECDevice {
>      uint32_t range2_size_lo;
>      uint32_t range2_base_hi;
>      uint32_t range2_base_lo;
> -} CXLDVSECDevice;
> -QEMU_BUILD_BUG_ON(sizeof(CXLDVSECDevice) != 0x38);
> +    uint16_t cap3;
> +} QEMU_PACKED CXLDVSECDevice;
> +QEMU_BUILD_BUG_ON(sizeof(CXLDVSECDevice) != 0x3A);

...and CXLDVSECDevice is only size 0x3A, so we try to read off the
end of the struct.

What was supposed to happen here?

> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -319,7 +319,7 @@ static void build_dvsecs(CXLType3Dev *ct3d)
>      cxl_component_create_dvsec(cxl_cstate, CXL2_TYPE3_DEVICE,
>                                 PCIE_CXL_DEVICE_DVSEC_LENGTH,
>                                 PCIE_CXL_DEVICE_DVSEC,
> -                               PCIE_CXL2_DEVICE_DVSEC_REVID, dvsec);
> +                               PCIE_CXL31_DEVICE_DVSEC_REVID, dvsec);
>
>      dvsec = (uint8_t *)&(CXLDVSECRegisterLocator){
>          .rsvd         = 0,

Perhaps this call to cxl_component_create_dvsec() was
supposed to have the length argument changed, as seems
to have been done with this other call:

> @@ -346,9 +346,9 @@ static void build_dvsecs(CXLType3Dev *ct3d)
>          .rcvd_mod_ts_data_phase1 = 0xef, /* WTF? */
>      };
>      cxl_component_create_dvsec(cxl_cstate, CXL2_TYPE3_DEVICE,
> -                               PCIE_FLEXBUS_PORT_DVSEC_LENGTH_2_0,
> +                               PCIE_CXL3_FLEXBUS_PORT_DVSEC_LENGTH,
>                                 PCIE_FLEXBUS_PORT_DVSEC,
> -                               PCIE_FLEXBUS_PORT_DVSEC_REVID_2_0, dvsec);
> +                               PCIE_CXL3_FLEXBUS_PORT_DVSEC_REVID, dvsec);
>  }

 static void hdm_decoder_commit(CXLType3Dev *ct3d, int which)


and with similar other calls in the commit ?

Is there a way we could write this that would catch this error?
I'm thinking maybe something like

#define CXL_CREATE_DVSEC(CXL, DEVTYPE, TYPE, DATA) do { \
     assert(sizeof(*DATA) == TYPE##_LENGTH); \
     cxl_component_create_dvsec(CXL, DEVTYPE, TYPE##_LENGTH, \
                                TYPE, TYPE##_REVID, (uint8_t*)DATA); \
     } while (0)

thanks
-- PMM