Re: [PATCH v1 1/3] hw/ssi/xilinx_spips: fix an out of bound access
On Sat, Nov 25, 2023 at 12:38 AM 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
> Signed-off-by: Frederic Konrad
Reviewed-by: Alistair Francis
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(>iomem, OBJECT(s), xsc->reg_ops, s,
> - "spi", XLNX_ZYNQMP_SPIPS_R_MAX * 4);
> + "spi", xsc->reg_size);
> sysbus_init_mmio(sbd, >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 = _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 = _xilinx_spips;
>
> xsc->reg_ops = _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 = _xlnx_zynqmp_qspips;
> device_class_set_props(dc, xilinx_zynqmp_qspips_properties);
> xsc->reg_ops = _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
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
Signed-off-by: Frederic Konrad
Reviewed-by: Francisco Iglesias
---
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(>iomem, OBJECT(s), xsc->reg_ops, s,
- "spi", XLNX_ZYNQMP_SPIPS_R_MAX * 4);
+ "spi", xsc->reg_size);
sysbus_init_mmio(sbd, >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 = _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 = _xilinx_spips;
xsc->reg_ops = _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 = _xlnx_zynqmp_qspips;
device_class_set_props(dc, xilinx_zynqmp_qspips_properties);
xsc->reg_ops = _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
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
Signed-off-by: Frederic Konrad
---
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(>iomem, OBJECT(s), xsc->reg_ops, s,
- "spi", XLNX_ZYNQMP_SPIPS_R_MAX * 4);
+ "spi", xsc->reg_size);
sysbus_init_mmio(sbd, >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 = _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 = _xilinx_spips;
xsc->reg_ops = _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 = _xlnx_zynqmp_qspips;
device_class_set_props(dc, xilinx_zynqmp_qspips_properties);
xsc->reg_ops = _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
