> On Oct 4, 2021, at 4:43 AM, Cédric Le Goater <c...@kaod.org> wrote:
>
> On 10/4/21 11:07, Cédric Le Goater wrote:
>> On 9/28/21 05:43, p...@fb.com wrote:
>>> From: Peter Delevoryas <p...@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 <p...@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.
