hw/cxl: Fix register block locator size

[PATCH qemu] hw/cxl: Fix register block locator size

Posted by Jonathan Cameron via 8 months, 1 week ago

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 <Jonathan.Cameron@huawei.com>
---
 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

Re: [PATCH qemu] hw/cxl: Fix register block locator size

Posted by Jonathan Cameron via 8 months, 1 week ago

On Thu, 29 May 2025 14:48:28 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> 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 <Jonathan.Cameron@huawei.com>
> ---
>  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

Posted by Zhijian Li (Fujitsu) via 8 months, 1 week ago


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 <Jonathan.Cameron@huawei.com>
> ---
>   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,
  57                                REG_LOC_DVSEC_LENGTH, REG_LOC_DVSEC,
  58                                REG_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

Posted by Jonathan Cameron via 8 months, 1 week ago

On Fri, 30 May 2025 02:59:40 +0000
"Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com> 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 <Jonathan.Cameron@huawei.com>
> > ---
> >   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,
>   57                                REG_LOC_DVSEC_LENGTH, REG_LOC_DVSEC,
>   58                                REG_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

Posted by Fan Ni 8 months, 1 week ago

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 <Jonathan.Cameron@huawei.com>
> ---

Reviewed-by: Fan Ni <fan.ni@samsung.com>

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