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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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