[PATCH 0/1] hw: aspeed_gpio: Fix GPIO array indexing

[PATCH 0/1] hw: aspeed_gpio: Fix GPIO array indexing

[pdel]

From: Peter Delevoryas <pdel@fb.com>

Hey everyone,

I think there might be a bug aspeed_gpio_update, when it's selecting a
GPIO IRQ to update. I was testing booting Facebook's OpenBMC platform
"YosemiteV2" (fby2), and I was hitting a segfault in QEMU:

qemu-system-arm -machine ast2500-evb \
    -drive file=fby2.mtd,format=raw,if=mtd \
    -serial stdio -display none
...
Setup Caching for Bridge IC info..done.
Setup Front Panel Daemon..done.
Setup fan speed...
FAN CONFIG : Single Rotor FAN
Unexpected 4 Servers config! Run FSC 4 TLs Config as default config
Setting Zone 0 speed to 70%
Setting Zone 1 speed to 70%
ok: run: fscd: (pid 1726) 0s
done.
Powering fru 1 to ON state...
Segmentation fault (core dumped)

In gdb:

Thread 3 "qemu-system-arm" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff20ee700 (LWP 1840353)]
qemu_set_irq (irq=0xffffffff00000000, level=1) at ../hw/core/irq.c:45
45          irq->handler(irq->opaque, irq->n, level);
(gdb) p irq
$1 = (qemu_irq) 0xffffffff00000000
(gdb) up
#1  0x00005555558e36f5 in aspeed_gpio_update (s=0x7ffff7ecffb0, regs=0x7ffff7ed0c94, value=128) at ../hw/gpio/aspeed_gpio.c:287
287                     qemu_set_irq(s->gpios[offset], !!(new & mask));
(gdb) p s->gpios
$2 = {0x0 <repeats 228 times>}
(gdb) p offset
$3 = 231
(gdb) p set
$5 = 7
(gdb) p gpio
$4 = 7

The commit message for the fix has a little more info on the bug here,
see that for more info.

I tested this by verifying that after this diff, I can boot this fby2
platform. I don't see any unit or qtest's for aspeed gpio's, maybe I
could add one? I figured that, first, I could just put out an email to
let everyone know about it, and get the diff reviewed.

The image I was using is here:

https://github.com/peterdelevoryas/openbmc/releases/tag/fby2.debug.mtd

Peter Delevoryas (1):
  hw: aspeed_gpio: Fix GPIO array indexing

 hw/gpio/aspeed_gpio.c         | 80 +++++++++++++++--------------------
 include/hw/gpio/aspeed_gpio.h |  5 +--
 2 files changed, 35 insertions(+), 50 deletions(-)

-- 
2.30.2

[PATCH 1/1] hw: aspeed_gpio: Fix GPIO array indexing

[pdel]

From: Peter Delevoryas <pdel@fb.com>

The gpio array is declared as a dense array:

...
qemu_irq gpios[ASPEED_GPIO_NR_PINS];

(AST2500 has 228, AST2400 has 216, AST2600 has 208)

However, this array is used like a matrix of GPIO sets
(e.g. gpio[NR_SETS][NR_PINS_PER_SET] = gpio[8][32])

size_t offset = set * GPIOS_PER_SET + gpio;
qemu_set_irq(s->gpios[offset], !!(new & mask));

This can result in an out-of-bounds access to "s->gpios" because the
gpio sets do _not_ have the same length. Some of the groups (e.g.
GPIOAB) only have 4 pins. 228 != 8 * 32 == 256.

To fix this, I converted the gpio array from dense to sparse, to that
match both the hardware layout and this existing indexing code.

Also, I noticed that some of the property specifications looked wrong:
the lower 8 bits in the input and output u32's correspond to the first
group label, and the upper 8 bits correspond to the last group label.

I looked at the datasheet and several of these declarations seemed
incorrect to me (I was basing it off of the I/O column). If somebody
can double-check this, I'd really appreciate it!

Some were definitely wrong though, like "Y" and "Z" in the 2600.

@@ -796,7 +776,7 @@ static const GPIOSetProperties ast2500_set_props[] = {
     [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
     [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
     [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
-    [6] = {0xffffff0f,  0x0fffff0f,  {"Y", "Z", "AA", "AB"} },
+    [6] = {0x0fffffff,  0x0fffffff,  {"Y", "Z", "AA", "AB"} },
     [7] = {0x000000ff,  0x000000ff,  {"AC"} },
 };

@@ -805,9 +785,9 @@ static GPIOSetProperties ast2600_3_3v_set_props[] = {
     [1] = {0xffffffff,  0xffffffff,  {"E", "F", "G", "H"} },
     [2] = {0xffffffff,  0xffffffff,  {"I", "J", "K", "L"} },
     [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
-    [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
-    [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
-    [6] = {0xffff0000,  0x0fff0000,  {"Y", "Z", "", ""} },
+    [4] = {0xffffffff,  0x00ffffff,  {"Q", "R", "S", "T"} },
+    [5] = {0xffffffff,  0xffffff00,  {"U", "V", "W", "X"} },
+    [6] = {0x0000ffff,  0x0000ffff,  {"Y", "Z", "", ""} },
 };

Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
 hw/gpio/aspeed_gpio.c         | 80 +++++++++++++++--------------------
 include/hw/gpio/aspeed_gpio.h |  5 +--
 2 files changed, 35 insertions(+), 50 deletions(-)

diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index dfa6d6cb40..8e3f7e4398 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -16,11 +16,7 @@
 #include "hw/irq.h"
 #include "migration/vmstate.h"
 
-#define GPIOS_PER_REG 32
-#define GPIOS_PER_SET GPIOS_PER_REG
-#define GPIO_PIN_GAP_SIZE 4
 #define GPIOS_PER_GROUP 8
-#define GPIO_GROUP_SHIFT 3
 
 /* GPIO Source Types */
 #define ASPEED_CMD_SRC_MASK         0x01010101
@@ -259,7 +255,7 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
 
     diff = old ^ new;
     if (diff) {
-        for (gpio = 0; gpio < GPIOS_PER_REG; gpio++) {
+        for (gpio = 0; gpio < ASPEED_GPIOS_PER_SET; gpio++) {
             uint32_t mask = 1 << gpio;
 
             /* If the gpio needs to be updated... */
@@ -283,8 +279,7 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
             if (direction & mask) {
                 /* ...trigger the line-state IRQ */
                 ptrdiff_t set = aspeed_gpio_set_idx(s, regs);
-                size_t offset = set * GPIOS_PER_SET + gpio;
-                qemu_set_irq(s->gpios[offset], !!(new & mask));
+                qemu_set_irq(s->gpios[set][gpio], !!(new & mask));
             } else {
                 /* ...otherwise if we meet the line's current IRQ policy... */
                 if (aspeed_evaluate_irq(regs, old & mask, gpio)) {
@@ -297,21 +292,6 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
     qemu_set_irq(s->irq, !!(s->pending));
 }
 
-static uint32_t aspeed_adjust_pin(AspeedGPIOState *s, uint32_t pin)
-{
-    AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
-    /*
-     * The 2500 has a 4 pin gap in group AB and the 2400 has a 4 pin
-     * gap in group Y (and only four pins in AB but this is the last group so
-     * it doesn't matter).
-     */
-    if (agc->gap && pin >= agc->gap) {
-        pin += GPIO_PIN_GAP_SIZE;
-    }
-
-    return pin;
-}
-
 static bool aspeed_gpio_get_pin_level(AspeedGPIOState *s, uint32_t set_idx,
                                       uint32_t pin)
 {
@@ -367,7 +347,7 @@ static uint32_t update_value_control_source(GPIOSets *regs, uint32_t old_value,
     uint32_t new_value = 0;
 
     /* for each group in set */
-    for (i = 0; i < GPIOS_PER_REG; i += GPIOS_PER_GROUP) {
+    for (i = 0; i < ASPEED_GPIOS_PER_SET; i += GPIOS_PER_GROUP) {
         cmd_source = extract32(regs->cmd_source_0, i, 1)
                 | (extract32(regs->cmd_source_1, i, 1) << 1);
 
@@ -637,7 +617,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data,
          *   bidirectional  |   1       |   1        |  data
          *   input only     |   1       |   0        |   0
          *   output only    |   0       |   1        |   1
-         *   no pin / gap   |   0       |   0        |   0
+         *   no pin         |   0       |   0        |   0
          *
          *  which is captured by:
          *  data = ( data | ~input) & output;
@@ -796,7 +776,7 @@ static const GPIOSetProperties ast2500_set_props[] = {
     [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
     [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
     [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
-    [6] = {0xffffff0f,  0x0fffff0f,  {"Y", "Z", "AA", "AB"} },
+    [6] = {0x0fffffff,  0x0fffffff,  {"Y", "Z", "AA", "AB"} },
     [7] = {0x000000ff,  0x000000ff,  {"AC"} },
 };
 
@@ -805,9 +785,9 @@ static GPIOSetProperties ast2600_3_3v_set_props[] = {
     [1] = {0xffffffff,  0xffffffff,  {"E", "F", "G", "H"} },
     [2] = {0xffffffff,  0xffffffff,  {"I", "J", "K", "L"} },
     [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
-    [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
-    [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
-    [6] = {0xffff0000,  0x0fff0000,  {"Y", "Z", "", ""} },
+    [4] = {0xffffffff,  0x00ffffff,  {"Q", "R", "S", "T"} },
+    [5] = {0xffffffff,  0xffffff00,  {"U", "V", "W", "X"} },
+    [6] = {0x0000ffff,  0x0000ffff,  {"Y", "Z", "", ""} },
 };
 
 static GPIOSetProperties ast2600_1_8v_set_props[] = {
@@ -836,14 +816,20 @@ static void aspeed_gpio_realize(DeviceState *dev, Error **errp)
     AspeedGPIOState *s = ASPEED_GPIO(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
-    int pin;
 
     /* Interrupt parent line */
     sysbus_init_irq(sbd, &s->irq);
 
     /* Individual GPIOs */
-    for (pin = 0; pin < agc->nr_gpio_pins; pin++) {
-        sysbus_init_irq(sbd, &s->gpios[pin]);
+    for (int i = 0; i < ASPEED_GPIO_MAX_NR_SETS; i++) {
+        const GPIOSetProperties *props = &agc->props[i];
+        uint32_t skip = ~(props->input | props->output);
+        for (int j = 0; j < ASPEED_GPIOS_PER_SET; j++) {
+            if (skip >> j & 1) {
+                continue;
+            }
+            sysbus_init_irq(sbd, &s->gpios[i][j]);
+        }
     }
 
     memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_gpio_ops, s,
@@ -856,20 +842,22 @@ static void aspeed_gpio_init(Object *obj)
 {
     AspeedGPIOState *s = ASPEED_GPIO(obj);
     AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
-    int pin;
-
-    for (pin = 0; pin < agc->nr_gpio_pins; pin++) {
-        char *name;
-        int set_idx = pin / GPIOS_PER_SET;
-        int pin_idx = aspeed_adjust_pin(s, pin) - (set_idx * GPIOS_PER_SET);
-        int group_idx = pin_idx >> GPIO_GROUP_SHIFT;
-        const GPIOSetProperties *props = &agc->props[set_idx];
-
-        name = g_strdup_printf("gpio%s%d", props->group_label[group_idx],
-                               pin_idx % GPIOS_PER_GROUP);
-        object_property_add(obj, name, "bool", aspeed_gpio_get_pin,
-                            aspeed_gpio_set_pin, NULL, NULL);
-        g_free(name);
+
+    for (int i = 0; i < ASPEED_GPIO_MAX_NR_SETS; i++) {
+        const GPIOSetProperties *props = &agc->props[i];
+        uint32_t skip = ~(props->input | props->output);
+        for (int j = 0; j < ASPEED_GPIOS_PER_SET; j++) {
+            if (skip >> j & 1) {
+                continue;
+            }
+            int group_idx = j / GPIOS_PER_GROUP;
+            int pin_idx = j % GPIOS_PER_GROUP;
+            const char *group = &props->group_label[group_idx][0];
+            char *name = g_strdup_printf("gpio%s%d", group, pin_idx);
+            object_property_add(obj, name, "bool", aspeed_gpio_get_pin,
+                                aspeed_gpio_set_pin, NULL, NULL);
+            g_free(name);
+        }
     }
 }
 
@@ -926,7 +914,6 @@ static void aspeed_gpio_ast2400_class_init(ObjectClass *klass, void *data)
     agc->props = ast2400_set_props;
     agc->nr_gpio_pins = 216;
     agc->nr_gpio_sets = 7;
-    agc->gap = 196;
     agc->reg_table = aspeed_3_3v_gpios;
 }
 
@@ -937,7 +924,6 @@ static void aspeed_gpio_2500_class_init(ObjectClass *klass, void *data)
     agc->props = ast2500_set_props;
     agc->nr_gpio_pins = 228;
     agc->nr_gpio_sets = 8;
-    agc->gap = 220;
     agc->reg_table = aspeed_3_3v_gpios;
 }
 
diff --git a/include/hw/gpio/aspeed_gpio.h b/include/hw/gpio/aspeed_gpio.h
index e1636ce7fe..801846befb 100644
--- a/include/hw/gpio/aspeed_gpio.h
+++ b/include/hw/gpio/aspeed_gpio.h
@@ -17,9 +17,9 @@
 OBJECT_DECLARE_TYPE(AspeedGPIOState, AspeedGPIOClass, ASPEED_GPIO)
 
 #define ASPEED_GPIO_MAX_NR_SETS 8
+#define ASPEED_GPIOS_PER_SET 32
 #define ASPEED_REGS_PER_BANK 14
 #define ASPEED_GPIO_MAX_NR_REGS (ASPEED_REGS_PER_BANK * ASPEED_GPIO_MAX_NR_SETS)
-#define ASPEED_GPIO_NR_PINS 228
 #define ASPEED_GROUPS_PER_SET 4
 #define ASPEED_GPIO_NR_DEBOUNCE_REGS 3
 #define ASPEED_CHARS_PER_GROUP_LABEL 4
@@ -60,7 +60,6 @@ struct AspeedGPIOClass {
     const GPIOSetProperties *props;
     uint32_t nr_gpio_pins;
     uint32_t nr_gpio_sets;
-    uint32_t gap;
     const AspeedGPIOReg *reg_table;
 };
 
@@ -72,7 +71,7 @@ struct AspeedGPIOState {
     MemoryRegion iomem;
     int pending;
     qemu_irq irq;
-    qemu_irq gpios[ASPEED_GPIO_NR_PINS];
+    qemu_irq gpios[ASPEED_GPIO_MAX_NR_SETS][ASPEED_GPIOS_PER_SET];
 
 /* Parallel GPIO Registers */
     uint32_t debounce_regs[ASPEED_GPIO_NR_DEBOUNCE_REGS];
-- 
2.30.2

Re: [PATCH 1/1] hw: aspeed_gpio: Fix GPIO array indexing

[Philippe Mathieu-Daudé]

Hi Peter,

On 9/24/21 08:19, pdel@fb.com wrote:
> From: Peter Delevoryas <pdel@fb.com>
> 
> The gpio array is declared as a dense array:
> 
> ...
> qemu_irq gpios[ASPEED_GPIO_NR_PINS];
> 
> (AST2500 has 228, AST2400 has 216, AST2600 has 208)
> 
> However, this array is used like a matrix of GPIO sets
> (e.g. gpio[NR_SETS][NR_PINS_PER_SET] = gpio[8][32])
> 
> size_t offset = set * GPIOS_PER_SET + gpio;
> qemu_set_irq(s->gpios[offset], !!(new & mask));
> 
> This can result in an out-of-bounds access to "s->gpios" because the
> gpio sets do _not_ have the same length. Some of the groups (e.g.
> GPIOAB) only have 4 pins. 228 != 8 * 32 == 256.
> 
> To fix this, I converted the gpio array from dense to sparse, to that
> match both the hardware layout and this existing indexing code.

This is one logical change: 1 patch

> Also, I noticed that some of the property specifications looked wrong:
> the lower 8 bits in the input and output u32's correspond to the first
> group label, and the upper 8 bits correspond to the last group label.

Another logical change: another patch :)

So please split this patch in 2. Maybe easier to fix GPIOSetProperties
first, then convert from dense to sparse array.

Regards,

Phil.

> I looked at the datasheet and several of these declarations seemed
> incorrect to me (I was basing it off of the I/O column). If somebody
> can double-check this, I'd really appreciate it!
> 
> Some were definitely wrong though, like "Y" and "Z" in the 2600.
> 
> @@ -796,7 +776,7 @@ static const GPIOSetProperties ast2500_set_props[] = {
>       [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
>       [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
>       [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
> -    [6] = {0xffffff0f,  0x0fffff0f,  {"Y", "Z", "AA", "AB"} },
> +    [6] = {0x0fffffff,  0x0fffffff,  {"Y", "Z", "AA", "AB"} },
>       [7] = {0x000000ff,  0x000000ff,  {"AC"} },
>   };
> 
> @@ -805,9 +785,9 @@ static GPIOSetProperties ast2600_3_3v_set_props[] = {
>       [1] = {0xffffffff,  0xffffffff,  {"E", "F", "G", "H"} },
>       [2] = {0xffffffff,  0xffffffff,  {"I", "J", "K", "L"} },
>       [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
> -    [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
> -    [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
> -    [6] = {0xffff0000,  0x0fff0000,  {"Y", "Z", "", ""} },
> +    [4] = {0xffffffff,  0x00ffffff,  {"Q", "R", "S", "T"} },
> +    [5] = {0xffffffff,  0xffffff00,  {"U", "V", "W", "X"} },
> +    [6] = {0x0000ffff,  0x0000ffff,  {"Y", "Z", "", ""} },
>   };
> 
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---
>   hw/gpio/aspeed_gpio.c         | 80 +++++++++++++++--------------------
>   include/hw/gpio/aspeed_gpio.h |  5 +--
>   2 files changed, 35 insertions(+), 50 deletions(-)

Re: [PATCH 1/1] hw: aspeed_gpio: Fix GPIO array indexing

[Peter Delevoryas]

> On Sep 25, 2021, at 4:03 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 
> Hi Peter,
> 
>> On 9/24/21 08:19, pdel@fb.com wrote:
>> From: Peter Delevoryas <pdel@fb.com>
>> The gpio array is declared as a dense array:
>> ...
>> qemu_irq gpios[ASPEED_GPIO_NR_PINS];
>> (AST2500 has 228, AST2400 has 216, AST2600 has 208)
>> However, this array is used like a matrix of GPIO sets
>> (e.g. gpio[NR_SETS][NR_PINS_PER_SET] = gpio[8][32])
>> size_t offset = set * GPIOS_PER_SET + gpio;
>> qemu_set_irq(s->gpios[offset], !!(new & mask));
>> This can result in an out-of-bounds access to "s->gpios" because the
>> gpio sets do _not_ have the same length. Some of the groups (e.g.
>> GPIOAB) only have 4 pins. 228 != 8 * 32 == 256.
>> To fix this, I converted the gpio array from dense to sparse, to that
>> match both the hardware layout and this existing indexing code.
> 
> This is one logical change: 1 patch
> 
>> Also, I noticed that some of the property specifications looked wrong:
>> the lower 8 bits in the input and output u32's correspond to the first
>> group label, and the upper 8 bits correspond to the last group label.
> 
> Another logical change: another patch :)
> 
> So please split this patch in 2. Maybe easier to fix GPIOSetProperties
> first, then convert from dense to sparse array.
> 

Good point, I’ll split it up then!

> Regards,
> 
> Phil.
> 
>> I looked at the datasheet and several of these declarations seemed
>> incorrect to me (I was basing it off of the I/O column). If somebody
>> can double-check this, I'd really appreciate it!
>> Some were definitely wrong though, like "Y" and "Z" in the 2600.
>> @@ -796,7 +776,7 @@ static const GPIOSetProperties ast2500_set_props[] = {
>>      [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
>>      [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
>>      [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
>> -    [6] = {0xffffff0f,  0x0fffff0f,  {"Y", "Z", "AA", "AB"} },
>> +    [6] = {0x0fffffff,  0x0fffffff,  {"Y", "Z", "AA", "AB"} },
>>      [7] = {0x000000ff,  0x000000ff,  {"AC"} },
>>  };
>> @@ -805,9 +785,9 @@ static GPIOSetProperties ast2600_3_3v_set_props[] = {
>>      [1] = {0xffffffff,  0xffffffff,  {"E", "F", "G", "H"} },
>>      [2] = {0xffffffff,  0xffffffff,  {"I", "J", "K", "L"} },
>>      [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
>> -    [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
>> -    [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
>> -    [6] = {0xffff0000,  0x0fff0000,  {"Y", "Z", "", ""} },
>> +    [4] = {0xffffffff,  0x00ffffff,  {"Q", "R", "S", "T"} },
>> +    [5] = {0xffffffff,  0xffffff00,  {"U", "V", "W", "X"} },
>> +    [6] = {0x0000ffff,  0x0000ffff,  {"Y", "Z", "", ""} },
>>  };
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>> ---
>>  hw/gpio/aspeed_gpio.c         | 80 +++++++++++++++--------------------
>>  include/hw/gpio/aspeed_gpio.h |  5 +--
>>  2 files changed, 35 insertions(+), 50 deletions(-)

Re: [PATCH 1/1] hw: aspeed_gpio: Fix GPIO array indexing

[Cédric Le Goater]

On 9/25/21 16:28, Peter Delevoryas wrote:
> 
>> On Sep 25, 2021, at 4:03 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Hi Peter,
>>
>>> On 9/24/21 08:19, pdel@fb.com wrote:
>>> From: Peter Delevoryas <pdel@fb.com>
>>> The gpio array is declared as a dense array:
>>> ...
>>> qemu_irq gpios[ASPEED_GPIO_NR_PINS];
>>> (AST2500 has 228, AST2400 has 216, AST2600 has 208)
>>> However, this array is used like a matrix of GPIO sets
>>> (e.g. gpio[NR_SETS][NR_PINS_PER_SET] = gpio[8][32])
>>> size_t offset = set * GPIOS_PER_SET + gpio;
>>> qemu_set_irq(s->gpios[offset], !!(new & mask));
>>> This can result in an out-of-bounds access to "s->gpios" because the
>>> gpio sets do _not_ have the same length. Some of the groups (e.g.
>>> GPIOAB) only have 4 pins. 228 != 8 * 32 == 256.
>>> To fix this, I converted the gpio array from dense to sparse, to that
>>> match both the hardware layout and this existing indexing code.
>>
>> This is one logical change: 1 patch
>>
>>> Also, I noticed that some of the property specifications looked wrong:
>>> the lower 8 bits in the input and output u32's correspond to the first
>>> group label, and the upper 8 bits correspond to the last group label.
>>
>> Another logical change: another patch :)
>>
>> So please split this patch in 2. Maybe easier to fix GPIOSetProperties
>> first, then convert from dense to sparse array.
>>
> 
> Good point, I’ll split it up then!

Yes. We can surely merge the GPIOSetProperties patch quickly.

I hope Rashmica has some time to check the second part.
  
Thanks,

C.


> 
>> Regards,
>>
>> Phil.
>>
>>> I looked at the datasheet and several of these declarations seemed
>>> incorrect to me (I was basing it off of the I/O column). If somebody
>>> can double-check this, I'd really appreciate it!
>>> Some were definitely wrong though, like "Y" and "Z" in the 2600.
>>> @@ -796,7 +776,7 @@ static const GPIOSetProperties ast2500_set_props[] = {
>>>       [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
>>>       [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
>>>       [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
>>> -    [6] = {0xffffff0f,  0x0fffff0f,  {"Y", "Z", "AA", "AB"} },
>>> +    [6] = {0x0fffffff,  0x0fffffff,  {"Y", "Z", "AA", "AB"} },
>>>       [7] = {0x000000ff,  0x000000ff,  {"AC"} },
>>>   };
>>> @@ -805,9 +785,9 @@ static GPIOSetProperties ast2600_3_3v_set_props[] = {
>>>       [1] = {0xffffffff,  0xffffffff,  {"E", "F", "G", "H"} },
>>>       [2] = {0xffffffff,  0xffffffff,  {"I", "J", "K", "L"} },
>>>       [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
>>> -    [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
>>> -    [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
>>> -    [6] = {0xffff0000,  0x0fff0000,  {"Y", "Z", "", ""} },
>>> +    [4] = {0xffffffff,  0x00ffffff,  {"Q", "R", "S", "T"} },
>>> +    [5] = {0xffffffff,  0xffffff00,  {"U", "V", "W", "X"} },
>>> +    [6] = {0x0000ffff,  0x0000ffff,  {"Y", "Z", "", ""} },
>>>   };
>>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>>> ---
>>>   hw/gpio/aspeed_gpio.c         | 80 +++++++++++++++--------------------
>>>   include/hw/gpio/aspeed_gpio.h |  5 +--
>>>   2 files changed, 35 insertions(+), 50 deletions(-)

Re: [PATCH 1/1] hw: aspeed_gpio: Fix GPIO array indexing

[Peter Delevoryas]

> On Oct 4, 2021, at 4:43 AM, Cédric Le Goater <clg@kaod.org> wrote:
> 
> On 10/4/21 11:07, Cédric Le Goater wrote:
>> On 9/28/21 05:43, pdel@fb.com wrote:
>>> From: Peter Delevoryas <pdel@fb.com>
>>> 
>>> The gpio array is declared as a dense array:
>>> 
>>>    qemu_irq gpios[ASPEED_GPIO_NR_PINS];
>>> 
>>> (AST2500 has 228, AST2400 has 216, AST2600 has 208)
>>> 
>>> However, this array is used like a matrix of GPIO sets
>>> (e.g. gpio[NR_SETS][NR_PINS_PER_SET] = gpio[8][32])
>>> 
>>>    size_t offset = set * GPIOS_PER_SET + gpio;
>>>    qemu_set_irq(s->gpios[offset], !!(new & mask));
>>> 
>>> This can result in an out-of-bounds access to "s->gpios" because the
>>> gpio sets do _not_ have the same length. Some of the groups (e.g.
>>> GPIOAB) only have 4 pins. 228 != 8 * 32 == 256.
>>> 
>>> To fix this, I converted the gpio array from dense to sparse, to match
>>> both the hardware layout and this existing indexing code.
>>> 
>>> Fixes: 4b7f956862dc2db4c5c ("hw/gpio: Add basic Aspeed GPIO model for AST2400 and AST2500")
>>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>> This patch is raising an error in qtest-arm/qom-test when compiled
>> with clang :
>> Running test qtest-arm/qom-test
>> Unexpected error in object_property_try_add() at ../qom/object.c:1224:
>> qemu-system-arm: attempt to add duplicate property 'gpio0' to object (type 'aspeed.gpio-ast2600-1_8v')
>> Broken pipe
>> ERROR qtest-arm/qom-test - too few tests run (expected 78, got 0)
>> make: *** [Makefile.mtest:24: run-test-1] Error 1
> 
> The GPIOSetProperties arrary is smaller for the ast2600_1_8v model :
> 
>  static GPIOSetProperties ast2600_1_8v_set_props[] = {
>      [0] = {0xffffffff,  0xffffffff,  {"18A", "18B", "18C", "18D"} },
>      [1] = {0x0000000f,  0x0000000f,  {"18E"} },
>  };
> 
> and in aspeed_gpio_init() :
> 
>    for (int i = 0; i < ASPEED_GPIO_MAX_NR_SETS; i++) {
> 
> we loop beyond.
> 
> To configure compilation with clang, use the configure option :
> 
>  --cc=clang
> 
> and simply run 'make check-qtest-arm'

Thanks for pointing this out! To fix it, I think the simplest thing I could do
is just make sure all of the GPIOSetProperties arrays have the same length,
ASPEED_GPIO_MAX_NR_SETS. There is already a filtering mechanism in place in
the code that iterates over the properties to skip zeroed entries. Alternatively,
we could keep some variable length nr_sets value with each class, but I figured
that is more complicated and error-prone. I’m submitting the v2 version
with this fix.

Thanks,
Peter

diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index 911d21c8cf..78b0f64b03 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -759,7 +759,7 @@ static void aspeed_gpio_set_pin(Object *obj, Visitor *v, const char *name,
 }

 /****************** Setup functions ******************/
-static const GPIOSetProperties ast2400_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
+static const GPIOSetProperties ast2400_set_props[] = {
     [0] = {0xffffffff,  0xffffffff,  {"A", "B", "C", "D"} },
     [1] = {0xffffffff,  0xffffffff,  {"E", "F", "G", "H"} },
     [2] = {0xffffffff,  0xffffffff,  {"I", "J", "K", "L"} },
@@ -769,7 +769,7 @@ static const GPIOSetProperties ast2400_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
     [6] = {0x0000000f,  0x0fffff0f,  {"Y", "Z", "AA", "AB"} },
 };

-static const GPIOSetProperties ast2500_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
+static const GPIOSetProperties ast2500_set_props[] = {
     [0] = {0xffffffff,  0xffffffff,  {"A", "B", "C", "D"} },
     [1] = {0xffffffff,  0xffffffff,  {"E", "F", "G", "H"} },
     [2] = {0xffffffff,  0xffffffff,  {"I", "J", "K", "L"} },
@@ -780,7 +780,7 @@ static const GPIOSetProperties ast2500_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
     [7] = {0x000000ff,  0x000000ff,  {"AC"} },
 };

-static GPIOSetProperties ast2600_3_3v_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
+static GPIOSetProperties ast2600_3_3v_set_props[] = {
     [0] = {0xffffffff,  0xffffffff,  {"A", "B", "C", "D"} },
     [1] = {0xffffffff,  0xffffffff,  {"E", "F", "G", "H"} },
     [2] = {0xffffffff,  0xffffffff,  {"I", "J", "K", "L"} },
@@ -790,7 +790,7 @@ static GPIOSetProperties ast2600_3_3v_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
     [6] = {0x0000ffff,  0x0000ffff,  {"Y", "Z"} },
 };

-static GPIOSetProperties ast2600_1_8v_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
+static GPIOSetProperties ast2600_1_8v_set_props[] = {
     [0] = {0xffffffff,  0xffffffff,  {"18A", "18B", "18C", "18D"} },
     [1] = {0x0000000f,  0x0000000f,  {"18E"} },
 };

> 
> Thanks
> 
> C.

Re: [PATCH 1/1] hw: aspeed_gpio: Fix GPIO array indexing

[Cédric Le Goater]

On 10/4/21 11:07, Cédric Le Goater wrote:
> On 9/28/21 05:43, pdel@fb.com wrote:
>> From: Peter Delevoryas <pdel@fb.com>
>>
>> The gpio array is declared as a dense array:
>>
>>    qemu_irq gpios[ASPEED_GPIO_NR_PINS];
>>
>> (AST2500 has 228, AST2400 has 216, AST2600 has 208)
>>
>> However, this array is used like a matrix of GPIO sets
>> (e.g. gpio[NR_SETS][NR_PINS_PER_SET] = gpio[8][32])
>>
>>    size_t offset = set * GPIOS_PER_SET + gpio;
>>    qemu_set_irq(s->gpios[offset], !!(new & mask));
>>
>> This can result in an out-of-bounds access to "s->gpios" because the
>> gpio sets do _not_ have the same length. Some of the groups (e.g.
>> GPIOAB) only have 4 pins. 228 != 8 * 32 == 256.
>>
>> To fix this, I converted the gpio array from dense to sparse, to match
>> both the hardware layout and this existing indexing code.
>>
>> Fixes: 4b7f956862dc2db4c5c ("hw/gpio: Add basic Aspeed GPIO model for AST2400 and AST2500")
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> 
> 
> This patch is raising an error in qtest-arm/qom-test when compiled
> with clang :
> 
> Running test qtest-arm/qom-test
> Unexpected error in object_property_try_add() at ../qom/object.c:1224:
> qemu-system-arm: attempt to add duplicate property 'gpio0' to object (type 'aspeed.gpio-ast2600-1_8v')
> Broken pipe
> ERROR qtest-arm/qom-test - too few tests run (expected 78, got 0)
> make: *** [Makefile.mtest:24: run-test-1] Error 1

The GPIOSetProperties arrary is smaller for the ast2600_1_8v model :

   static GPIOSetProperties ast2600_1_8v_set_props[] = {
       [0] = {0xffffffff,  0xffffffff,  {"18A", "18B", "18C", "18D"} },
       [1] = {0x0000000f,  0x0000000f,  {"18E"} },
   };

and in aspeed_gpio_init() :

     for (int i = 0; i < ASPEED_GPIO_MAX_NR_SETS; i++) {

we loop beyond.

To configure compilation with clang, use the configure option :

   --cc=clang

and simply run 'make check-qtest-arm'

Thanks

C.

Re: [PATCH 1/1] hw: aspeed_gpio: Fix GPIO array indexing

[Cédric Le Goater]

On 9/28/21 05:43, pdel@fb.com wrote:
> From: Peter Delevoryas <pdel@fb.com>
> 
> The gpio array is declared as a dense array:
> 
>    qemu_irq gpios[ASPEED_GPIO_NR_PINS];
> 
> (AST2500 has 228, AST2400 has 216, AST2600 has 208)
> 
> However, this array is used like a matrix of GPIO sets
> (e.g. gpio[NR_SETS][NR_PINS_PER_SET] = gpio[8][32])
> 
>    size_t offset = set * GPIOS_PER_SET + gpio;
>    qemu_set_irq(s->gpios[offset], !!(new & mask));
> 
> This can result in an out-of-bounds access to "s->gpios" because the
> gpio sets do _not_ have the same length. Some of the groups (e.g.
> GPIOAB) only have 4 pins. 228 != 8 * 32 == 256.
> 
> To fix this, I converted the gpio array from dense to sparse, to match
> both the hardware layout and this existing indexing code.
> 
> Fixes: 4b7f956862dc2db4c5c ("hw/gpio: Add basic Aspeed GPIO model for AST2400 and AST2500")
> Signed-off-by: Peter Delevoryas <pdel@fb.com>


This patch is raising an error in qtest-arm/qom-test when compiled
with clang :

Running test qtest-arm/qom-test
Unexpected error in object_property_try_add() at ../qom/object.c:1224:
qemu-system-arm: attempt to add duplicate property 'gpio0' to object (type 'aspeed.gpio-ast2600-1_8v')
Broken pipe
ERROR qtest-arm/qom-test - too few tests run (expected 78, got 0)
make: *** [Makefile.mtest:24: run-test-1] Error 1

Thanks,

C.


> ---
>   hw/gpio/aspeed_gpio.c         | 72 ++++++++++++++---------------------
>   include/hw/gpio/aspeed_gpio.h |  5 +--
>   2 files changed, 31 insertions(+), 46 deletions(-)
> 
> diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> index dfa6d6cb40..f04d4a454c 100644
> --- a/hw/gpio/aspeed_gpio.c
> +++ b/hw/gpio/aspeed_gpio.c
> @@ -16,11 +16,7 @@
>   #include "hw/irq.h"
>   #include "migration/vmstate.h"
>   
> -#define GPIOS_PER_REG 32
> -#define GPIOS_PER_SET GPIOS_PER_REG
> -#define GPIO_PIN_GAP_SIZE 4
>   #define GPIOS_PER_GROUP 8
> -#define GPIO_GROUP_SHIFT 3
>   
>   /* GPIO Source Types */
>   #define ASPEED_CMD_SRC_MASK         0x01010101
> @@ -259,7 +255,7 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
>   
>       diff = old ^ new;
>       if (diff) {
> -        for (gpio = 0; gpio < GPIOS_PER_REG; gpio++) {
> +        for (gpio = 0; gpio < ASPEED_GPIOS_PER_SET; gpio++) {
>               uint32_t mask = 1 << gpio;
>   
>               /* If the gpio needs to be updated... */
> @@ -283,8 +279,7 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
>               if (direction & mask) {
>                   /* ...trigger the line-state IRQ */
>                   ptrdiff_t set = aspeed_gpio_set_idx(s, regs);
> -                size_t offset = set * GPIOS_PER_SET + gpio;
> -                qemu_set_irq(s->gpios[offset], !!(new & mask));
> +                qemu_set_irq(s->gpios[set][gpio], !!(new & mask));
>               } else {
>                   /* ...otherwise if we meet the line's current IRQ policy... */
>                   if (aspeed_evaluate_irq(regs, old & mask, gpio)) {
> @@ -297,21 +292,6 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
>       qemu_set_irq(s->irq, !!(s->pending));
>   }
>   
> -static uint32_t aspeed_adjust_pin(AspeedGPIOState *s, uint32_t pin)
> -{
> -    AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
> -    /*
> -     * The 2500 has a 4 pin gap in group AB and the 2400 has a 4 pin
> -     * gap in group Y (and only four pins in AB but this is the last group so
> -     * it doesn't matter).
> -     */
> -    if (agc->gap && pin >= agc->gap) {
> -        pin += GPIO_PIN_GAP_SIZE;
> -    }
> -
> -    return pin;
> -}
> -
>   static bool aspeed_gpio_get_pin_level(AspeedGPIOState *s, uint32_t set_idx,
>                                         uint32_t pin)
>   {
> @@ -367,7 +347,7 @@ static uint32_t update_value_control_source(GPIOSets *regs, uint32_t old_value,
>       uint32_t new_value = 0;
>   
>       /* for each group in set */
> -    for (i = 0; i < GPIOS_PER_REG; i += GPIOS_PER_GROUP) {
> +    for (i = 0; i < ASPEED_GPIOS_PER_SET; i += GPIOS_PER_GROUP) {
>           cmd_source = extract32(regs->cmd_source_0, i, 1)
>                   | (extract32(regs->cmd_source_1, i, 1) << 1);
>   
> @@ -637,7 +617,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data,
>            *   bidirectional  |   1       |   1        |  data
>            *   input only     |   1       |   0        |   0
>            *   output only    |   0       |   1        |   1
> -         *   no pin / gap   |   0       |   0        |   0
> +         *   no pin         |   0       |   0        |   0
>            *
>            *  which is captured by:
>            *  data = ( data | ~input) & output;
> @@ -836,14 +816,20 @@ static void aspeed_gpio_realize(DeviceState *dev, Error **errp)
>       AspeedGPIOState *s = ASPEED_GPIO(dev);
>       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>       AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
> -    int pin;
>   
>       /* Interrupt parent line */
>       sysbus_init_irq(sbd, &s->irq);
>   
>       /* Individual GPIOs */
> -    for (pin = 0; pin < agc->nr_gpio_pins; pin++) {
> -        sysbus_init_irq(sbd, &s->gpios[pin]);
> +    for (int i = 0; i < ASPEED_GPIO_MAX_NR_SETS; i++) {
> +        const GPIOSetProperties *props = &agc->props[i];
> +        uint32_t skip = ~(props->input | props->output);
> +        for (int j = 0; j < ASPEED_GPIOS_PER_SET; j++) {
> +            if (skip >> j & 1) {
> +                continue;
> +            }
> +            sysbus_init_irq(sbd, &s->gpios[i][j]);
> +        }
>       }
>   
>       memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_gpio_ops, s,
> @@ -856,20 +842,22 @@ static void aspeed_gpio_init(Object *obj)
>   {
>       AspeedGPIOState *s = ASPEED_GPIO(obj);
>       AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
> -    int pin;
> -
> -    for (pin = 0; pin < agc->nr_gpio_pins; pin++) {
> -        char *name;
> -        int set_idx = pin / GPIOS_PER_SET;
> -        int pin_idx = aspeed_adjust_pin(s, pin) - (set_idx * GPIOS_PER_SET);
> -        int group_idx = pin_idx >> GPIO_GROUP_SHIFT;
> -        const GPIOSetProperties *props = &agc->props[set_idx];
> -
> -        name = g_strdup_printf("gpio%s%d", props->group_label[group_idx],
> -                               pin_idx % GPIOS_PER_GROUP);
> -        object_property_add(obj, name, "bool", aspeed_gpio_get_pin,
> -                            aspeed_gpio_set_pin, NULL, NULL);
> -        g_free(name);
> +
> +    for (int i = 0; i < ASPEED_GPIO_MAX_NR_SETS; i++) {
> +        const GPIOSetProperties *props = &agc->props[i];
> +        uint32_t skip = ~(props->input | props->output);
> +        for (int j = 0; j < ASPEED_GPIOS_PER_SET; j++) {
> +            if (skip >> j & 1) {
> +                continue;
> +            }
> +            int group_idx = j / GPIOS_PER_GROUP;
> +            int pin_idx = j % GPIOS_PER_GROUP;
> +            const char *group = &props->group_label[group_idx][0];
> +            char *name = g_strdup_printf("gpio%s%d", group, pin_idx);
> +            object_property_add(obj, name, "bool", aspeed_gpio_get_pin,
> +                                aspeed_gpio_set_pin, NULL, NULL);
> +            g_free(name);
> +        }
>       }
>   }
>   
> @@ -926,7 +914,6 @@ static void aspeed_gpio_ast2400_class_init(ObjectClass *klass, void *data)
>       agc->props = ast2400_set_props;
>       agc->nr_gpio_pins = 216;
>       agc->nr_gpio_sets = 7;
> -    agc->gap = 196;
>       agc->reg_table = aspeed_3_3v_gpios;
>   }
>   
> @@ -937,7 +924,6 @@ static void aspeed_gpio_2500_class_init(ObjectClass *klass, void *data)
>       agc->props = ast2500_set_props;
>       agc->nr_gpio_pins = 228;
>       agc->nr_gpio_sets = 8;
> -    agc->gap = 220;
>       agc->reg_table = aspeed_3_3v_gpios;
>   }
>   
> diff --git a/include/hw/gpio/aspeed_gpio.h b/include/hw/gpio/aspeed_gpio.h
> index e1636ce7fe..801846befb 100644
> --- a/include/hw/gpio/aspeed_gpio.h
> +++ b/include/hw/gpio/aspeed_gpio.h
> @@ -17,9 +17,9 @@
>   OBJECT_DECLARE_TYPE(AspeedGPIOState, AspeedGPIOClass, ASPEED_GPIO)
>   
>   #define ASPEED_GPIO_MAX_NR_SETS 8
> +#define ASPEED_GPIOS_PER_SET 32
>   #define ASPEED_REGS_PER_BANK 14
>   #define ASPEED_GPIO_MAX_NR_REGS (ASPEED_REGS_PER_BANK * ASPEED_GPIO_MAX_NR_SETS)
> -#define ASPEED_GPIO_NR_PINS 228
>   #define ASPEED_GROUPS_PER_SET 4
>   #define ASPEED_GPIO_NR_DEBOUNCE_REGS 3
>   #define ASPEED_CHARS_PER_GROUP_LABEL 4
> @@ -60,7 +60,6 @@ struct AspeedGPIOClass {
>       const GPIOSetProperties *props;
>       uint32_t nr_gpio_pins;
>       uint32_t nr_gpio_sets;
> -    uint32_t gap;
>       const AspeedGPIOReg *reg_table;
>   };
>   
> @@ -72,7 +71,7 @@ struct AspeedGPIOState {
>       MemoryRegion iomem;
>       int pending;
>       qemu_irq irq;
> -    qemu_irq gpios[ASPEED_GPIO_NR_PINS];
> +    qemu_irq gpios[ASPEED_GPIO_MAX_NR_SETS][ASPEED_GPIOS_PER_SET];
>   
>   /* Parallel GPIO Registers */
>       uint32_t debounce_regs[ASPEED_GPIO_NR_DEBOUNCE_REGS];
> 

[PATCH 1/1] hw: aspeed_gpio: Fix GPIO array indexing

[pdel]

From: Peter Delevoryas <pdel@fb.com>

The gpio array is declared as a dense array:

  qemu_irq gpios[ASPEED_GPIO_NR_PINS];

(AST2500 has 228, AST2400 has 216, AST2600 has 208)

However, this array is used like a matrix of GPIO sets
(e.g. gpio[NR_SETS][NR_PINS_PER_SET] = gpio[8][32])

  size_t offset = set * GPIOS_PER_SET + gpio;
  qemu_set_irq(s->gpios[offset], !!(new & mask));

This can result in an out-of-bounds access to "s->gpios" because the
gpio sets do _not_ have the same length. Some of the groups (e.g.
GPIOAB) only have 4 pins. 228 != 8 * 32 == 256.

To fix this, I converted the gpio array from dense to sparse, to match
both the hardware layout and this existing indexing code.

Fixes: 4b7f956862dc2db4c5c ("hw/gpio: Add basic Aspeed GPIO model for AST2400 and AST2500")
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
 hw/gpio/aspeed_gpio.c         | 72 ++++++++++++++---------------------
 include/hw/gpio/aspeed_gpio.h |  5 +--
 2 files changed, 31 insertions(+), 46 deletions(-)

diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index dfa6d6cb40..f04d4a454c 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -16,11 +16,7 @@
 #include "hw/irq.h"
 #include "migration/vmstate.h"
 
-#define GPIOS_PER_REG 32
-#define GPIOS_PER_SET GPIOS_PER_REG
-#define GPIO_PIN_GAP_SIZE 4
 #define GPIOS_PER_GROUP 8
-#define GPIO_GROUP_SHIFT 3
 
 /* GPIO Source Types */
 #define ASPEED_CMD_SRC_MASK         0x01010101
@@ -259,7 +255,7 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
 
     diff = old ^ new;
     if (diff) {
-        for (gpio = 0; gpio < GPIOS_PER_REG; gpio++) {
+        for (gpio = 0; gpio < ASPEED_GPIOS_PER_SET; gpio++) {
             uint32_t mask = 1 << gpio;
 
             /* If the gpio needs to be updated... */
@@ -283,8 +279,7 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
             if (direction & mask) {
                 /* ...trigger the line-state IRQ */
                 ptrdiff_t set = aspeed_gpio_set_idx(s, regs);
-                size_t offset = set * GPIOS_PER_SET + gpio;
-                qemu_set_irq(s->gpios[offset], !!(new & mask));
+                qemu_set_irq(s->gpios[set][gpio], !!(new & mask));
             } else {
                 /* ...otherwise if we meet the line's current IRQ policy... */
                 if (aspeed_evaluate_irq(regs, old & mask, gpio)) {
@@ -297,21 +292,6 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
     qemu_set_irq(s->irq, !!(s->pending));
 }
 
-static uint32_t aspeed_adjust_pin(AspeedGPIOState *s, uint32_t pin)
-{
-    AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
-    /*
-     * The 2500 has a 4 pin gap in group AB and the 2400 has a 4 pin
-     * gap in group Y (and only four pins in AB but this is the last group so
-     * it doesn't matter).
-     */
-    if (agc->gap && pin >= agc->gap) {
-        pin += GPIO_PIN_GAP_SIZE;
-    }
-
-    return pin;
-}
-
 static bool aspeed_gpio_get_pin_level(AspeedGPIOState *s, uint32_t set_idx,
                                       uint32_t pin)
 {
@@ -367,7 +347,7 @@ static uint32_t update_value_control_source(GPIOSets *regs, uint32_t old_value,
     uint32_t new_value = 0;
 
     /* for each group in set */
-    for (i = 0; i < GPIOS_PER_REG; i += GPIOS_PER_GROUP) {
+    for (i = 0; i < ASPEED_GPIOS_PER_SET; i += GPIOS_PER_GROUP) {
         cmd_source = extract32(regs->cmd_source_0, i, 1)
                 | (extract32(regs->cmd_source_1, i, 1) << 1);
 
@@ -637,7 +617,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data,
          *   bidirectional  |   1       |   1        |  data
          *   input only     |   1       |   0        |   0
          *   output only    |   0       |   1        |   1
-         *   no pin / gap   |   0       |   0        |   0
+         *   no pin         |   0       |   0        |   0
          *
          *  which is captured by:
          *  data = ( data | ~input) & output;
@@ -836,14 +816,20 @@ static void aspeed_gpio_realize(DeviceState *dev, Error **errp)
     AspeedGPIOState *s = ASPEED_GPIO(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
-    int pin;
 
     /* Interrupt parent line */
     sysbus_init_irq(sbd, &s->irq);
 
     /* Individual GPIOs */
-    for (pin = 0; pin < agc->nr_gpio_pins; pin++) {
-        sysbus_init_irq(sbd, &s->gpios[pin]);
+    for (int i = 0; i < ASPEED_GPIO_MAX_NR_SETS; i++) {
+        const GPIOSetProperties *props = &agc->props[i];
+        uint32_t skip = ~(props->input | props->output);
+        for (int j = 0; j < ASPEED_GPIOS_PER_SET; j++) {
+            if (skip >> j & 1) {
+                continue;
+            }
+            sysbus_init_irq(sbd, &s->gpios[i][j]);
+        }
     }
 
     memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_gpio_ops, s,
@@ -856,20 +842,22 @@ static void aspeed_gpio_init(Object *obj)
 {
     AspeedGPIOState *s = ASPEED_GPIO(obj);
     AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
-    int pin;
-
-    for (pin = 0; pin < agc->nr_gpio_pins; pin++) {
-        char *name;
-        int set_idx = pin / GPIOS_PER_SET;
-        int pin_idx = aspeed_adjust_pin(s, pin) - (set_idx * GPIOS_PER_SET);
-        int group_idx = pin_idx >> GPIO_GROUP_SHIFT;
-        const GPIOSetProperties *props = &agc->props[set_idx];
-
-        name = g_strdup_printf("gpio%s%d", props->group_label[group_idx],
-                               pin_idx % GPIOS_PER_GROUP);
-        object_property_add(obj, name, "bool", aspeed_gpio_get_pin,
-                            aspeed_gpio_set_pin, NULL, NULL);
-        g_free(name);
+
+    for (int i = 0; i < ASPEED_GPIO_MAX_NR_SETS; i++) {
+        const GPIOSetProperties *props = &agc->props[i];
+        uint32_t skip = ~(props->input | props->output);
+        for (int j = 0; j < ASPEED_GPIOS_PER_SET; j++) {
+            if (skip >> j & 1) {
+                continue;
+            }
+            int group_idx = j / GPIOS_PER_GROUP;
+            int pin_idx = j % GPIOS_PER_GROUP;
+            const char *group = &props->group_label[group_idx][0];
+            char *name = g_strdup_printf("gpio%s%d", group, pin_idx);
+            object_property_add(obj, name, "bool", aspeed_gpio_get_pin,
+                                aspeed_gpio_set_pin, NULL, NULL);
+            g_free(name);
+        }
     }
 }
 
@@ -926,7 +914,6 @@ static void aspeed_gpio_ast2400_class_init(ObjectClass *klass, void *data)
     agc->props = ast2400_set_props;
     agc->nr_gpio_pins = 216;
     agc->nr_gpio_sets = 7;
-    agc->gap = 196;
     agc->reg_table = aspeed_3_3v_gpios;
 }
 
@@ -937,7 +924,6 @@ static void aspeed_gpio_2500_class_init(ObjectClass *klass, void *data)
     agc->props = ast2500_set_props;
     agc->nr_gpio_pins = 228;
     agc->nr_gpio_sets = 8;
-    agc->gap = 220;
     agc->reg_table = aspeed_3_3v_gpios;
 }
 
diff --git a/include/hw/gpio/aspeed_gpio.h b/include/hw/gpio/aspeed_gpio.h
index e1636ce7fe..801846befb 100644
--- a/include/hw/gpio/aspeed_gpio.h
+++ b/include/hw/gpio/aspeed_gpio.h
@@ -17,9 +17,9 @@
 OBJECT_DECLARE_TYPE(AspeedGPIOState, AspeedGPIOClass, ASPEED_GPIO)
 
 #define ASPEED_GPIO_MAX_NR_SETS 8
+#define ASPEED_GPIOS_PER_SET 32
 #define ASPEED_REGS_PER_BANK 14
 #define ASPEED_GPIO_MAX_NR_REGS (ASPEED_REGS_PER_BANK * ASPEED_GPIO_MAX_NR_SETS)
-#define ASPEED_GPIO_NR_PINS 228
 #define ASPEED_GROUPS_PER_SET 4
 #define ASPEED_GPIO_NR_DEBOUNCE_REGS 3
 #define ASPEED_CHARS_PER_GROUP_LABEL 4
@@ -60,7 +60,6 @@ struct AspeedGPIOClass {
     const GPIOSetProperties *props;
     uint32_t nr_gpio_pins;
     uint32_t nr_gpio_sets;
-    uint32_t gap;
     const AspeedGPIOReg *reg_table;
 };
 
@@ -72,7 +71,7 @@ struct AspeedGPIOState {
     MemoryRegion iomem;
     int pending;
     qemu_irq irq;
-    qemu_irq gpios[ASPEED_GPIO_NR_PINS];
+    qemu_irq gpios[ASPEED_GPIO_MAX_NR_SETS][ASPEED_GPIOS_PER_SET];
 
 /* Parallel GPIO Registers */
     uint32_t debounce_regs[ASPEED_GPIO_NR_DEBOUNCE_REGS];
-- 
2.30.2