ZynqMP / Versal: various fixes

[PATCH v1 1/3] hw/ssi/xilinx_spips: fix an out of bound access

Posted by Frederic Konrad 1 year ago

The spips, qspips, and zynqmp-qspips share the same realize function
(xilinx_spips_realize) and initialize their io memory region with different
mmio_ops passed through the class.  The size of the memory region is set to
the largest area (0x200 bytes for zynqmp-qspips) thus it is possible to write
out of s->regs[addr] in xilinx_spips_write for spips and qspips.

This fixes that wrong behavior.

Reviewed-by: Luc Michel <luc.michel@amd.com>
Signed-off-by: Frederic Konrad <fkonrad@amd.com>
---
 hw/ssi/xilinx_spips.c         | 7 ++++++-
 include/hw/ssi/xilinx_spips.h | 3 +++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
index a3955c6c50..0bdfad7e2e 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -973,6 +973,8 @@ static void xilinx_spips_write(void *opaque, hwaddr addr,
 
     DB_PRINT_L(0, "addr=" HWADDR_FMT_plx " = %x\n", addr, (unsigned)value);
     addr >>= 2;
+    assert(addr < XLNX_SPIPS_R_MAX);
+
     switch (addr) {
     case R_CONFIG:
         mask = ~(R_CONFIG_RSVD | MAN_START_COM);
@@ -1299,7 +1301,7 @@ static void xilinx_spips_realize(DeviceState *dev, Error **errp)
     }
 
     memory_region_init_io(&s->iomem, OBJECT(s), xsc->reg_ops, s,
-                          "spi", XLNX_ZYNQMP_SPIPS_R_MAX * 4);
+                          "spi", xsc->reg_size);
     sysbus_init_mmio(sbd, &s->iomem);
 
     s->irqline = -1;
@@ -1435,6 +1437,7 @@ static void xilinx_qspips_class_init(ObjectClass *klass, void * data)
 
     dc->realize = xilinx_qspips_realize;
     xsc->reg_ops = &qspips_ops;
+    xsc->reg_size = XLNX_SPIPS_R_MAX * 4;
     xsc->rx_fifo_size = RXFF_A_Q;
     xsc->tx_fifo_size = TXFF_A_Q;
 }
@@ -1450,6 +1453,7 @@ static void xilinx_spips_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_xilinx_spips;
 
     xsc->reg_ops = &spips_ops;
+    xsc->reg_size = XLNX_SPIPS_R_MAX * 4;
     xsc->rx_fifo_size = RXFF_A;
     xsc->tx_fifo_size = TXFF_A;
 }
@@ -1464,6 +1468,7 @@ static void xlnx_zynqmp_qspips_class_init(ObjectClass *klass, void * data)
     dc->vmsd = &vmstate_xlnx_zynqmp_qspips;
     device_class_set_props(dc, xilinx_zynqmp_qspips_properties);
     xsc->reg_ops = &xlnx_zynqmp_qspips_ops;
+    xsc->reg_size = XLNX_ZYNQMP_SPIPS_R_MAX * 4;
     xsc->rx_fifo_size = RXFF_A_Q;
     xsc->tx_fifo_size = TXFF_A_Q;
 }
diff --git a/include/hw/ssi/xilinx_spips.h b/include/hw/ssi/xilinx_spips.h
index 1386d5ac8f..7a754bf67a 100644
--- a/include/hw/ssi/xilinx_spips.h
+++ b/include/hw/ssi/xilinx_spips.h
@@ -33,7 +33,9 @@
 
 typedef struct XilinxSPIPS XilinxSPIPS;
 
+/* For SPIPS, QSPIPS.  */
 #define XLNX_SPIPS_R_MAX        (0x100 / 4)
+/* For ZYNQMP_QSPIPS.  */
 #define XLNX_ZYNQMP_SPIPS_R_MAX (0x200 / 4)
 
 /* Bite off 4k chunks at a time */
@@ -125,6 +127,7 @@ struct XilinxSPIPSClass {
     SysBusDeviceClass parent_class;
 
     const MemoryRegionOps *reg_ops;
+    uint64_t reg_size;
 
     uint32_t rx_fifo_size;
     uint32_t tx_fifo_size;
-- 
2.25.1

Re: [PATCH v1 1/3] hw/ssi/xilinx_spips: fix an out of bound access

Posted by Alistair Francis 11 months, 3 weeks ago

On Sat, Nov 25, 2023 at 12:38 AM Frederic Konrad <fkonrad@amd.com> wrote:
>
> The spips, qspips, and zynqmp-qspips share the same realize function
> (xilinx_spips_realize) and initialize their io memory region with different
> mmio_ops passed through the class.  The size of the memory region is set to
> the largest area (0x200 bytes for zynqmp-qspips) thus it is possible to write
> out of s->regs[addr] in xilinx_spips_write for spips and qspips.
>
> This fixes that wrong behavior.
>
> Reviewed-by: Luc Michel <luc.michel@amd.com>
> Signed-off-by: Frederic Konrad <fkonrad@amd.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/ssi/xilinx_spips.c         | 7 ++++++-
>  include/hw/ssi/xilinx_spips.h | 3 +++
>  2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index a3955c6c50..0bdfad7e2e 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -973,6 +973,8 @@ static void xilinx_spips_write(void *opaque, hwaddr addr,
>
>      DB_PRINT_L(0, "addr=" HWADDR_FMT_plx " = %x\n", addr, (unsigned)value);
>      addr >>= 2;
> +    assert(addr < XLNX_SPIPS_R_MAX);
> +
>      switch (addr) {
>      case R_CONFIG:
>          mask = ~(R_CONFIG_RSVD | MAN_START_COM);
> @@ -1299,7 +1301,7 @@ static void xilinx_spips_realize(DeviceState *dev, Error **errp)
>      }
>
>      memory_region_init_io(&s->iomem, OBJECT(s), xsc->reg_ops, s,
> -                          "spi", XLNX_ZYNQMP_SPIPS_R_MAX * 4);
> +                          "spi", xsc->reg_size);
>      sysbus_init_mmio(sbd, &s->iomem);
>
>      s->irqline = -1;
> @@ -1435,6 +1437,7 @@ static void xilinx_qspips_class_init(ObjectClass *klass, void * data)
>
>      dc->realize = xilinx_qspips_realize;
>      xsc->reg_ops = &qspips_ops;
> +    xsc->reg_size = XLNX_SPIPS_R_MAX * 4;
>      xsc->rx_fifo_size = RXFF_A_Q;
>      xsc->tx_fifo_size = TXFF_A_Q;
>  }
> @@ -1450,6 +1453,7 @@ static void xilinx_spips_class_init(ObjectClass *klass, void *data)
>      dc->vmsd = &vmstate_xilinx_spips;
>
>      xsc->reg_ops = &spips_ops;
> +    xsc->reg_size = XLNX_SPIPS_R_MAX * 4;
>      xsc->rx_fifo_size = RXFF_A;
>      xsc->tx_fifo_size = TXFF_A;
>  }
> @@ -1464,6 +1468,7 @@ static void xlnx_zynqmp_qspips_class_init(ObjectClass *klass, void * data)
>      dc->vmsd = &vmstate_xlnx_zynqmp_qspips;
>      device_class_set_props(dc, xilinx_zynqmp_qspips_properties);
>      xsc->reg_ops = &xlnx_zynqmp_qspips_ops;
> +    xsc->reg_size = XLNX_ZYNQMP_SPIPS_R_MAX * 4;
>      xsc->rx_fifo_size = RXFF_A_Q;
>      xsc->tx_fifo_size = TXFF_A_Q;
>  }
> diff --git a/include/hw/ssi/xilinx_spips.h b/include/hw/ssi/xilinx_spips.h
> index 1386d5ac8f..7a754bf67a 100644
> --- a/include/hw/ssi/xilinx_spips.h
> +++ b/include/hw/ssi/xilinx_spips.h
> @@ -33,7 +33,9 @@
>
>  typedef struct XilinxSPIPS XilinxSPIPS;
>
> +/* For SPIPS, QSPIPS.  */
>  #define XLNX_SPIPS_R_MAX        (0x100 / 4)
> +/* For ZYNQMP_QSPIPS.  */
>  #define XLNX_ZYNQMP_SPIPS_R_MAX (0x200 / 4)
>
>  /* Bite off 4k chunks at a time */
> @@ -125,6 +127,7 @@ struct XilinxSPIPSClass {
>      SysBusDeviceClass parent_class;
>
>      const MemoryRegionOps *reg_ops;
> +    uint64_t reg_size;
>
>      uint32_t rx_fifo_size;
>      uint32_t tx_fifo_size;
> --
> 2.25.1
>
>

Re: [PATCH v1 1/3] hw/ssi/xilinx_spips: fix an out of bound access

Posted by Francisco Iglesias 1 year ago


On 2023-11-24 15:35, Frederic Konrad wrote:
> The spips, qspips, and zynqmp-qspips share the same realize function
> (xilinx_spips_realize) and initialize their io memory region with different
> mmio_ops passed through the class.  The size of the memory region is set to
> the largest area (0x200 bytes for zynqmp-qspips) thus it is possible to write
> out of s->regs[addr] in xilinx_spips_write for spips and qspips.
> 
> This fixes that wrong behavior.
> 
> Reviewed-by: Luc Michel <luc.michel@amd.com>
> Signed-off-by: Frederic Konrad <fkonrad@amd.com>

Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>


> ---
>   hw/ssi/xilinx_spips.c         | 7 ++++++-
>   include/hw/ssi/xilinx_spips.h | 3 +++
>   2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index a3955c6c50..0bdfad7e2e 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -973,6 +973,8 @@ static void xilinx_spips_write(void *opaque, hwaddr addr,
>   
>       DB_PRINT_L(0, "addr=" HWADDR_FMT_plx " = %x\n", addr, (unsigned)value);
>       addr >>= 2;
> +    assert(addr < XLNX_SPIPS_R_MAX);
> +
>       switch (addr) {
>       case R_CONFIG:
>           mask = ~(R_CONFIG_RSVD | MAN_START_COM);
> @@ -1299,7 +1301,7 @@ static void xilinx_spips_realize(DeviceState *dev, Error **errp)
>       }
>   
>       memory_region_init_io(&s->iomem, OBJECT(s), xsc->reg_ops, s,
> -                          "spi", XLNX_ZYNQMP_SPIPS_R_MAX * 4);
> +                          "spi", xsc->reg_size);
>       sysbus_init_mmio(sbd, &s->iomem);
>   
>       s->irqline = -1;
> @@ -1435,6 +1437,7 @@ static void xilinx_qspips_class_init(ObjectClass *klass, void * data)
>   
>       dc->realize = xilinx_qspips_realize;
>       xsc->reg_ops = &qspips_ops;
> +    xsc->reg_size = XLNX_SPIPS_R_MAX * 4;
>       xsc->rx_fifo_size = RXFF_A_Q;
>       xsc->tx_fifo_size = TXFF_A_Q;
>   }
> @@ -1450,6 +1453,7 @@ static void xilinx_spips_class_init(ObjectClass *klass, void *data)
>       dc->vmsd = &vmstate_xilinx_spips;
>   
>       xsc->reg_ops = &spips_ops;
> +    xsc->reg_size = XLNX_SPIPS_R_MAX * 4;
>       xsc->rx_fifo_size = RXFF_A;
>       xsc->tx_fifo_size = TXFF_A;
>   }
> @@ -1464,6 +1468,7 @@ static void xlnx_zynqmp_qspips_class_init(ObjectClass *klass, void * data)
>       dc->vmsd = &vmstate_xlnx_zynqmp_qspips;
>       device_class_set_props(dc, xilinx_zynqmp_qspips_properties);
>       xsc->reg_ops = &xlnx_zynqmp_qspips_ops;
> +    xsc->reg_size = XLNX_ZYNQMP_SPIPS_R_MAX * 4;
>       xsc->rx_fifo_size = RXFF_A_Q;
>       xsc->tx_fifo_size = TXFF_A_Q;
>   }
> diff --git a/include/hw/ssi/xilinx_spips.h b/include/hw/ssi/xilinx_spips.h
> index 1386d5ac8f..7a754bf67a 100644
> --- a/include/hw/ssi/xilinx_spips.h
> +++ b/include/hw/ssi/xilinx_spips.h
> @@ -33,7 +33,9 @@
>   
>   typedef struct XilinxSPIPS XilinxSPIPS;
>   
> +/* For SPIPS, QSPIPS.  */
>   #define XLNX_SPIPS_R_MAX        (0x100 / 4)
> +/* For ZYNQMP_QSPIPS.  */
>   #define XLNX_ZYNQMP_SPIPS_R_MAX (0x200 / 4)
>   
>   /* Bite off 4k chunks at a time */
> @@ -125,6 +127,7 @@ struct XilinxSPIPSClass {
>       SysBusDeviceClass parent_class;
>   
>       const MemoryRegionOps *reg_ops;
> +    uint64_t reg_size;
>   
>       uint32_t rx_fifo_size;
>       uint32_t tx_fifo_size;

[PATCH v1 1/3] hw/ssi/xilinx_spips: fix an out of bound access
[PATCH v1 2/3] fix some url for amd / xilinx models
[PATCH v1 3/3] hw/dma/xlnx_csu_dma: don't throw guest errors when stopping the SRC DMA