[PATCH v4] hw/gpio/aspeed: Add reg_table_count to AspeedGPIOClass

[PATCH v4] hw/gpio/aspeed: Add reg_table_count to AspeedGPIOClass

[Zheyu Ma]

ASan detected a global-buffer-overflow error in the aspeed_gpio_read()
function. This issue occurred when reading beyond the bounds of the
reg_table.

To enhance the safety and maintainability of the Aspeed GPIO code, this commit
introduces a reg_table_count member to the AspeedGPIOClass structure. This
change ensures that the size of the GPIO register table is explicitly tracked
and initialized, reducing the risk of errors if new register tables are
introduced in the future.

Reproducer:
cat << EOF | qemu-system-aarch64 -display none \
-machine accel=qtest, -m 512M -machine ast1030-evb -qtest stdio
readq 0x7e780272
EOF

ASAN log indicating the issue:
==2602930==ERROR: AddressSanitizer: global-buffer-overflow on address 0x55a5da29e128 at pc 0x55a5d700dc62 bp 0x7fff096c4e90 sp 0x7fff096c4e88
READ of size 2 at 0x55a5da29e128 thread T0
    #0 0x55a5d700dc61 in aspeed_gpio_read hw/gpio/aspeed_gpio.c:564:14
    #1 0x55a5d933f3ab in memory_region_read_accessor system/memory.c:445:11
    #2 0x55a5d92fba40 in access_with_adjusted_size system/memory.c:573:18
    #3 0x55a5d92f842c in memory_region_dispatch_read1 system/memory.c:1426:16
    #4 0x55a5d92f7b68 in memory_region_dispatch_read system/memory.c:1459:9
    #5 0x55a5d9376ad1 in flatview_read_continue_step system/physmem.c:2836:18
    #6 0x55a5d9376399 in flatview_read_continue system/physmem.c:2877:19
    #7 0x55a5d93775b8 in flatview_read system/physmem.c:2907:12

Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
---
Changes in v4:
- Change the variable name to 'reg_table_count'
- Change the 'reg_table_count' type to unsigned
Changes in v3:
- Add the reproducer
---
 hw/gpio/aspeed_gpio.c         | 17 +++++++++++++++++
 include/hw/gpio/aspeed_gpio.h |  1 +
 2 files changed, 18 insertions(+)

diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index c1781e2ba3..6474bb8de5 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -559,6 +559,12 @@ static uint64_t aspeed_gpio_read(void *opaque, hwaddr offset, uint32_t size)
         return debounce_value;
     }
 
+    if (idx >= agc->reg_table_count) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: idx 0x%" PRIx64 " out of bounds\n",
+                      __func__, idx);
+        return 0;
+    }
+
     reg = &agc->reg_table[idx];
     if (reg->set_idx >= agc->nr_gpio_sets) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: no getter for offset 0x%"
@@ -785,6 +791,12 @@ static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data,
         return;
     }
 
+    if (idx >= agc->reg_table_count) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: idx 0x%" PRIx64 " out of bounds\n",
+                      __func__, idx);
+        return;
+    }
+
     reg = &agc->reg_table[idx];
     if (reg->set_idx >= agc->nr_gpio_sets) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: no setter for offset 0x%"
@@ -1117,6 +1129,7 @@ static void aspeed_gpio_ast2400_class_init(ObjectClass *klass, void *data)
     agc->nr_gpio_pins = 216;
     agc->nr_gpio_sets = 7;
     agc->reg_table = aspeed_3_3v_gpios;
+    agc->reg_table_count = GPIO_3_3V_REG_ARRAY_SIZE;
 }
 
 static void aspeed_gpio_2500_class_init(ObjectClass *klass, void *data)
@@ -1127,6 +1140,7 @@ static void aspeed_gpio_2500_class_init(ObjectClass *klass, void *data)
     agc->nr_gpio_pins = 228;
     agc->nr_gpio_sets = 8;
     agc->reg_table = aspeed_3_3v_gpios;
+    agc->reg_table_count = GPIO_3_3V_REG_ARRAY_SIZE;
 }
 
 static void aspeed_gpio_ast2600_3_3v_class_init(ObjectClass *klass, void *data)
@@ -1137,6 +1151,7 @@ static void aspeed_gpio_ast2600_3_3v_class_init(ObjectClass *klass, void *data)
     agc->nr_gpio_pins = 208;
     agc->nr_gpio_sets = 7;
     agc->reg_table = aspeed_3_3v_gpios;
+    agc->reg_table_count = GPIO_3_3V_REG_ARRAY_SIZE;
 }
 
 static void aspeed_gpio_ast2600_1_8v_class_init(ObjectClass *klass, void *data)
@@ -1147,6 +1162,7 @@ static void aspeed_gpio_ast2600_1_8v_class_init(ObjectClass *klass, void *data)
     agc->nr_gpio_pins = 36;
     agc->nr_gpio_sets = 2;
     agc->reg_table = aspeed_1_8v_gpios;
+    agc->reg_table_count = GPIO_1_8V_REG_ARRAY_SIZE;
 }
 
 static void aspeed_gpio_1030_class_init(ObjectClass *klass, void *data)
@@ -1157,6 +1173,7 @@ static void aspeed_gpio_1030_class_init(ObjectClass *klass, void *data)
     agc->nr_gpio_pins = 151;
     agc->nr_gpio_sets = 6;
     agc->reg_table = aspeed_3_3v_gpios;
+    agc->reg_table_count = GPIO_3_3V_REG_ARRAY_SIZE;
 }
 
 static const TypeInfo aspeed_gpio_info = {
diff --git a/include/hw/gpio/aspeed_gpio.h b/include/hw/gpio/aspeed_gpio.h
index 904eecf62c..90a12ae318 100644
--- a/include/hw/gpio/aspeed_gpio.h
+++ b/include/hw/gpio/aspeed_gpio.h
@@ -75,6 +75,7 @@ struct AspeedGPIOClass {
     uint32_t nr_gpio_pins;
     uint32_t nr_gpio_sets;
     const AspeedGPIOReg *reg_table;
+    unsigned reg_table_count;
 };
 
 struct AspeedGPIOState {
-- 
2.34.1

Re: [PATCH v4] hw/gpio/aspeed: Add reg_table_count to AspeedGPIOClass

[Philippe Mathieu-Daudé]

On 20/6/24 16:02, Zheyu Ma wrote:
> ASan detected a global-buffer-overflow error in the aspeed_gpio_read()
> function. This issue occurred when reading beyond the bounds of the
> reg_table.
> 
> To enhance the safety and maintainability of the Aspeed GPIO code, this commit
> introduces a reg_table_count member to the AspeedGPIOClass structure. This
> change ensures that the size of the GPIO register table is explicitly tracked
> and initialized, reducing the risk of errors if new register tables are
> introduced in the future.
> 
> Reproducer:
> cat << EOF | qemu-system-aarch64 -display none \
> -machine accel=qtest, -m 512M -machine ast1030-evb -qtest stdio
> readq 0x7e780272
> EOF
> 
> ASAN log indicating the issue:
> ==2602930==ERROR: AddressSanitizer: global-buffer-overflow on address 0x55a5da29e128 at pc 0x55a5d700dc62 bp 0x7fff096c4e90 sp 0x7fff096c4e88
> READ of size 2 at 0x55a5da29e128 thread T0
>      #0 0x55a5d700dc61 in aspeed_gpio_read hw/gpio/aspeed_gpio.c:564:14
>      #1 0x55a5d933f3ab in memory_region_read_accessor system/memory.c:445:11
>      #2 0x55a5d92fba40 in access_with_adjusted_size system/memory.c:573:18
>      #3 0x55a5d92f842c in memory_region_dispatch_read1 system/memory.c:1426:16
>      #4 0x55a5d92f7b68 in memory_region_dispatch_read system/memory.c:1459:9
>      #5 0x55a5d9376ad1 in flatview_read_continue_step system/physmem.c:2836:18
>      #6 0x55a5d9376399 in flatview_read_continue system/physmem.c:2877:19
>      #7 0x55a5d93775b8 in flatview_read system/physmem.c:2907:12
> 
> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> ---
> Changes in v4:
> - Change the variable name to 'reg_table_count'
> - Change the 'reg_table_count' type to unsigned

Thanks,

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> Changes in v3:
> - Add the reproducer
> ---
>   hw/gpio/aspeed_gpio.c         | 17 +++++++++++++++++
>   include/hw/gpio/aspeed_gpio.h |  1 +
>   2 files changed, 18 insertions(+)

Re: [PATCH v4] hw/gpio/aspeed: Add reg_table_count to AspeedGPIOClass

[Philippe Mathieu-Daudé]

On 20/6/24 16:20, Philippe Mathieu-Daudé wrote:
> On 20/6/24 16:02, Zheyu Ma wrote:
>> ASan detected a global-buffer-overflow error in the aspeed_gpio_read()
>> function. This issue occurred when reading beyond the bounds of the
>> reg_table.
>>
>> To enhance the safety and maintainability of the Aspeed GPIO code, 
>> this commit
>> introduces a reg_table_count member to the AspeedGPIOClass structure. 
>> This
>> change ensures that the size of the GPIO register table is explicitly 
>> tracked
>> and initialized, reducing the risk of errors if new register tables are
>> introduced in the future.
>>
>> Reproducer:
>> cat << EOF | qemu-system-aarch64 -display none \
>> -machine accel=qtest, -m 512M -machine ast1030-evb -qtest stdio
>> readq 0x7e780272
>> EOF
>>
>> ASAN log indicating the issue:
>> ==2602930==ERROR: AddressSanitizer: global-buffer-overflow on address 
>> 0x55a5da29e128 at pc 0x55a5d700dc62 bp 0x7fff096c4e90 sp 0x7fff096c4e88
>> READ of size 2 at 0x55a5da29e128 thread T0
>>      #0 0x55a5d700dc61 in aspeed_gpio_read hw/gpio/aspeed_gpio.c:564:14
>>      #1 0x55a5d933f3ab in memory_region_read_accessor 
>> system/memory.c:445:11
>>      #2 0x55a5d92fba40 in access_with_adjusted_size 
>> system/memory.c:573:18
>>      #3 0x55a5d92f842c in memory_region_dispatch_read1 
>> system/memory.c:1426:16
>>      #4 0x55a5d92f7b68 in memory_region_dispatch_read 
>> system/memory.c:1459:9
>>      #5 0x55a5d9376ad1 in flatview_read_continue_step 
>> system/physmem.c:2836:18
>>      #6 0x55a5d9376399 in flatview_read_continue system/physmem.c:2877:19
>>      #7 0x55a5d93775b8 in flatview_read system/physmem.c:2907:12
>>

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2355

>> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
>> ---
>> Changes in v4:
>> - Change the variable name to 'reg_table_count'
>> - Change the 'reg_table_count' type to unsigned
> 
> Thanks,
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
>> Changes in v3:
>> - Add the reproducer
>> ---
>>   hw/gpio/aspeed_gpio.c         | 17 +++++++++++++++++
>>   include/hw/gpio/aspeed_gpio.h |  1 +
>>   2 files changed, 18 insertions(+)
> 

Re: [PATCH v4] hw/gpio/aspeed: Add reg_table_count to AspeedGPIOClass

[Andrew Jeffery]

On Thu, 2024-06-20 at 16:02 +0200, Zheyu Ma wrote:
> ASan detected a global-buffer-overflow error in the aspeed_gpio_read()
> function. This issue occurred when reading beyond the bounds of the
> reg_table.
> 
> To enhance the safety and maintainability of the Aspeed GPIO code, this commit
> introduces a reg_table_count member to the AspeedGPIOClass structure. This
> change ensures that the size of the GPIO register table is explicitly tracked
> and initialized, reducing the risk of errors if new register tables are
> introduced in the future.
> 
> Reproducer:
> cat << EOF | qemu-system-aarch64 -display none \
> -machine accel=qtest, -m 512M -machine ast1030-evb -qtest stdio
> readq 0x7e780272
> EOF
> 
> ASAN log indicating the issue:
> ==2602930==ERROR: AddressSanitizer: global-buffer-overflow on address 0x55a5da29e128 at pc 0x55a5d700dc62 bp 0x7fff096c4e90 sp 0x7fff096c4e88
> READ of size 2 at 0x55a5da29e128 thread T0
>     #0 0x55a5d700dc61 in aspeed_gpio_read hw/gpio/aspeed_gpio.c:564:14
>     #1 0x55a5d933f3ab in memory_region_read_accessor system/memory.c:445:11
>     #2 0x55a5d92fba40 in access_with_adjusted_size system/memory.c:573:18
>     #3 0x55a5d92f842c in memory_region_dispatch_read1 system/memory.c:1426:16
>     #4 0x55a5d92f7b68 in memory_region_dispatch_read system/memory.c:1459:9
>     #5 0x55a5d9376ad1 in flatview_read_continue_step system/physmem.c:2836:18
>     #6 0x55a5d9376399 in flatview_read_continue system/physmem.c:2877:19
>     #7 0x55a5d93775b8 in flatview_read system/physmem.c:2907:12
> 
> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>

Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>

Re: [PATCH v4] hw/gpio/aspeed: Add reg_table_count to AspeedGPIOClass

[Cédric Le Goater]

On 6/20/24 4:02 PM, Zheyu Ma wrote:
> ASan detected a global-buffer-overflow error in the aspeed_gpio_read()
> function. This issue occurred when reading beyond the bounds of the
> reg_table.
> 
> To enhance the safety and maintainability of the Aspeed GPIO code, this commit
> introduces a reg_table_count member to the AspeedGPIOClass structure. This
> change ensures that the size of the GPIO register table is explicitly tracked
> and initialized, reducing the risk of errors if new register tables are
> introduced in the future.
> 
> Reproducer:
> cat << EOF | qemu-system-aarch64 -display none \
> -machine accel=qtest, -m 512M -machine ast1030-evb -qtest stdio
> readq 0x7e780272
> EOF
> 
> ASAN log indicating the issue:
> ==2602930==ERROR: AddressSanitizer: global-buffer-overflow on address 0x55a5da29e128 at pc 0x55a5d700dc62 bp 0x7fff096c4e90 sp 0x7fff096c4e88
> READ of size 2 at 0x55a5da29e128 thread T0
>      #0 0x55a5d700dc61 in aspeed_gpio_read hw/gpio/aspeed_gpio.c:564:14
>      #1 0x55a5d933f3ab in memory_region_read_accessor system/memory.c:445:11
>      #2 0x55a5d92fba40 in access_with_adjusted_size system/memory.c:573:18
>      #3 0x55a5d92f842c in memory_region_dispatch_read1 system/memory.c:1426:16
>      #4 0x55a5d92f7b68 in memory_region_dispatch_read system/memory.c:1459:9
>      #5 0x55a5d9376ad1 in flatview_read_continue_step system/physmem.c:2836:18
>      #6 0x55a5d9376399 in flatview_read_continue system/physmem.c:2877:19
>      #7 0x55a5d93775b8 in flatview_read system/physmem.c:2907:12
> 
> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>

Applied to aspeed-next.

Thanks,

C.


> ---
> Changes in v4:
> - Change the variable name to 'reg_table_count'
> - Change the 'reg_table_count' type to unsigned
> Changes in v3:
> - Add the reproducer
> ---
>   hw/gpio/aspeed_gpio.c         | 17 +++++++++++++++++
>   include/hw/gpio/aspeed_gpio.h |  1 +
>   2 files changed, 18 insertions(+)
> 
> diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> index c1781e2ba3..6474bb8de5 100644
> --- a/hw/gpio/aspeed_gpio.c
> +++ b/hw/gpio/aspeed_gpio.c
> @@ -559,6 +559,12 @@ static uint64_t aspeed_gpio_read(void *opaque, hwaddr offset, uint32_t size)
>           return debounce_value;
>       }
>   
> +    if (idx >= agc->reg_table_count) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: idx 0x%" PRIx64 " out of bounds\n",
> +                      __func__, idx);
> +        return 0;
> +    }
> +
>       reg = &agc->reg_table[idx];
>       if (reg->set_idx >= agc->nr_gpio_sets) {
>           qemu_log_mask(LOG_GUEST_ERROR, "%s: no getter for offset 0x%"
> @@ -785,6 +791,12 @@ static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data,
>           return;
>       }
>   
> +    if (idx >= agc->reg_table_count) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: idx 0x%" PRIx64 " out of bounds\n",
> +                      __func__, idx);
> +        return;
> +    }
> +
>       reg = &agc->reg_table[idx];
>       if (reg->set_idx >= agc->nr_gpio_sets) {
>           qemu_log_mask(LOG_GUEST_ERROR, "%s: no setter for offset 0x%"
> @@ -1117,6 +1129,7 @@ static void aspeed_gpio_ast2400_class_init(ObjectClass *klass, void *data)
>       agc->nr_gpio_pins = 216;
>       agc->nr_gpio_sets = 7;
>       agc->reg_table = aspeed_3_3v_gpios;
> +    agc->reg_table_count = GPIO_3_3V_REG_ARRAY_SIZE;
>   }
>   
>   static void aspeed_gpio_2500_class_init(ObjectClass *klass, void *data)
> @@ -1127,6 +1140,7 @@ static void aspeed_gpio_2500_class_init(ObjectClass *klass, void *data)
>       agc->nr_gpio_pins = 228;
>       agc->nr_gpio_sets = 8;
>       agc->reg_table = aspeed_3_3v_gpios;
> +    agc->reg_table_count = GPIO_3_3V_REG_ARRAY_SIZE;
>   }
>   
>   static void aspeed_gpio_ast2600_3_3v_class_init(ObjectClass *klass, void *data)
> @@ -1137,6 +1151,7 @@ static void aspeed_gpio_ast2600_3_3v_class_init(ObjectClass *klass, void *data)
>       agc->nr_gpio_pins = 208;
>       agc->nr_gpio_sets = 7;
>       agc->reg_table = aspeed_3_3v_gpios;
> +    agc->reg_table_count = GPIO_3_3V_REG_ARRAY_SIZE;
>   }
>   
>   static void aspeed_gpio_ast2600_1_8v_class_init(ObjectClass *klass, void *data)
> @@ -1147,6 +1162,7 @@ static void aspeed_gpio_ast2600_1_8v_class_init(ObjectClass *klass, void *data)
>       agc->nr_gpio_pins = 36;
>       agc->nr_gpio_sets = 2;
>       agc->reg_table = aspeed_1_8v_gpios;
> +    agc->reg_table_count = GPIO_1_8V_REG_ARRAY_SIZE;
>   }
>   
>   static void aspeed_gpio_1030_class_init(ObjectClass *klass, void *data)
> @@ -1157,6 +1173,7 @@ static void aspeed_gpio_1030_class_init(ObjectClass *klass, void *data)
>       agc->nr_gpio_pins = 151;
>       agc->nr_gpio_sets = 6;
>       agc->reg_table = aspeed_3_3v_gpios;
> +    agc->reg_table_count = GPIO_3_3V_REG_ARRAY_SIZE;
>   }
>   
>   static const TypeInfo aspeed_gpio_info = {
> diff --git a/include/hw/gpio/aspeed_gpio.h b/include/hw/gpio/aspeed_gpio.h
> index 904eecf62c..90a12ae318 100644
> --- a/include/hw/gpio/aspeed_gpio.h
> +++ b/include/hw/gpio/aspeed_gpio.h
> @@ -75,6 +75,7 @@ struct AspeedGPIOClass {
>       uint32_t nr_gpio_pins;
>       uint32_t nr_gpio_sets;
>       const AspeedGPIOReg *reg_table;
> +    unsigned reg_table_count;
>   };
>   
>   struct AspeedGPIOState {