Re: [PATCH qemu] hw/cxl: Fix register block locator size
On Fri, 30 May 2025 02:59:40 +
"Zhijian Li (Fujitsu)" wrote:
> On 29/05/2025 21:48, Jonathan Cameron via wrote:
> > This has been wrong from day 1. For now we only have
> > two entries (component and device registers).
>
> Wow, I finally understood this.
>
>
> >
> > The wrong size could lead to arbitrary data off the stack being presented
> > in PCIe config space.
> >
> > Signed-off-by: Jonathan Cameron
> > ---
> > include/hw/cxl/cxl_pci.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/hw/cxl/cxl_pci.h b/include/hw/cxl/cxl_pci.h
> > index d0855ed78b..3bb882ce89 100644
> > --- a/include/hw/cxl/cxl_pci.h
> > +++ b/include/hw/cxl/cxl_pci.h
> > @@ -31,7 +31,7 @@
> > #define PCIE_CXL3_FLEXBUS_PORT_DVSEC_LENGTH 0x20
> > #define PCIE_CXL3_FLEXBUS_PORT_DVSEC_REVID 2
> >
> > -#define REG_LOC_DVSEC_LENGTH 0x24
> > +#define REG_LOC_DVSEC_LENGTH 0x1C
>
> IMHO, REG_LOC_DVSEC_LENGTH is device specific, that mean we shouldn't put it
> in
> a general header with a general name
>
> try:
> $ git grep REG_LOC_DVSEC_LENGTH
>
> we got another REG_LOC_DVSEC_LENGTH, shouldn't its value (0x1C - 0x8)?
>
>
> 51 regloc_dvsec = &(CXLDVSECRegisterLocator) {
> 52 .rsvd = 0,
> 53 .reg0_base_lo = RBI_CXL_DEVICE_REG | 0,
> 54 .reg0_base_hi = 0,
> 55 };
> 56 cxl_component_create_dvsec(cxl_cstate, CXL3_SWITCH_MAILBOX_CCI,
> 57REG_LOC_DVSEC_LENGTH, REG_LOC_DVSEC,
> 58REG_LOC_DVSEC_REVID, (uint8_t
> *)regloc_dvsec);
>
Ah. This isn't a bug at all. I clearly needed more caffeine.
We are fine because at least in 3.2 the register block identifier of 0 is
reserved and
I misread the code completely. It is odd to have empty entries but not a bug.
Jonathan
>
> Thanks
> Zhijian
>
>
>
> > #define REG_LOC_DVSEC_REVID 0
> >
> > enum
Re: [PATCH qemu] hw/cxl: Fix register block locator size
On Thu, 29 May 2025 14:48:28 +0100
Jonathan Cameron wrote:
> This has been wrong from day 1. For now we only have
> two entries (component and device registers).
>
> The wrong size could lead to arbitrary data off the stack being presented
> in PCIe config space.
As noted in reply to Zhijian, this whole patch is garbage.
A fixed 'larger' size is fine as it will be 0 filled and that
is valid under the spec.
Sorry for the noise.
Jonathan
>
> Signed-off-by: Jonathan Cameron
> ---
> include/hw/cxl/cxl_pci.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/hw/cxl/cxl_pci.h b/include/hw/cxl/cxl_pci.h
> index d0855ed78b..3bb882ce89 100644
> --- a/include/hw/cxl/cxl_pci.h
> +++ b/include/hw/cxl/cxl_pci.h
> @@ -31,7 +31,7 @@
> #define PCIE_CXL3_FLEXBUS_PORT_DVSEC_LENGTH 0x20
> #define PCIE_CXL3_FLEXBUS_PORT_DVSEC_REVID 2
>
> -#define REG_LOC_DVSEC_LENGTH 0x24
> +#define REG_LOC_DVSEC_LENGTH 0x1C
> #define REG_LOC_DVSEC_REVID 0
>
> enum {
Re: [PATCH qemu] hw/cxl: Fix register block locator size
On 29/05/2025 21:48, Jonathan Cameron via wrote:
> This has been wrong from day 1. For now we only have
> two entries (component and device registers).
Wow, I finally understood this.
>
> The wrong size could lead to arbitrary data off the stack being presented
> in PCIe config space.
>
> Signed-off-by: Jonathan Cameron
> ---
> include/hw/cxl/cxl_pci.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/hw/cxl/cxl_pci.h b/include/hw/cxl/cxl_pci.h
> index d0855ed78b..3bb882ce89 100644
> --- a/include/hw/cxl/cxl_pci.h
> +++ b/include/hw/cxl/cxl_pci.h
> @@ -31,7 +31,7 @@
> #define PCIE_CXL3_FLEXBUS_PORT_DVSEC_LENGTH 0x20
> #define PCIE_CXL3_FLEXBUS_PORT_DVSEC_REVID 2
>
> -#define REG_LOC_DVSEC_LENGTH 0x24
> +#define REG_LOC_DVSEC_LENGTH 0x1C
IMHO, REG_LOC_DVSEC_LENGTH is device specific, that mean we shouldn't put it in
a general header with a general name
try:
$ git grep REG_LOC_DVSEC_LENGTH
we got another REG_LOC_DVSEC_LENGTH, shouldn't its value (0x1C - 0x8)?
51 regloc_dvsec = &(CXLDVSECRegisterLocator) {
52 .rsvd = 0,
53 .reg0_base_lo = RBI_CXL_DEVICE_REG | 0,
54 .reg0_base_hi = 0,
55 };
56 cxl_component_create_dvsec(cxl_cstate, CXL3_SWITCH_MAILBOX_CCI,
57REG_LOC_DVSEC_LENGTH, REG_LOC_DVSEC,
58REG_LOC_DVSEC_REVID, (uint8_t
*)regloc_dvsec);
Thanks
Zhijian
> #define REG_LOC_DVSEC_REVID 0
>
> enum {
Re: [PATCH qemu] hw/cxl: Fix register block locator size
On Thu, May 29, 2025 at 02:48:28PM +0100, Jonathan Cameron wrote:
> This has been wrong from day 1. For now we only have
> two entries (component and device registers).
>
> The wrong size could lead to arbitrary data off the stack being presented
> in PCIe config space.
>
> Signed-off-by: Jonathan Cameron
> ---
Reviewed-by: Fan Ni
> include/hw/cxl/cxl_pci.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/hw/cxl/cxl_pci.h b/include/hw/cxl/cxl_pci.h
> index d0855ed78b..3bb882ce89 100644
> --- a/include/hw/cxl/cxl_pci.h
> +++ b/include/hw/cxl/cxl_pci.h
> @@ -31,7 +31,7 @@
> #define PCIE_CXL3_FLEXBUS_PORT_DVSEC_LENGTH 0x20
> #define PCIE_CXL3_FLEXBUS_PORT_DVSEC_REVID 2
>
> -#define REG_LOC_DVSEC_LENGTH 0x24
> +#define REG_LOC_DVSEC_LENGTH 0x1C
> #define REG_LOC_DVSEC_REVID 0
>
> enum {
> --
> 2.48.1
>
--
Fan Ni (From gmail)
