| Message ID | 20240703155633.17352-1-zheyuma97@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] hw/display/sm501: Validate local memory size index in sm501_system_config_write | expand |
On Wed, 3 Jul 2024, Zheyu Ma wrote: > Ensure that the local_mem_size_index is within valid bounds and does not > exceed the allocated memory size before updating it in sm501_system_config_write > to prevent out-of-bounds read. > > ASAN log: > ==3067247==ERROR: AddressSanitizer: global-buffer-overflow on address 0x55c6586e4d3c at pc 0x55c655d4e0ac bp 0x7ffc9d5c6a10 sp 0x7ffc9d5c6a08 > READ of size 4 at 0x55c6586e4d3c thread T0 > #0 0x55c655d4e0ab in sm501_2d_operation qemu/hw/display/sm501.c:729:21 > #1 0x55c655d4b8a1 in sm501_2d_engine_write qemu/hw/display/sm501.c:1551:13 > > Reproducer: > cat << EOF | qemu-system-x86_64 \ > -display none -machine accel=qtest, -m 512M -machine q35 -nodefaults \ > -device sm501 -qtest stdio > outl 0xcf8 0x80000814 > outl 0xcfc 0xe4000000 > outl 0xcf8 0x80000804 > outw 0xcfc 0x02 > writel 0xe4000010 0xe000 > writel 0xe4100010 0x10000 > writel 0xe4100008 0x10001 > writel 0xe410000c 0x80000000 > EOF > > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com> > --- > Changes in v2: > - Also check the memory_region_size bound > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com> > --- > hw/display/sm501.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/hw/display/sm501.c b/hw/display/sm501.c > index 26dc8170d8..a878c35dd9 100644 > --- a/hw/display/sm501.c > +++ b/hw/display/sm501.c > @@ -1020,11 +1020,21 @@ static void sm501_system_config_write(void *opaque, hwaddr addr, > s->gpio_63_32_control = value & 0xFF80FFFF; > break; > case SM501_DRAM_CONTROL: > - s->local_mem_size_index = (value >> 13) & 0x7; > - /* TODO : check validity of size change */ > + { > + int local_mem_size_index = (value >> 13) & 0x7; > + if (local_mem_size_index < ARRAY_SIZE(sm501_mem_local_size) && Sorry for keep asking for more changes but now the error is not very specific. Maybe it's better to invert these checks as: if index >= ARRAY_SIZE log error invalid index else if selected size > local_mem_region size log_error memory size cannot be more than vram_size else set size index endif But then it can be that the value in dram_control and s->local_mem_size_index no longer match so you may need to also change sm501_system_config_read() case SM501_DRAM_CONTROL: to remove the mask from there and just return the dram_control value the guest has set and not combine it with mem size from index (keep the mask in write though to mask out non r/w bits). I don't know what the real chip does in this case when one overwrires this value with larger than the installed RAM so don't know what would the correct behaviour be in this case. Regards, BALATON Zoltan > + sm501_mem_local_size[local_mem_size_index] <= > + memory_region_size(&s->local_mem_region)) { > + s->local_mem_size_index = local_mem_size_index; > + } else { > + qemu_log_mask(LOG_GUEST_ERROR, > + "sm501: Invalid local_mem_size_index value: %d or memory size too large\n", > + local_mem_size_index); > + } > s->dram_control &= 0x80000000; > s->dram_control |= value & 0x7FFFFFC3; > break; > + } > case SM501_ARBTRTN_CONTROL: > s->arbitration_control = value & 0x37777777; > break; >
diff --git a/hw/display/sm501.c b/hw/display/sm501.c index 26dc8170d8..a878c35dd9 100644 --- a/hw/display/sm501.c +++ b/hw/display/sm501.c @@ -1020,11 +1020,21 @@ static void sm501_system_config_write(void *opaque, hwaddr addr, s->gpio_63_32_control = value & 0xFF80FFFF; break; case SM501_DRAM_CONTROL: - s->local_mem_size_index = (value >> 13) & 0x7; - /* TODO : check validity of size change */ + { + int local_mem_size_index = (value >> 13) & 0x7; + if (local_mem_size_index < ARRAY_SIZE(sm501_mem_local_size) && + sm501_mem_local_size[local_mem_size_index] <= + memory_region_size(&s->local_mem_region)) { + s->local_mem_size_index = local_mem_size_index; + } else { + qemu_log_mask(LOG_GUEST_ERROR, + "sm501: Invalid local_mem_size_index value: %d or memory size too large\n", + local_mem_size_index); + } s->dram_control &= 0x80000000; s->dram_control |= value & 0x7FFFFFC3; break; + } case SM501_ARBTRTN_CONTROL: s->arbitration_control = value & 0x37777777; break;
Ensure that the local_mem_size_index is within valid bounds and does not exceed the allocated memory size before updating it in sm501_system_config_write to prevent out-of-bounds read. ASAN log: ==3067247==ERROR: AddressSanitizer: global-buffer-overflow on address 0x55c6586e4d3c at pc 0x55c655d4e0ac bp 0x7ffc9d5c6a10 sp 0x7ffc9d5c6a08 READ of size 4 at 0x55c6586e4d3c thread T0 #0 0x55c655d4e0ab in sm501_2d_operation qemu/hw/display/sm501.c:729:21 #1 0x55c655d4b8a1 in sm501_2d_engine_write qemu/hw/display/sm501.c:1551:13 Reproducer: cat << EOF | qemu-system-x86_64 \ -display none -machine accel=qtest, -m 512M -machine q35 -nodefaults \ -device sm501 -qtest stdio outl 0xcf8 0x80000814 outl 0xcfc 0xe4000000 outl 0xcf8 0x80000804 outw 0xcfc 0x02 writel 0xe4000010 0xe000 writel 0xe4100010 0x10000 writel 0xe4100008 0x10001 writel 0xe410000c 0x80000000 EOF Signed-off-by: Zheyu Ma <zheyuma97@gmail.com> --- Changes in v2: - Also check the memory_region_size bound Signed-off-by: Zheyu Ma <zheyuma97@gmail.com> --- hw/display/sm501.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)