| Message ID | 20240618130928.3075494-1-zheyuma97@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hw/gpio/aspeed: Add bounds checking for register table access | expand |
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
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
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
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 > >
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%"
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(+)