1. Patchew
  2. QEMU
  3. [PATCH v1 00/10] Support ASPEED AST2700 A2
  4. View patch

[PATCH v1 01/10] hw/i2c/aspeed_i2c: Fix Out-of-Bounds access by using dynamic register array

Jamin Lin posted 10 patches 3 days, 6 hours ago
Maintainers: "Cédric Le Goater" <clg@kaod.org>, Peter Maydell <peter.maydell@linaro.org>, Steven Lee <steven_lee@aspeedtech.com>, Troy Lee <leetroy@gmail.com>, Jamin Lin <jamin_lin@aspeedtech.com>, Andrew Jeffery <andrew@codeconstruct.com.au>, Joel Stanley <joel@jms.id.au>, Fabiano Rosas <farosas@suse.de>, Laurent Vivier <lvivier@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
[PATCH v1 01/10] hw/i2c/aspeed_i2c: Fix Out-of-Bounds access by using dynamic register array
Posted by Jamin Lin 3 days, 6 hours ago 
The ASPEED I2C controller emulation used a fixed-size register array
(28 dwords) for all SoC variants, while multiple ASPEED SoCs
(AST2600, AST1030, AST2700) expose a larger MMIO register window
(e.g. reg_size = 0x80).

This mismatch allows MMIO accesses beyond the allocated register
array, leading to out-of-bounds reads in the I2C controller model.

Fix this by converting the register storage to a dynamically allocated
array sized according to the controller class reg_size. The register
array is now allocated during bus realize and free on unrealize,
ensuring safe access across different ASPEED SoC implementations.

This change eliminates I2C register out-of-bounds access caused by
SoC-specific register size differences.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3290
---
 include/hw/i2c/aspeed_i2c.h |  4 +---
 hw/i2c/aspeed_i2c.c         | 18 ++++++++++++++----
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 68bd138026..205f0a58d2 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -36,8 +36,6 @@ OBJECT_DECLARE_TYPE(AspeedI2CState, AspeedI2CClass, ASPEED_I2C)
 #define ASPEED_I2C_NR_BUSSES 16
 #define ASPEED_I2C_SHARE_POOL_SIZE 0x800
 #define ASPEED_I2C_BUS_POOL_SIZE 0x20
-#define ASPEED_I2C_OLD_NUM_REG 11
-#define ASPEED_I2C_NEW_NUM_REG 28
 
 #define A_I2CD_M_STOP_CMD       BIT(5)
 #define A_I2CD_M_RX_CMD         BIT(3)
@@ -256,7 +254,7 @@ struct AspeedI2CBus {
     uint8_t id;
     qemu_irq irq;
 
-    uint32_t regs[ASPEED_I2C_NEW_NUM_REG];
+    uint32_t *regs;
     uint8_t pool[ASPEED_I2C_BUS_POOL_SIZE];
     uint64_t dma_dram_offset;
 };
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index fb3d6a5600..cf3a003978 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -1091,10 +1091,9 @@ static const MemoryRegionOps aspeed_i2c_bus_pool_ops = {
 
 static const VMStateDescription aspeed_i2c_bus_vmstate = {
     .name = TYPE_ASPEED_I2C,
-    .version_id = 6,
-    .minimum_version_id = 6,
+    .version_id = 7,
+    .minimum_version_id = 7,
     .fields = (const VMStateField[]) {
-        VMSTATE_UINT32_ARRAY(regs, AspeedI2CBus, ASPEED_I2C_NEW_NUM_REG),
         VMSTATE_UINT8_ARRAY(pool, AspeedI2CBus, ASPEED_I2C_BUS_POOL_SIZE),
         VMSTATE_UINT64(dma_dram_offset, AspeedI2CBus),
         VMSTATE_END_OF_LIST()
@@ -1465,8 +1464,9 @@ static const TypeInfo aspeed_i2c_bus_slave_info = {
 static void aspeed_i2c_bus_reset(DeviceState *dev)
 {
     AspeedI2CBus *s = ASPEED_I2C_BUS(dev);
+    AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(s->controller);
 
-    memset(s->regs, 0, sizeof(s->regs));
+    memset(s->regs, 0, aic->reg_size);
     i2c_end_transfer(s->bus);
 }
 
@@ -1492,6 +1492,7 @@ static void aspeed_i2c_bus_realize(DeviceState *dev, Error **errp)
     s->slave = i2c_slave_create_simple(s->bus, TYPE_ASPEED_I2C_BUS_SLAVE,
                                        0xff);
 
+    s->regs = g_new(uint32_t, aic->reg_size >> 2);
     memory_region_init_io(&s->mr, OBJECT(s), &aspeed_i2c_bus_ops,
                           s, s->name, aic->reg_size);
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mr);
@@ -1501,6 +1502,14 @@ static void aspeed_i2c_bus_realize(DeviceState *dev, Error **errp)
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mr_pool);
 }
 
+static void aspeed_i2c_bus_unrealize(DeviceState *dev)
+{
+    AspeedI2CBus *s = ASPEED_I2C_BUS(dev);
+
+    g_free(s->regs);
+    s->regs = NULL;
+}
+
 static const Property aspeed_i2c_bus_properties[] = {
     DEFINE_PROP_UINT8("bus-id", AspeedI2CBus, id, 0),
     DEFINE_PROP_LINK("controller", AspeedI2CBus, controller, TYPE_ASPEED_I2C,
@@ -1514,6 +1523,7 @@ static void aspeed_i2c_bus_class_init(ObjectClass *klass, const void *data)
 
     dc->desc = "Aspeed I2C Bus";
     dc->realize = aspeed_i2c_bus_realize;
+    dc->unrealize = aspeed_i2c_bus_unrealize;
     device_class_set_legacy_reset(dc, aspeed_i2c_bus_reset);
     device_class_set_props(dc, aspeed_i2c_bus_properties);
 }
-- 
2.43.0
Re: [PATCH v1 01/10] hw/i2c/aspeed_i2c: Fix Out-of-Bounds access by using dynamic register array
Posted by Cédric Le Goater 3 days, 1 hour ago 
On 2/6/26 06:33, Jamin Lin wrote:
> The ASPEED I2C controller emulation used a fixed-size register array
> (28 dwords) for all SoC variants, while multiple ASPEED SoCs
> (AST2600, AST1030, AST2700) expose a larger MMIO register window
> (e.g. reg_size = 0x80).
> 
> This mismatch allows MMIO accesses beyond the allocated register
> array, leading to out-of-bounds reads in the I2C controller model.
> 
> Fix this by converting the register storage to a dynamically allocated
> array sized according to the controller class reg_size. The register
> array is now allocated during bus realize and free on unrealize,
> ensuring safe access across different ASPEED SoC implementations.
> 
> This change eliminates I2C register out-of-bounds access caused by
> SoC-specific register size differences.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3290

Please keep this patch separate from the A2 series.

> ---
>   include/hw/i2c/aspeed_i2c.h |  4 +---
>   hw/i2c/aspeed_i2c.c         | 18 ++++++++++++++----
>   2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> index 68bd138026..205f0a58d2 100644
> --- a/include/hw/i2c/aspeed_i2c.h
> +++ b/include/hw/i2c/aspeed_i2c.h
> @@ -36,8 +36,6 @@ OBJECT_DECLARE_TYPE(AspeedI2CState, AspeedI2CClass, ASPEED_I2C)
>   #define ASPEED_I2C_NR_BUSSES 16
>   #define ASPEED_I2C_SHARE_POOL_SIZE 0x800
>   #define ASPEED_I2C_BUS_POOL_SIZE 0x20
> -#define ASPEED_I2C_OLD_NUM_REG 11
> -#define ASPEED_I2C_NEW_NUM_REG 28
>   
>   #define A_I2CD_M_STOP_CMD       BIT(5)
>   #define A_I2CD_M_RX_CMD         BIT(3)
> @@ -256,7 +254,7 @@ struct AspeedI2CBus {
>       uint8_t id;
>       qemu_irq irq;
>   
> -    uint32_t regs[ASPEED_I2C_NEW_NUM_REG];
> +    uint32_t *regs;
>       uint8_t pool[ASPEED_I2C_BUS_POOL_SIZE];
>       uint64_t dma_dram_offset;
>   };
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index fb3d6a5600..cf3a003978 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -1091,10 +1091,9 @@ static const MemoryRegionOps aspeed_i2c_bus_pool_ops = {
>   
>   static const VMStateDescription aspeed_i2c_bus_vmstate = {
>       .name = TYPE_ASPEED_I2C,
> -    .version_id = 6,
> -    .minimum_version_id = 6,
> +    .version_id = 7,
> +    .minimum_version_id = 7,
>       .fields = (const VMStateField[]) {
> -        VMSTATE_UINT32_ARRAY(regs, AspeedI2CBus, ASPEED_I2C_NEW_NUM_REG),

hmm, why not use VMSTATE_VARRAY_UINT32 ?

>           VMSTATE_UINT8_ARRAY(pool, AspeedI2CBus, ASPEED_I2C_BUS_POOL_SIZE),
>           VMSTATE_UINT64(dma_dram_offset, AspeedI2CBus),
>           VMSTATE_END_OF_LIST()
> @@ -1465,8 +1464,9 @@ static const TypeInfo aspeed_i2c_bus_slave_info = {
>   static void aspeed_i2c_bus_reset(DeviceState *dev)
>   {
>       AspeedI2CBus *s = ASPEED_I2C_BUS(dev);
> +    AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(s->controller);
>   
> -    memset(s->regs, 0, sizeof(s->regs));
> +    memset(s->regs, 0, aic->reg_size);
>       i2c_end_transfer(s->bus);
>   }
>   
> @@ -1492,6 +1492,7 @@ static void aspeed_i2c_bus_realize(DeviceState *dev, Error **errp)
>       s->slave = i2c_slave_create_simple(s->bus, TYPE_ASPEED_I2C_BUS_SLAVE,
>                                          0xff);
>   
> +    s->regs = g_new(uint32_t, aic->reg_size >> 2);

g_new0 ? even if the memset in reset will set regs[] contents to 0.

>       memory_region_init_io(&s->mr, OBJECT(s), &aspeed_i2c_bus_ops,
>                             s, s->name, aic->reg_size);
>       sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mr);
> @@ -1501,6 +1502,14 @@ static void aspeed_i2c_bus_realize(DeviceState *dev, Error **errp)
>       sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mr_pool);
>   }
>   
> +static void aspeed_i2c_bus_unrealize(DeviceState *dev)
> +{
> +    AspeedI2CBus *s = ASPEED_I2C_BUS(dev);
> +
> +    g_free(s->regs);
> +    s->regs = NULL;
> +}
> +
>   static const Property aspeed_i2c_bus_properties[] = {
>       DEFINE_PROP_UINT8("bus-id", AspeedI2CBus, id, 0),
>       DEFINE_PROP_LINK("controller", AspeedI2CBus, controller, TYPE_ASPEED_I2C,
> @@ -1514,6 +1523,7 @@ static void aspeed_i2c_bus_class_init(ObjectClass *klass, const void *data)
>   
>       dc->desc = "Aspeed I2C Bus";
>       dc->realize = aspeed_i2c_bus_realize;
> +    dc->unrealize = aspeed_i2c_bus_unrealize;
>       device_class_set_legacy_reset(dc, aspeed_i2c_bus_reset);
>       device_class_set_props(dc, aspeed_i2c_bus_properties);
>   }
RE: [PATCH v1 01/10] hw/i2c/aspeed_i2c: Fix Out-of-Bounds access by using dynamic register array
Posted by Jamin Lin 10 hours ago 
Hi Cédric

> Subject: Re: [PATCH v1 01/10] hw/i2c/aspeed_i2c: Fix Out-of-Bounds access by
> using dynamic register array
> 
> On 2/6/26 06:33, Jamin Lin wrote:
> > The ASPEED I2C controller emulation used a fixed-size register array
> > (28 dwords) for all SoC variants, while multiple ASPEED SoCs (AST2600,
> > AST1030, AST2700) expose a larger MMIO register window (e.g. reg_size
> > = 0x80).
> >
> > This mismatch allows MMIO accesses beyond the allocated register
> > array, leading to out-of-bounds reads in the I2C controller model.
> >
> > Fix this by converting the register storage to a dynamically allocated
> > array sized according to the controller class reg_size. The register
> > array is now allocated during bus realize and free on unrealize,
> > ensuring safe access across different ASPEED SoC implementations.
> >
> > This change eliminates I2C register out-of-bounds access caused by
> > SoC-specific register size differences.
> >
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3290
> 
> Please keep this patch separate from the A2 series.
Thanks for the review.
Will resend

Thanks-Jamin

> 
> > ---
> >   include/hw/i2c/aspeed_i2c.h |  4 +---
> >   hw/i2c/aspeed_i2c.c         | 18 ++++++++++++++----
> >   2 files changed, 15 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> > index 68bd138026..205f0a58d2 100644
> > --- a/include/hw/i2c/aspeed_i2c.h
> > +++ b/include/hw/i2c/aspeed_i2c.h
> > @@ -36,8 +36,6 @@ OBJECT_DECLARE_TYPE(AspeedI2CState,
> AspeedI2CClass, ASPEED_I2C)
> >   #define ASPEED_I2C_NR_BUSSES 16
> >   #define ASPEED_I2C_SHARE_POOL_SIZE 0x800
> >   #define ASPEED_I2C_BUS_POOL_SIZE 0x20 -#define
> > ASPEED_I2C_OLD_NUM_REG 11 -#define ASPEED_I2C_NEW_NUM_REG 28
> >
> >   #define A_I2CD_M_STOP_CMD       BIT(5)
> >   #define A_I2CD_M_RX_CMD         BIT(3)
> > @@ -256,7 +254,7 @@ struct AspeedI2CBus {
> >       uint8_t id;
> >       qemu_irq irq;
> >
> > -    uint32_t regs[ASPEED_I2C_NEW_NUM_REG];
> > +    uint32_t *regs;
> >       uint8_t pool[ASPEED_I2C_BUS_POOL_SIZE];
> >       uint64_t dma_dram_offset;
> >   };
> > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index
> > fb3d6a5600..cf3a003978 100644
> > --- a/hw/i2c/aspeed_i2c.c
> > +++ b/hw/i2c/aspeed_i2c.c
> > @@ -1091,10 +1091,9 @@ static const MemoryRegionOps
> > aspeed_i2c_bus_pool_ops = {
> >
> >   static const VMStateDescription aspeed_i2c_bus_vmstate = {
> >       .name = TYPE_ASPEED_I2C,
> > -    .version_id = 6,
> > -    .minimum_version_id = 6,
> > +    .version_id = 7,
> > +    .minimum_version_id = 7,
> >       .fields = (const VMStateField[]) {
> > -        VMSTATE_UINT32_ARRAY(regs, AspeedI2CBus,
> ASPEED_I2C_NEW_NUM_REG),
> 
> hmm, why not use VMSTATE_VARRAY_UINT32 ?
> 
> >           VMSTATE_UINT8_ARRAY(pool, AspeedI2CBus,
> ASPEED_I2C_BUS_POOL_SIZE),
> >           VMSTATE_UINT64(dma_dram_offset, AspeedI2CBus),
> >           VMSTATE_END_OF_LIST()
> > @@ -1465,8 +1464,9 @@ static const TypeInfo aspeed_i2c_bus_slave_info =
> {
> >   static void aspeed_i2c_bus_reset(DeviceState *dev)
> >   {
> >       AspeedI2CBus *s = ASPEED_I2C_BUS(dev);
> > +    AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(s->controller);
> >
> > -    memset(s->regs, 0, sizeof(s->regs));
> > +    memset(s->regs, 0, aic->reg_size);
> >       i2c_end_transfer(s->bus);
> >   }
> >
> > @@ -1492,6 +1492,7 @@ static void aspeed_i2c_bus_realize(DeviceState
> *dev, Error **errp)
> >       s->slave = i2c_slave_create_simple(s->bus,
> TYPE_ASPEED_I2C_BUS_SLAVE,
> >                                          0xff);
> >
> > +    s->regs = g_new(uint32_t, aic->reg_size >> 2);
> 
> g_new0 ? even if the memset in reset will set regs[] contents to 0.
> 
> >       memory_region_init_io(&s->mr, OBJECT(s), &aspeed_i2c_bus_ops,
> >                             s, s->name, aic->reg_size);
> >       sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mr); @@ -1501,6
> > +1502,14 @@ static void aspeed_i2c_bus_realize(DeviceState *dev, Error
> **errp)
> >       sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mr_pool);
> >   }
> >
> > +static void aspeed_i2c_bus_unrealize(DeviceState *dev) {
> > +    AspeedI2CBus *s = ASPEED_I2C_BUS(dev);
> > +
> > +    g_free(s->regs);
> > +    s->regs = NULL;
> > +}
> > +
> >   static const Property aspeed_i2c_bus_properties[] = {
> >       DEFINE_PROP_UINT8("bus-id", AspeedI2CBus, id, 0),
> >       DEFINE_PROP_LINK("controller", AspeedI2CBus, controller,
> > TYPE_ASPEED_I2C, @@ -1514,6 +1523,7 @@ static void
> > aspeed_i2c_bus_class_init(ObjectClass *klass, const void *data)
> >
> >       dc->desc = "Aspeed I2C Bus";
> >       dc->realize = aspeed_i2c_bus_realize;
> > +    dc->unrealize = aspeed_i2c_bus_unrealize;
> >       device_class_set_legacy_reset(dc, aspeed_i2c_bus_reset);
> >       device_class_set_props(dc, aspeed_i2c_bus_properties);
> >   }

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.