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.