Re: [PATCH v2 4/4] hw/display/artist.c: fix out of bounds check

[Alexander Bulekov]

Hi,
I applied this patch, but I can still trigger a segfault and heap
overread through artist_reg_write -> fill_window. I dont know if these
problems are related to what this patch fixes. If not, let me know and
I can create a separate launchpad report for these.

-Alex

(1) Segfault:
cat << EOF | ./hppa-softmmu/qemu-system-hppa -display none \
-qtest stdio -accel qtest
writeq 0xf8100a02 0x845c235c223f0584
EOF

AddressSanitizer: SEGV on unknown address 0x7fa50235cc00
#0 0x555577f8b392 in artist_rop8/hw/display/artist.c:284:14
#1 0x555577f84603 in fill_window/hw/display/artist.c:549:13
#2 0x555577f7abfc in artist_reg_write/hw/display/artist.c:895:9
#3 0x55557766d7a3 in memory_region_write_accessor/softmmu/memory.c:483:5
#4 0x55557766cadc in access_with_adjusted_size/softmmu/memory.c:539:18
#5 0x55557766a873 in memory_region_dispatch_write/softmmu/memory.c:1466:16
#6 0x555576d18056 in flatview_write_continue/exec.c:3176:23
#7 0x555576d00866 in flatview_write/exec.c:3216:14
#8 0x555576d00387 in address_space_write/exec.c:3308:18
#9 0x555577714604 in qtest_process_command/softmmu/qtest.c:452:13

===========================================================

(2) Heap Overflow:
cat << EOF | ./hppa-softmmu/qemu-system-hppa -display none -m 64 \
-qtest stdio -accel qtest
writeq 0xf8100a02 0x8cd00011900a0203
EOF

AddressSanitizer: heap-buffer-overflow on address 0x603000045bc8 at pc 
0x55bb3196f704 bp 0x7fff1c701d70 sp 0x7fff1c701d68
READ of size 8 at 0x603000045bc8 thread T0

#0 0x55bb3196f703 in 
cpu_physical_memory_set_dirty_range/include/exec/ram_addr.h:318:35
#1 0x55bb3196e6f2 in memory_region_set_dirty/softmmu/memory.c:1994:5
#2 0x55bb32279bb6 in artist_invalidate_lines/hw/display/artist.c:212:9
#3 0x55bb3227165d in fill_window/hw/display/artist.c:552:5
#4 0x55bb32267bfc in artist_reg_write/hw/display/artist.c:895:9
#5 0x55bb3195a7a3 in memory_region_write_accessor/softmmu/memory.c:483:5
#6 0x55bb31959adc in access_with_adjusted_size/softmmu/memory.c:539:18
#7 0x55bb31957873 in memory_region_dispatch_write/softmmu/memory.c:1466:16
#8 0x55bb31005056 in flatview_write_continue/exec.c:3176:23
#9 0x55bb30fed866 in flatview_write/exec.c:3216:14
#10 0x55bb30fed387 in address_space_write/exec.c:3308:18

0x603000045bc8 is located 0 bytes to the right of 24-byte region 
[0x603000045bb0,0x603000045bc8)
allocated by thread T0 here:
#0 0x55bb30f7111d in malloc ()
#1 0x7fdae3d35500 in g_malloc 
(/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x54500)
#2 0x55bb30fd84d4 in ram_block_add/exec.c:2268:9
#3 0x55bb30fded16 in qemu_ram_alloc_internal/exec.c:2441:5
#4 0x55bb30fdefed in qemu_ram_alloc/exec.c:2460:12
#5 0x55bb3195c0be in 
memory_region_init_ram_shared_nomigrate/softmmu/memory.c:1515:21
#6 0x55bb31cd6544 in ram_backend_memory_alloc/backends/hostmem-ram.c:30:5
#7 0x55bb31ccf875 in 
host_memory_backend_memory_complete/backends/hostmem.c:333:9
#8 0x55bb3360737e in user_creatable_complete/qom/object_interfaces.c:23:9
#9 0x55bb31a44e59 in create_default_memdev/softmmu/vl.c:2830:5
#10 0x55bb31a2d528 in qemu_init/softmmu/vl.c:4352:9
#11 0x55bb3405390c in main/softmmu/main.c:48:5


On 200801 1513, Helge Deller wrote:
> From: Sven Schnelle <svens@stackframe.org>
> 
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> Signed-off-by: Helge Deller <deller@gmx.de>
> ---
>  hw/display/artist.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/display/artist.c b/hw/display/artist.c
> index 6261bfe65b..de56200dbf 100644
> --- a/hw/display/artist.c
> +++ b/hw/display/artist.c
> @@ -340,14 +340,13 @@ static void vram_bit_write(ARTISTState *s, int posx, 
> int posy, bool incr_x,
>  {
>      struct vram_buffer *buf;
>      uint32_t vram_bitmask = s->vram_bitmask;
> -    int mask, i, pix_count, pix_length, offset, height, width;
> +    int mask, i, pix_count, pix_length, offset, width;
>      uint8_t *data8, *p;
> 
>      pix_count = vram_write_pix_per_transfer(s);
>      pix_length = vram_pixel_length(s);
> 
>      buf = vram_write_buffer(s);
> -    height = buf->height;
>      width = buf->width;
> 
>      if (s->cmap_bm_access) {
> @@ -367,13 +366,6 @@ static void vram_bit_write(ARTISTState *s, int posx, int 
> posy, bool incr_x,
>          pix_count = size * 8;
>      }
> 
> -    if (posy * width + posx + pix_count > buf->size) {
> -        qemu_log("write outside bounds: wants %dx%d, max size %dx%d\n",
> -                 posx, posy, width, height);
> -        return;
> -    }
> -
> -
>      switch (pix_length) {
>      case 0:
>          if (s->image_bitmap_op & 0x20000000) {
> @@ -381,8 +373,11 @@ static void vram_bit_write(ARTISTState *s, int posx, int 
> posy, bool incr_x,
>          }
> 
>          for (i = 0; i < pix_count; i++) {
> -            artist_rop8(s, p + offset + pix_count - 1 - i,
> -                        (data & 1) ? (s->plane_mask >> 24) : 0);
> +            uint32_t off = offset + pix_count - 1 - i;
> +            if (off < buf->size) {
> +                artist_rop8(s, p + off,
> +                            (data & 1) ? (s->plane_mask >> 24) : 0);
> +            }
>              data >>= 1;
>          }
>          memory_region_set_dirty(&buf->mr, offset, pix_count);
> @@ -398,7 +393,10 @@ static void vram_bit_write(ARTISTState *s, int posx, int 
> posy, bool incr_x,
>          for (i = 3; i >= 0; i--) {
>              if (!(s->image_bitmap_op & 0x20000000) ||
>                  s->vram_bitmask & (1 << (28 + i))) {
> -                artist_rop8(s, p + offset + 3 - i, data8[ROP8OFF(i)]);
> +                uint32_t off = offset + 3 - i;
> +                if (off < buf->size) {
> +                    artist_rop8(s, p + off, data8[ROP8OFF(i)]);
> +                }
>              }
>          }
>          memory_region_set_dirty(&buf->mr, offset, 3);
> @@ -420,7 +418,7 @@ static void vram_bit_write(ARTISTState *s, int posx, int 
> posy, bool incr_x,
>              break;
>          }
> 
> -        for (i = 0; i < pix_count; i++) {
> +        for (i = 0; i < pix_count && offset + i < buf->size; i++) {
>              mask = 1 << (pix_count - 1 - i);
> 
>              if (!(s->image_bitmap_op & 0x20000000) ||
> --
> 2.21.3
> 
>