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

[Alexander Bulekov]

Hi Helge, Sven,
I think this patch introduces an issue:

cat << EOF | ./hppa-softmmu/qemu-system-hppa -m 64 -display none \
-qtest stdio -accel qtest
writeq 0xf810049f 0x7ed7087fffffff0d
writew 0xf8118001 0x30fb
writew 0xf8118000 0x5bfb
writeq 0xf81005fb 0xd8d8d8d83d83d6
EOF

AddressSanitizer:DEADLYSIGNAL
=================================================================
==440==ERROR: AddressSanitizer: SEGV on unknown address 0x7fc4f3c87fff (pc 
0x55a0e0ef024f bp 0x7ffcf40a8740 sp 0x7ffcf40a7f00 T0)
==440==The signal is caused by a WRITE memory access.
    #0 0x55a0e0ef024f in vram_bit_write /hw/display/artist.c:397:39
    #1 0x55a0e0ee82ab in artist_reg_write /hw/display/artist.c:886:9
    #2 0x55a0e05dc7a3 in memory_region_write_accessor /softmmu/memory.c:483:5
    #3 0x55a0e05dbadc in access_with_adjusted_size /softmmu/memory.c:539:18
    #4 0x55a0e05d9873 in memory_region_dispatch_write /softmmu/memory.c:1466:16
    #5 0x55a0dfc87056 in flatview_write_continue /exec.c:3176:23
    #6 0x55a0dfc6f866 in flatview_write /exec.c:3216:14
    #7 0x55a0dfc6f387 in address_space_write /exec.c:3308:18
    #8 0x55a0e0683604 in qtest_process_command /softmmu/qtest.c:452:13
    #9 0x55a0e067ac08 in qtest_process_inbuf /softmmu/qtest.c:710:9
    #10 0x55a0e0679895 in qtest_read /softmmu/qtest.c:722:5
    #11 0x55a0e2b35c43 in qemu_chr_be_write_impl /chardev/char.c:188:9
    #12 0x55a0e2b35dc7 in qemu_chr_be_write /chardev/char.c:200:9
    #13 0x55a0e2b4a0b3 in fd_chr_read /chardev/char-fd.c:68:9
    #14 0x55a0e2c9e474 in qio_channel_fd_source_dispatch 
/io/channel-watch.c:84:12
    #15 0x7fc52b5d3897 in g_main_context_dispatch 
(/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4e897)
    #16 0x55a0e309637b in glib_pollfds_poll /util/main-loop.c:217:9
    #17 0x55a0e3093aab in os_host_main_loop_wait /util/main-loop.c:240:5
    #18 0x55a0e3093444 in main_loop_wait /util/main-loop.c:516:11
    #19 0x55a0e069bd00 in qemu_main_loop /softmmu/vl.c:1676:9
    #20 0x55a0e2cd6261 in main /softmmu/main.c:49:5
    #21 0x7fc52a159e0a in __libc_start_main 
/build/glibc-GwnBeO/glibc-2.30/csu/../csu/libc-start.c:308:16
    #22 0x55a0dfb7a729 in _start 
(/home/alxndr/Development/qemu/general-fuzz/build/hppa-softmmu/qemu-system-hppa+0x22d4729)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /hw/display/artist.c:397:39 in vram_bit_write
==440==ABORTING

I bisected it to this patch.
Thanks
-Alex

On 200804 1600, Helge Deller wrote:
> From: Sven Schnelle <svens@stackframe.org>
> 
> Fix the following runtime warning with artist framebuffer:
> "write outside bounds: wants 1256x1023, max size 1280x1024"
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.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
> 
>