Hi Andrew,
On Wed, Jun 19, 2024 at 1:58 AM Andrew Jeffery <and...@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 <zheyum...@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
