On 19/6/24 08:49, Zheyu Ma wrote:
Hi Andrew,
On Wed, Jun 19, 2024 at 1:58 AM Andrew Jeffery
<and...@codeconstruct.com.au <mailto: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.
Could you share the reproducer? (As you did in your other patches,
it is very useful to reproduce).
>
> Signed-off-by: Zheyu Ma <zheyum...@gmail.com
<mailto: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
