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

[Francisco Iglesias]

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.mic...@amd.com>
Signed-off-by: Frederic Konrad <fkon...@amd.com>

Reviewed-by: Francisco Iglesias <francisco.igles...@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;