[PATCH] hw/gpio/aspeed: Add bounds checking for register table access

[PATCH] hw/gpio/aspeed: Add bounds checking for register table access

[Zheyu Ma]

Added bounds checking in the aspeed_gpio_read() and aspeed_gpio_write()
functions to ensure the index idx is within the valid range of the
reg_table array.

The correct size of reg_table is determined dynamically based on whether
it is aspeed_3_3v_gpios or aspeed_1_8v_gpios. If idx exceeds the
size of reg_table, an error is logged, and the function returns.

AddressSanitizer 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>
---
 hw/gpio/aspeed_gpio.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index c1781e2ba3..1441046f6c 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -550,6 +550,7 @@ static uint64_t aspeed_gpio_read(void *opaque, hwaddr offset, uint32_t size)
     GPIOSets *set;
     uint32_t value = 0;
     uint64_t debounce_value;
+    uint32_t reg_table_size;
 
     idx = offset >> 2;
     if (idx >= GPIO_DEBOUNCE_TIME_1 && idx <= GPIO_DEBOUNCE_TIME_3) {
@@ -559,6 +560,18 @@ static uint64_t aspeed_gpio_read(void *opaque, hwaddr offset, uint32_t size)
         return debounce_value;
     }
 
+    if (agc->reg_table == aspeed_3_3v_gpios) {
+        reg_table_size = GPIO_3_3V_REG_ARRAY_SIZE;
+    } else {
+        reg_table_size = GPIO_1_8V_REG_ARRAY_SIZE;
+    }
+
+    if (idx >= reg_table_size) {
+        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%"
@@ -768,6 +781,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data,
     const AspeedGPIOReg *reg;
     GPIOSets *set;
     uint32_t cleared;
+    uint32_t reg_table_size;
 
     trace_aspeed_gpio_write(offset, data);
 
@@ -785,6 +799,18 @@ static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data,
         return;
     }
 
+    if (agc->reg_table == aspeed_3_3v_gpios) {
+        reg_table_size = GPIO_3_3V_REG_ARRAY_SIZE;
+    } else {
+        reg_table_size = GPIO_1_8V_REG_ARRAY_SIZE;
+    }
+
+    if (idx >= reg_table_size) {
+        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%"
-- 
2.34.1

Re: [PATCH] hw/gpio/aspeed: Add bounds checking for register table access

[Andrew Jeffery]

Hello Zheyu Ma,

On Tue, 2024-06-18 at 15:09 +0200, Zheyu Ma wrote:
> Added bounds checking in the aspeed_gpio_read() and aspeed_gpio_write()
> functions to ensure the index idx is within the valid range of the
> reg_table array.
> 
> The correct size of reg_table is determined dynamically based on whether
> it is aspeed_3_3v_gpios or aspeed_1_8v_gpios. If idx exceeds the
> size of reg_table, an error is logged, and the function returns.
> 
> AddressSanitizer 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

I'm mildly interested in what you were doing to trigger this. Certainly
we could do with a guard in the model to prevent it, but I'm curious
all the same.

> 
> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> ---
>  hw/gpio/aspeed_gpio.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> index c1781e2ba3..1441046f6c 100644
> --- a/hw/gpio/aspeed_gpio.c
> +++ b/hw/gpio/aspeed_gpio.c
> @@ -550,6 +550,7 @@ static uint64_t aspeed_gpio_read(void *opaque, hwaddr offset, uint32_t size)
>      GPIOSets *set;
>      uint32_t value = 0;
>      uint64_t debounce_value;
> +    uint32_t reg_table_size;
>  
>      idx = offset >> 2;
>      if (idx >= GPIO_DEBOUNCE_TIME_1 && idx <= GPIO_DEBOUNCE_TIME_3) {
> @@ -559,6 +560,18 @@ static uint64_t aspeed_gpio_read(void *opaque, hwaddr offset, uint32_t size)
>          return debounce_value;
>      }
>  
> +    if (agc->reg_table == aspeed_3_3v_gpios) {
> +        reg_table_size = GPIO_3_3V_REG_ARRAY_SIZE;
> +    } else {
> +        reg_table_size = GPIO_1_8V_REG_ARRAY_SIZE;
> +    }

I think I'd prefer we add reg_table_size as a member of AspeedGPIOClass
and initialise it at the same time as we initialise reg_table. I feel
it would help maintain safety in the face of future changes (i.e. if
another reg table were introduced). With that approach the hunk above
can be dropped.

> +
> +    if (idx >= reg_table_size) {

This condition would then become:

```
if (idx >= agc->reg_table_size) {
```

Thoughts?

Andrew

Re: [PATCH] hw/gpio/aspeed: Add bounds checking for register table access

[Zheyu Ma]

[-- Attachment #1: Type: text/plain, Size: 3356 bytes --]

Hi Andrew,

On Wed, Jun 19, 2024 at 1:58 AM Andrew Jeffery <andrew@codeconstruct.com.au>
wrote:

> Hello Zheyu Ma,
>
> On Tue, 2024-06-18 at 15:09 +0200, Zheyu Ma wrote:
> > Added bounds checking in the aspeed_gpio_read() and aspeed_gpio_write()
> > functions to ensure the index idx is within the valid range of the
> > reg_table array.
> >
> > The correct size of reg_table is determined dynamically based on whether
> > it is aspeed_3_3v_gpios or aspeed_1_8v_gpios. If idx exceeds the
> > size of reg_table, an error is logged, and the function returns.
> >
> > AddressSanitizer 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
>
> I'm mildly interested in what you were doing to trigger this. Certainly
> we could do with a guard in the model to prevent it, but I'm curious
> all the same.
>

Actually, I'm doing the virtual device fuzzing test and trying to discover
bugs.

>
> >
> > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> > ---
> >  hw/gpio/aspeed_gpio.c | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> > index c1781e2ba3..1441046f6c 100644
> > --- a/hw/gpio/aspeed_gpio.c
> > +++ b/hw/gpio/aspeed_gpio.c
> > @@ -550,6 +550,7 @@ static uint64_t aspeed_gpio_read(void *opaque,
> hwaddr offset, uint32_t size)
> >      GPIOSets *set;
> >      uint32_t value = 0;
> >      uint64_t debounce_value;
> > +    uint32_t reg_table_size;
> >
> >      idx = offset >> 2;
> >      if (idx >= GPIO_DEBOUNCE_TIME_1 && idx <= GPIO_DEBOUNCE_TIME_3) {
> > @@ -559,6 +560,18 @@ static uint64_t aspeed_gpio_read(void *opaque,
> hwaddr offset, uint32_t size)
> >          return debounce_value;
> >      }
> >
> > +    if (agc->reg_table == aspeed_3_3v_gpios) {
> > +        reg_table_size = GPIO_3_3V_REG_ARRAY_SIZE;
> > +    } else {
> > +        reg_table_size = GPIO_1_8V_REG_ARRAY_SIZE;
> > +    }
>
> I think I'd prefer we add reg_table_size as a member of AspeedGPIOClass
> and initialise it at the same time as we initialise reg_table. I feel
> it would help maintain safety in the face of future changes (i.e. if
> another reg table were introduced). With that approach the hunk above
> can be dropped.
>
> > +
> > +    if (idx >= reg_table_size) {
>
> This condition would then become:
>
> ```
> if (idx >= agc->reg_table_size) {
> ```
>
> Thoughts?
>

I agree with you, adding a new member is a more maintainable way, I'll send
a v2 patch, thanks!

Zheyu

[-- Attachment #2: Type: text/html, Size: 4316 bytes --]

Re: [PATCH] hw/gpio/aspeed: Add bounds checking for register table access

[Philippe Mathieu-Daudé]

On 19/6/24 08:49, Zheyu Ma wrote:
> Hi Andrew,
> 
> On Wed, Jun 19, 2024 at 1:58 AM Andrew Jeffery 
> <andrew@codeconstruct.com.au <mailto:andrew@codeconstruct.com.au>> wrote:
> 
>     Hello Zheyu Ma,
> 
>     On Tue, 2024-06-18 at 15:09 +0200, Zheyu Ma wrote:
>      > Added bounds checking in the aspeed_gpio_read() and
>     aspeed_gpio_write()
>      > functions to ensure the index idx is within the valid range of the
>      > reg_table array.
>      >
>      > The correct size of reg_table is determined dynamically based on
>     whether
>      > it is aspeed_3_3v_gpios or aspeed_1_8v_gpios. If idx exceeds the
>      > size of reg_table, an error is logged, and the function returns.
>      >
>      > AddressSanitizer 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
> 
>     I'm mildly interested in what you were doing to trigger this. Certainly
>     we could do with a guard in the model to prevent it, but I'm curious
>     all the same.
> 
> 
> Actually, I'm doing the virtual device fuzzing test and trying to 
> discover bugs.

Could you share the reproducer? (As you did in your other patches,
it is very useful to reproduce).

> 
>      >
>      > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com
>     <mailto:zheyuma97@gmail.com>>
>      > ---
>      >  hw/gpio/aspeed_gpio.c | 26 ++++++++++++++++++++++++++
>      >  1 file changed, 26 insertions(+)
>      >
>      > diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
>      > index c1781e2ba3..1441046f6c 100644
>      > --- a/hw/gpio/aspeed_gpio.c
>      > +++ b/hw/gpio/aspeed_gpio.c
>      > @@ -550,6 +550,7 @@ static uint64_t aspeed_gpio_read(void
>     *opaque, hwaddr offset, uint32_t size)
>      >      GPIOSets *set;
>      >      uint32_t value = 0;
>      >      uint64_t debounce_value;
>      > +    uint32_t reg_table_size;
>      >
>      >      idx = offset >> 2;
>      >      if (idx >= GPIO_DEBOUNCE_TIME_1 && idx <=
>     GPIO_DEBOUNCE_TIME_3) {
>      > @@ -559,6 +560,18 @@ static uint64_t aspeed_gpio_read(void
>     *opaque, hwaddr offset, uint32_t size)
>      >          return debounce_value;
>      >      }
>      >
>      > +    if (agc->reg_table == aspeed_3_3v_gpios) {
>      > +        reg_table_size = GPIO_3_3V_REG_ARRAY_SIZE;
>      > +    } else {
>      > +        reg_table_size = GPIO_1_8V_REG_ARRAY_SIZE;
>      > +    }
> 
>     I think I'd prefer we add reg_table_size as a member of AspeedGPIOClass
>     and initialise it at the same time as we initialise reg_table. I feel
>     it would help maintain safety in the face of future changes (i.e. if
>     another reg table were introduced). With that approach the hunk above
>     can be dropped.
> 
>      > +
>      > +    if (idx >= reg_table_size) {
> 
>     This condition would then become:
> 
>     ```
>     if (idx >= agc->reg_table_size) {
>     ```
> 
>     Thoughts?
> 
> 
> I agree with you, adding a new member is a more maintainable way, I'll 
> send a v2 patch, thanks!
> 
> Zheyu

Re: [PATCH] hw/gpio/aspeed: Add bounds checking for register table access

[Zheyu Ma]

[-- Attachment #1: Type: text/plain, Size: 4348 bytes --]

Hi Philippe,

On Wed, Jun 19, 2024 at 6:29 PM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> On 19/6/24 08:49, Zheyu Ma wrote:
> > Hi Andrew,
> >
> > On Wed, Jun 19, 2024 at 1:58 AM Andrew Jeffery
> > <andrew@codeconstruct.com.au <mailto:andrew@codeconstruct.com.au>>
> wrote:
> >
> >     Hello Zheyu Ma,
> >
> >     On Tue, 2024-06-18 at 15:09 +0200, Zheyu Ma wrote:
> >      > Added bounds checking in the aspeed_gpio_read() and
> >     aspeed_gpio_write()
> >      > functions to ensure the index idx is within the valid range of the
> >      > reg_table array.
> >      >
> >      > The correct size of reg_table is determined dynamically based on
> >     whether
> >      > it is aspeed_3_3v_gpios or aspeed_1_8v_gpios. If idx exceeds the
> >      > size of reg_table, an error is logged, and the function returns.
> >      >
> >      > AddressSanitizer 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
> >
> >     I'm mildly interested in what you were doing to trigger this.
> Certainly
> >     we could do with a guard in the model to prevent it, but I'm curious
> >     all the same.
> >
> >
> > Actually, I'm doing the virtual device fuzzing test and trying to
> > discover bugs.
>
> Could you share the reproducer? (As you did in your other patches,
> it is very useful to reproduce).
>

Sure, I've sent a v3 patch.

Zheyu

>
> >
> >      >
> >      > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com
> >     <mailto:zheyuma97@gmail.com>>
> >      > ---
> >      >  hw/gpio/aspeed_gpio.c | 26 ++++++++++++++++++++++++++
> >      >  1 file changed, 26 insertions(+)
> >      >
> >      > diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> >      > index c1781e2ba3..1441046f6c 100644
> >      > --- a/hw/gpio/aspeed_gpio.c
> >      > +++ b/hw/gpio/aspeed_gpio.c
> >      > @@ -550,6 +550,7 @@ static uint64_t aspeed_gpio_read(void
> >     *opaque, hwaddr offset, uint32_t size)
> >      >      GPIOSets *set;
> >      >      uint32_t value = 0;
> >      >      uint64_t debounce_value;
> >      > +    uint32_t reg_table_size;
> >      >
> >      >      idx = offset >> 2;
> >      >      if (idx >= GPIO_DEBOUNCE_TIME_1 && idx <=
> >     GPIO_DEBOUNCE_TIME_3) {
> >      > @@ -559,6 +560,18 @@ static uint64_t aspeed_gpio_read(void
> >     *opaque, hwaddr offset, uint32_t size)
> >      >          return debounce_value;
> >      >      }
> >      >
> >      > +    if (agc->reg_table == aspeed_3_3v_gpios) {
> >      > +        reg_table_size = GPIO_3_3V_REG_ARRAY_SIZE;
> >      > +    } else {
> >      > +        reg_table_size = GPIO_1_8V_REG_ARRAY_SIZE;
> >      > +    }
> >
> >     I think I'd prefer we add reg_table_size as a member of
> AspeedGPIOClass
> >     and initialise it at the same time as we initialise reg_table. I feel
> >     it would help maintain safety in the face of future changes (i.e. if
> >     another reg table were introduced). With that approach the hunk above
> >     can be dropped.
> >
> >      > +
> >      > +    if (idx >= reg_table_size) {
> >
> >     This condition would then become:
> >
> >     ```
> >     if (idx >= agc->reg_table_size) {
> >     ```
> >
> >     Thoughts?
> >
> >
> > I agree with you, adding a new member is a more maintainable way, I'll
> > send a v2 patch, thanks!
> >
> > Zheyu
>
>

[-- Attachment #2: Type: text/html, Size: 6114 bytes --]